Consider Scrollable location in text selection drag events #102992
Conversation
flutter-dashboard
bot
added
a: text input
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
I know passing around BuildContext isn't ideal.. looking a bit more into this, it looks like you could do this another way.
Let me know if I am off here, but it looks like the Scrollable in EditableTextState is the one you are after. Instead of looking it up using a context, it may be cleaner to refactor the ScrollController logic of that Scrollable.
If you were able to access the scroll controller, you could access the position similarly without a build context.
I am not super familiar with editable text, but from what I can tell, a user-provided scroll controller is passed down from the higher level classes like TextField.scrollController and CupertinoTextField.scrollController. If the user has not provided a ScrollController, one is created all the way down in EditableTextState as a fallback.
If instead, you were to take the fallback creation out of EditableTextState and instead create the fallback controller in the higher level classes, the controller (whether it is user provided or the fallback) can be passed to the *TextSelectionGestureDetectorBuilders and then in turn passed further down to EditableTextState. That would remove the need to call Scrollable.of, and no passing around a context that could become invalid.
What do you think? Let me know if you have questions or if I've been unclear. :)
|
@Piinks Actually I explicitly need the Scrollable above the EditableText, not inside of it. Previously we were compensating only for scrolling inside of the EditableText (done here with renderEditable.offset). This PR aims to compensate for any scrolling happening above the EditableText in the widget tree. The issue has an example. I'll see if I can do this in a cleaner way at all. I'll tag you for re-review. |
|
Oh! Ok, sorry I did not realize. :) So there may, or may not, be an enclosing Scrollable above the TextField. I think in this case then, |
justinmc
force-pushed
the
input-scrollable-offset
branch
from
a7775bf to
c357cc3
Compare
|
Hey @justinmc, is this PR still on your radar? |
|
Ah I had forgotten about it, will bring it back to life. |
|
@Piinks This is ready for review again. Sorry for the long delay since you last reviewed, but I've now used the |
| testWidgets('Mouse edge scrolling works in an outer scrollable', (WidgetTester tester) async { | ||
| // Regression test for https://github.com/flutter/flutter/issues/102484 | ||
| final TextEditingController controller = TextEditingController( | ||
| text: 'I love flutter!\n' * 8, |
| final ScrollableState? scrollableState = | ||
| delegate.editableTextKey.currentContext == null | ||
| ? null | ||
| : Scrollable.of(delegate.editableTextKey.currentContext!); | ||
| final double currentScrollDy = scrollableState == null | ||
| ? 0.0 | ||
| : scrollableState.position.pixels; |
There was a problem hiding this comment.
Can this and the same chunk of lines starting at 2122 be wrapped up in a method?
Co-authored-by: Kate Lovett <[email protected]>
|
I tried it and this seems to only fully fix #104500 on desktop. I'll update that issue. |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Thanks for checking on the other issue. This LGTM!
3 participants
Previously, edge scrolling was incorrect if it caused a Scrollable to scroll. Our math only considered scrolling the RenderEditable itself, but not a containing Scrollable. This PR fixes this by taking both into consideration.
The problem
Drag gestures in GestureDetector don't consider scrolling that happens during the event, as demonstrated in this dartpad. Because of this, we have to compensate for the scrolling manually when calculating the selection area during edge scrolling.
The scrolling location is not manually calculated, it's just done via renderEditable.showOnScreen, so we don't have the calculated scroll position anywhere.
Previously, this compensation was done only for scrolling that happens inside the RenderEditable (here). However, it's also possible that a tall text input in a small Scrollable causes the Scrollable to scroll (example in the issue), not the RenderEditable, and this must be compensated for, too.
The current solution
Grab the scroll position using
Scrollable.of(context).position.pixelsand use it to compensate for any scrolling when calculating the selection Rect.Related
Fixes #102484
Partially fixes #104500 (desktop only fix)