added new ScrollViewKeyboardDismissBehavior variant onScroll with tests #160507
Conversation
edf0219 to
047dea7
Compare
| @@ -44,6 +44,9 @@ enum ScrollViewKeyboardDismissBehavior { | |||
| /// `onDrag` means that the [ScrollView] will dismiss an on-screen keyboard | |||
| /// when a drag begins. | |||
| onDrag, | |||
| /// `noDrag` means that the [ScrollView] will dismiss an on-screen keyboard | |||
| /// when a scroll begins irrespective of a drag. | |||
| noDrag, | |||
There was a problem hiding this comment.
With noDrag, will the on-screen keyboard be dismissed on any scroll? If yes, onScroll might be a better name for it, since dragging is only one of many ways to scroll, so we probably don't want a 'drag'-specific name.
There was a problem hiding this comment.
Sure, onScroll makes sense.
There was a problem hiding this comment.
In the documentation for this new value, we should mention that when the keyboard opens and pushes the field to make it still visible, it will send a scroll notification that will cause the field to immediately be unfocused. Possibly breadcrumb to the issue, at least in a private comment.
There was a problem hiding this comment.
The said notification sends a scroll without a drag, so after we merge this PR
- It will work as expected to the users who will use
ScrollViewKeyboardDismissBehavior.onDragbecause field will not be unfocused (because there is no drag). - but field will be unfocused immediately for the users who used
ScrollViewKeyboardDismissBehavior.onScoll(irrespective of the drag).
Not sure about the right place for the documentation but as you said we can add breadcrumb to the issues #154515 or #150048 for the information on when to use onScroll vs onDrag.
There was a problem hiding this comment.
We can just add to the documentation for onScroll that it should be used to dismiss the keyboard on all scrolls (with a drag, trackpad, mouse wheel etc). And then add the caveat that because we are watching for all scroll notifications, programmatic scrolls will also dismiss the keyboard (e.g the keyboard scrolling to keep the field visible, etc). That should be enough in the API docs. Then in the implementation we can just add a comment to breadcrumb to the relevant issue.
There was a problem hiding this comment.
@victorsanni renamed the variant and updated comment.
|
Thanks for the PR. I'm leaning towards just reverting #154675 and reopening the linked issues. The core problem is that we have no way of tracking trackpad scrolls with ScrollNotification, only drag scrolls (via |
I played around with it a little more and I saw that drags still dismiss the keyboard. However, the issue occurs when the focused field is within the bounds of where the keyboard appears, so when the keyboard opens it scrolls to push the field up so that it remains visible. That sends a scroll notification that then unfocuses the field. Instead of increasing the API surface, can we add a fix to ignore these types of scrolls when unfocusing? |
|
Some scrolls produces a drag while others not. I think increasing the API surface will make the usage clear as to when to dismiss the keyboard If we don't want to increase the API surface, we need to unfocus the field when it goes out of the view which will fix #154515, #150048. This way we can revert #154675. |
|
It makes sense to add an The long-term solution imo should be to expose if the on-screen keyboard's opening triggered a scroll notification, and just ignore those notifications. But that would be quite an undertaking, so as long as we warn users clearly enough I'm fine with moving forward with this change. |
|
@victorsanni Currently we don't have enough information in the notification to find trigger behind the scroll. I think making it available will be a significant change. However if we merge this PR, it actually reverses #154675 and allows users to control the dismiss behavior |
I agree with @victorsanni. After that we can revisit this and determine how to fix the original issues properly. |
047dea7 to
73ae283
Compare
| /// Caveat: Since we are watching for all scroll notifications, programmatic | ||
| /// scrolls will also dismiss the keyboard (e.g opening of on-screen keyboard | ||
| /// produces a scroll to keep the field visible). | ||
| onScroll, |
There was a problem hiding this comment.
Hi @nick9822. The linked issues are both in the Autocomplete widget. Can we explore a fix contained within that widget?
Alternatively, in NotificationListener.onNotification:
- @Piinks suggested an approach to expose what device triggered the notification, or
- Expose if a scroll was triggered programmatically or manually.
- or another solution I can't currently think of.
The bug in #160331 is quite severe, and propagating it into a new enum value doesn't seem like the right fix even if we do warn users.
There was a problem hiding this comment.
@victorsanni I don't think this PR propagate the issue #160331, for that matter after the recent regression issue #160331 and this PR have no relation. This just extends the API surface giving user a choice to strictly dismiss keyboard on scroll.
I agree both issues #154515, #150048 are around Autocomplete and we can explore fix in that widget. #150048 actually makes sense because those scrolls doesn't produce any taps and hence focus remain unchanged specifically on Desktops. But #154515 seems like a UI defect which I think needs a fix.
Not sure on how @Piinks suggestions will help.
There was a problem hiding this comment.
I don't think this PR propagate the issue
The issue is that programmatic scrolls will dismiss the keyboard. If a user uses .onScroll, with this implementation, unintended programmatic scrolls like autoscrolling to keep a text field visible will dismiss the keyboard too, which is undesired. That's the crux of #160331.
If .onScroll can filter unintended programmatic scrolls from intended scrolls, then I think it would be a nice addition.
There was a problem hiding this comment.
okay, I will try to filter programmatic scrolls.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@nick9822 Just checking, do you still plan to return to finish this PR? |
|
@nick9822 Another friendly reminder here on this PR, do you plan to return to it? No worries if you still need time. |
0fbc070 to
0a0b6a9
Compare
It's in progress, will be ready for review soon. |
F438 ts including one previously absent.
…rogrammatic scrolls.
0a0b6a9 to
fed964e
Compare
| @@ -225,11 +231,15 @@ abstract class ViewportOffset extends ChangeNotifier { | |||
| /// like [ScrollPosition] handle it by adjusting [to] to prevent over or | |||
| /// underscroll. | |||
| Future<void> moveTo(double to, {Duration? duration, Curve? curve, bool? clamp}) { | |||
| isProgrammaticScroll = true; | |||
There was a problem hiding this comment.
I don't know that this is necessarily true. The user could call any of the methods here, moveTo, jumpTo, animateTo - all of those would be programmatic. What is the difference here?
Scroll actions, like in response to keyboard input call moveTo, scrollbars use moveTo, as do the methods for showing something in a viewport.
There was a problem hiding this comment.
I saw moveTo being used when the screen is being scrolled programmatically especially when the soft-keyboard is opened and the text field needs to be in focus. I think moveTo is only used when scroll happens without user directly intending to scroll (mostly show in viewport cases) only exception is scrollbar taps where page is scrolled towards the direction. In scroll taps we could have directly used animateTo instead of moveTo.
Anyways as you said jumpTo, animateTo are also programmatic in a sense, may be the name should be unintendedScroll. This was attempt towards the last review by @victorsanni
The issue is that programmatic scrolls will dismiss the keyboard. If a user uses .onScroll, with this implementation, unintended programmatic scrolls like autoscrolling to keep a text field visible will dismiss the keyboard too, which is undesired. That's the crux of #160331.
If .onScroll can filter unintended programmatic scrolls from intended scrolls, then I think it would be a nice addition.
I think filtering out unintendedScroll should be the target here.
There was a problem hiding this comment.
I don't think moveTo can be assumed to always be used this way. It is public API that anyone could be using in any number of ways, like I mentioned with scrollbars. That is calling moveTo in response to the user dragging on the scrollbar. So I don't think this is a safe assumption to make unfortunately.
| @@ -119,6 +119,12 @@ abstract class ViewportOffset extends ChangeNotifier { | |||
| /// correction. | |||
| factory ViewportOffset.zero() = _FixedViewportOffset.zero; | |||
|
|
|||
| /// Whether the current offset change was due to programmatic scroll. | |||
There was a problem hiding this comment.
What is a programmatic scroll?
There was a problem hiding this comment.
Automated scrolls (non-user initiated)
Previously
ScrollViewKeyboardDismissBehaviorwas limited in handling scroll events, proposal in this PR is to introduce a new variantonScrollwhich is for dismissing on-screen keyboard for all user-initiated scrolls which includes (non-drag) scrolls common on desktops (mouse scroll, two-finger scrolls) (#154515).Pre-launch Checklist
///).