-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[webview_flutter] Adds support to control whether to draw scrollbars #9024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to split up with one addition; once that's added you can go ahead and split out the sub PRs.
/// Whether the horizontal scrollbar should be drawn or not. | ||
Future<void> setHorizontalScrollBarEnabled(bool enabled) { | ||
return platform.setHorizontalScrollBarEnabled(enabled); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also plumb through a Future<bool> supportsSetScrollBarsEnabled()
method to the platform interface layer that clients can use to detect whether this is safe to call per https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#api-support-queries ? This won't make sense on all platforms, so we should expect that clients will need a support query indefinitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also assuming you meant for this to return bool
and not Future<bool>
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I missed this, and didn't catch it in the platform interface review. It should have been Future<bool>
in case there were a platform that could only determine this at runtime (e.g., based on the OS version).
Not a big deal though as that's an unlikely case to ever hit.
…raw the scrollbar (#9125) Part of flutter/flutter#62464 Platform interface portion for #9024 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…set whether to draw the scrollbar (#9249) Platform implementations of #9024 Part of flutter/flutter#62464 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one small change.
/// should be drawn or not. | ||
/// | ||
/// See [setVerticalScrollBarEnabled] and [setHorizontalScrollBarEnabled]. | ||
bool supportsSetScrollBarsEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's still make this Future<bool>
at this layer so that in the unlikely event that we have to change things we can keep the user-facing API the same, and only add a duplicate method at the platform interface layer.
flutter/packages@58d4016...af0b9a9 2025-05-19 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android][webview_flutter_wkwebview] Adds support to respond to recoverable SSL certificate errors (flutter/packages#9281) 2025-05-19 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump org.json:json from 20250107 to 20250517 in /packages/in_app_purchase/in_app_purchase_android/example/android/app (flutter/packages#9283) 2025-05-19 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump org.json:json from 20250107 to 20250517 in /packages/in_app_purchase/in_app_purchase/example/android/app (flutter/packages#9282) 2025-05-16 10687576+bparrishMines@users.noreply.github.com [webview_flutter_platform_interface] Adds support to respond to recoverable SSL certificate errors (flutter/packages#9248) 2025-05-16 10687576+bparrishMines@users.noreply.github.com [webview_flutter] Adds support to control whether to draw scrollbars (flutter/packages#9024) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…raw the scrollbar (flutter#9125) Part of flutter/flutter#62464 Platform interface portion for flutter#9024 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…set whether to draw the scrollbar (flutter#9249) Platform implementations of flutter#9024 Part of flutter/flutter#62464 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…lutter#9024) Fixes flutter/flutter#62464 Updated version of flutter#8174 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Fixes flutter/flutter#62464
Updated version of #8174
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3