Repository navigation
Stop using SelectionChangedCause internally to show the text selection toolbar - #27534
Conversation
goderbauer
added
framework
xster
added
the
a: text input
0b4791e to
f489feb
Compare
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This looks like a fine improvement over the way things were.
Only non-trivial feedback I have is: shouldn't the RenderEditable methods that have a cause parameter allow cause: null (or another enum value) for changes that were not triggered by an input event?
| tester.state<EditableTextState>(find.byType(EditableText)); | ||
|
|
||
| expect(state.showToolbar(), false); | ||
| await tester.pump(); |
There was a problem hiding this comment.
Shouldn't this be pumpAndSettle()? What if the toolbar animates into view?
There was a problem hiding this comment.
Visual behavior changes should be test-breaking changes.
| // Select something. Doesn't really matter what. | ||
| state.renderEditable.selectWordsInRange( | ||
| from: const Offset(0, 0), | ||
| cause: SelectionChangedCause.tap, |
There was a problem hiding this comment.
If this parameter needed? Since we're now being honest with the cause parameter, it seems like there should a special value (or just cause == null) that indicates a programmatic change.
There was a problem hiding this comment.
I'm not deprecating SelectionChangedCause (my branch name is a misnomer I guess :D). We're still trying to be honest™️ with the real cause to supply it to users of the existing APIs via the various onSelectionChanged and whatnot. We're just not using it internally anymore, but we're not breaking external users.
In this test, I'm intending for it to mean a single tap.
I'm not making the select*'s cause optional because they still aren't (to keep supporting onSelectionChanged users who might care).
So far, there are no 'fake' selection causes yet to have a new enum value.
There was a problem hiding this comment.
So far, there are no 'fake' selection causes yet to have a new enum value.
What about programmatic changes to the selection that are not triggered by an input event?
There was a problem hiding this comment.
Programmatic changes actually circumvents the whole thing it appears. i.e. nothing is triggered through the RenderEditable directly, no on*Changed is bubbled back through EditableText, there are no SelectionChangedCause involved, no toolbars are ever triggered and no on*Changed is called.
This is in the various @template flutter.widgets.editableText.onChanged docs. This callback does run not when the TextField's text is changed, via the TextField's [controller].
Adding some additional docs to RenderEditable.select* to indicate that it's used for translating UI manipulations to logical selections and if you own the TextEditingController, it's a lot easier to go manipulate that directly.
There was a problem hiding this comment.
Thanks for clarifying that. So the rough summary would be: the RenderEditable selection changing methods are only called in response to user input. That makes sense.
There was a problem hiding this comment.
It's a good question though. I only thought about it after you asked. But the new docs should address the apparent redundancy now.
| expect(find.text('PASTE'), findsNothing); | ||
|
|
||
| controller.text = 'blah'; | ||
| expect(state.showToolbar(), false); |
There was a problem hiding this comment.
Maybe pump a frame first, so that 'blah' actually ends up in the textfield?
|
PTAL |
|
I added some more docs to RenderEditable. Take a look to see if it's any helpful. |
| // Select something. Doesn't really matter what. | ||
| state.renderEditable.selectWordsInRange( | ||
| from: const Offset(0, 0), | ||
| cause: SelectionChangedCause.tap, |
There was a problem hiding this comment.
Thanks for clarifying that. So the rough summary would be: the RenderEditable selection changing methods are only called in response to user input. That makes sense.
4 participants
Stop passing the SelectionChangedCause from (Cupertino)TextField all the way down through EditableText into RenderEditable, then bubble back up to EditableTextState to decide whether to show a toolbar (since it depends on more than just the SelectionChangedCause).
Let (Cupertino)TextField just invoke it directly when factors are satisfied.
#26026
It's a soft API break. (Cupertino)TextField will continue to work as is. Various listeners using SelectionChangedHandler will also continue to work as is.
Users using EditableText directly with
rendererIgnoresPointer = falseor who calls the descendent RenderEditable'sselect*will also need to call EditableTextState.showToolbar() directly.