Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@math1man
Copy link
Contributor

@math1man math1man commented Mar 29, 2021

Fixes a bug where scroll bars on the Android non-hybrid WebView are rendered on the wrong side of the screen.

fixes flutter/flutter#79312
fixes flutter/flutter#66208

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…e comment here, this layout direction affects the rendering of the scroll bar.
@google-cla google-cla bot added the cla: yes label Mar 29, 2021
@github-actions github-actions bot added the p: webview_flutter Edits files for a webview_flutter plugin label Mar 29, 2021
// WebView content is not affected by the Android view's layout direction,
// we explicitly set it here so that the widget doesn't require an ambient
// directionality.
layoutDirection: TextDirection.rtl,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you tested it with older Android versions? https://github.com/flutter/plugins/pull/1618/files suggests that this is necessary for some version of Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing anywhere in that PR that specifically talks about this choice, or any indication that it would affect older Android versions. I'll do my best to test on a couple versions of Android though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested on M, O, and R, and haven't seen any negative side effects.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if Amir meant TextDirection.ltr.

I noted is that this is technically a small breaking change as now a parent widget must provide the layout direction. I don't know how common it's, but by looking at the changes to the unit test, I wonder if changing this value to TextDirection.ltr removes potential failures in other unit tests that we don't own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsurprisingly, that introduces the inverse problem that in an RTL locale, the scroll bars are on the wrong side.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also use https://api.flutter.dev/flutter/widgets/Directionality/maybeOf.html to detect the current direction, and if null we set LTR as the default. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh that's a neat trick. I left the default as RTL to minimize side effects, but I can swap to LTR if you'd prefer.

@math1man math1man marked this pull request as ready for review March 31, 2021 15:59
…t internally specify a layoutDirection, it must have a Directionality widget as an ancestor to derive the direction from."

This reverts commit 793902f.

# Conflicts:
#	packages/webview_flutter/test/webview_flutter_test.dart
…tDirection without introducing a possibly-breaking change.
Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes p: webview_flutter Edits files for a webview_flutter plugin

Projects

None yet

3 participants