[webview_flutter] Enable WebView programmatic scrolling. #2533

EitanSchwartz · 2020-02-16T10:53:20Z

Description

This commit adds the following WebView functionality:
scrollTo(int x, int y), scrollBy(int x, int y), getScrollX() & getScrollY().
The new code uses Android's WebView.scrollTo() & iOS WKWebView.scrollView.contentOffset.

Related Issues

flutter/flutter48708
flutter/flutter48717
flutter/flutter48720

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

…ew.scrollTo & iOS WKWebView.scrollView.contentOffset.

…ew.scrollTo & iOS WKWebView.scrollView.contentOffset. - Merged upstream back into branch.

amirh

Thanks! this looks very good! Sorry for the delay in reviewing I've been swamped recently.

I mostly had formatting nits.

amirh · 2020-04-24T16:08:24Z

packages/webview_flutter/example/test_driver/webview_flutter_e2e.dart

+      
+            <script type="text/javascript">
+              function config() {
+                //make sure scrolling is enabled on both x & y.


nit: I'm a little confused by the comment, is this make sure scrolling is enabled, or is it making sure that the page's content is big enough to require scrolling?

Thanks for note, you're correct. Changed to: "Create a page with dimensions big enough to allow scrolling on both x & y." in coming commit.

amirh · 2020-04-24T16:09:54Z

packages/webview_flutter/example/test_driver/webview_flutter_e2e.dart

+      
+            <script type="text/javascript">
+              function config() {
+                //make sure scrolling is enabled on both x & y.


uber nit: missing space after the // (same for single line Dart comments below)

Fixed in coming commit.

amirh · 2020-04-24T16:17:33Z

packages/webview_flutter/lib/webview_flutter.dart

    return _webViewPlatformController.getTitle();
  }
+
+  /// Set the scrolled position of this view.


nit: empty line after the summary for all Dartdoc comments.

https://dart.dev/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph

Of course, thx for pointing this out. Fixed in coming commit.

amirh · 2020-04-24T16:18:32Z

packages/webview_flutter/lib/webview_flutter.dart

  }
+
+  /// Set the scrolled position of this view.
+  /// 'x' - the x position to scroll to.


What is the unit for x and y? is it the same unit across platforms?

In the coming commit, I adjusted the comment to state pixel specifically.
I was not sure at first iOS also works in pixels but looking into it, the "point" unit they require eventually translates into pixel.

In Apple's official docs they name the unit as 'Point' and the value is translated using CGFloat but other articles proclaim that Apple rounds these points since graphical rendering cannot split pixels. Eventually it means iOS is using pixels as well.

Sources:

https://developer.apple.com/documentation/uikit/uiscrollview/1619404-contentoffset

http://andrewmarinov.com/uiscrollviews-contentoffset-precision/

Additionally it looks like common working practice is to treat this input as pixels:

https://stackoverflow.com/questions/3719753/iphone-uiscrollview-speed-check

https://stackoverflow.com/questions/48319873/uiscrollview-resting-at-incorrect-contentoffset-20-px-not-0px

Thanks!
We should emphasize that that these are pixels in the webview space (e.g these are not Flutter's logical pixels)

Hey Amir, I'm not sure what the best phrasing is, suggesting:
"the x position, in pixels, to scroll the WebView content to."
Same for y position?

What do you think about:

/// Sets the webview's content scroll position. /// /// The parameters `x` and `y` specify the scroll position in webview pixels.

100%, Added in incoming commit.

Merge branch 'upstream_master' into EnableProgrammaticScrollingInWebView # Conflicts: # AUTHORS # packages/webview_flutter/CHANGELOG.md # packages/webview_flutter/pubspec.yaml

amirh · 2020-04-27T15:27:37Z

packages/webview_flutter/lib/platform_interface.dart

        "WebView getTitle is not implemented on the current platform");
  }
+
+  /// Set the scrolled position of this view.


Empty line after the summary for all Dartdoc comments

Added in incoming commit.

Also made a pass to look for any other comments I introduced and correct this if needed.

amirh · 2020-04-27T15:31:23Z

packages/webview_flutter/lib/webview_flutter.dart

  }
+
+  /// Set the scrolled position of this view.
+  /// 'x' - the x position to scroll to.


Thanks!
We should emphasize that that these are pixels in the webview space (e.g these are not Flutter's logical pixels)

amirh

LGTM modulo some comment nits.

Thank you!

amirh · 2020-04-27T15:32:12Z

packages/webview_flutter/lib/platform_interface.dart

  }
+
+  /// Set the scrolled position of this view.
+  /// 'x' - the x position to scroll to.


Document the unit here as well, this documentation may be the only thing a platform implementor is reading.

Added in incoming commit.

amirh · 2020-04-27T15:32:56Z

packages/webview_flutter/lib/platform_interface.dart

        "WebView getTitle is not implemented on the current platform");
  }
+
+  /// Set the scrolled position of this view.


amirh · 2020-04-27T15:33:51Z

packages/webview_flutter/lib/webview_flutter.dart

+
+  /// Return the horizontal scroll position of this view.
+  ///
+  /// Scroll position is measured from left.


Mention the returned unit here and on getScrollY

Added in incoming commit.

amirh

LGTM, thanks!

Waiting for flutter/flutter#55743 (comment) before landing.

EitanSchwartz requested a review from amirh as a code owner February 16, 2020 10:53

googlebot added the cla: yes label Feb 16, 2020

[webview_flutter] Enable programmatic scrolling using Android's WebVi…

1f34e03

…ew.scrollTo & iOS WKWebView.scrollView.contentOffset.

EitanSchwartz force-pushed the EnableProgrammaticScrollingInWebView branch from c86317c to 1f34e03 Compare February 16, 2020 12:51

[webview_flutter] Enable programmatic scrolling using Android's WebVi…

202e373

…ew.scrollTo & iOS WKWebView.scrollView.contentOffset. - Merged upstream back into branch.

amirh reviewed Apr 24, 2020

View reviewed changes

Adjusted and clarified comments/Dartdoc to better fit standards.

c4aa91d

Merge branch 'upstream_master' into EnableProgrammaticScrollingInWebView # Conflicts: # AUTHORS # packages/webview_flutter/CHANGELOG.md # packages/webview_flutter/pubspec.yaml

amirh reviewed Apr 27, 2020

View reviewed changes

amirh approved these changes Apr 27, 2020

View reviewed changes

Final adjustments to comments/Dartdoc to better fit standards.

065458a

amirh approved these changes Apr 27, 2020

View reviewed changes

amirh merged commit a0b692d into flutter:master Apr 27, 2020

amirh added a commit to amirh/plugins that referenced this pull request Apr 27, 2020

[webview_flutter] Fix the version update for flutter#2533

1a68485

amirh mentioned this pull request Apr 27, 2020

[webview_flutter] Fix the version update for #2533 #2711

Merged

amirh added a commit that referenced this pull request Apr 27, 2020

[webview_flutter] Fix the version update for #2533 (#2711)

58f5b7a

cyanglaz mentioned this pull request May 12, 2020

[webview_flutter] setAndGetScrollPosition is flaky on ci. flutter/flutter#57041

Closed

slightfoot mentioned this pull request May 14, 2020

[webview_flutter] Addition of WebView Scrolling #2762

Closed

13 tasks

EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020

[webview_flutter] Enable WebView programmatic scrolling. (flutter#2533)

52e0860

EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020

[webview_flutter] Fix the version update for flutter#2533 (flutter#2711)

2105bec

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[webview_flutter] Enable WebView programmatic scrolling. (flutter#2533)

376c8d6

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[webview_flutter] Fix the version update for flutter#2533 (flutter#2711)

9739c6a

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[webview_flutter] Enable WebView programmatic scrolling. (flutter#2533)

68eae6e

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[webview_flutter] Fix the version update for flutter#2533 (flutter#2711)

968a6d7

[webview_flutter] Enable WebView programmatic scrolling. #2533

[webview_flutter] Enable WebView programmatic scrolling. #2533

Uh oh!

Conversation

EitanSchwartz commented Feb 16, 2020

Description

Related Issues

Checklist

Breaking Change

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EitanSchwartz Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EitanSchwartz Apr 27, 2020 •

edited

Loading