[web] Don't show handles when selection change is caused by keyboard #65127
Conversation
mdebbar
added
a: text input
| @@ -1643,7 +1643,11 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien | |||
| } | |||
| } | |||
|
|
|||
| _formatAndSetValue(value); | |||
| if (_isSelectionOnlyChange(value)) { | |||
There was a problem hiding this comment.
Does the web go through the text input client instead of raw keyboard event?
There was a problem hiding this comment.
Yes. All keyboard interactions with the text field are handled by the browser and we only receive the result of the change. e.g. If the user clicks "shift+right arrow", we receive an event from the browser telling us that selection has changed, so we read the new selection state from the input field and we send it to Flutter through the text input client.
| if (_isSelectionOnlyChange(value)) { | ||
| _handleSelectionChanged(value.selection, renderEditable, SelectionChangedCause.keyboard); | ||
| } else { | ||
| _formatAndSetValue(value); |
There was a problem hiding this comment.
This also attempt to sync the remove editing value in the embedding side, I think we should still need to do that if it is a selection change? cc @gspencergoog .
There was a problem hiding this comment.
What is a "remove editing value"? Shouldn't it send a TextEditingValue with an empty text in that case? And that would fail the _isSelectionOnlyChange check.
There was a problem hiding this comment.
@chunhtai can you explain what you mean again? I'm not sure I understand exactly what you're referring to.
There was a problem hiding this comment.
sorry it is a typo, i meant the remote editing value. The formatAndSetValue also called _updateRemoteEditingValueIfNeeded
This will attempt to sync the current editing value to the embedding. I was wondering if selection only change needs to be sync to the embedding as well.
There was a problem hiding this comment.
Yes, it does, because otherwise the next editing operation won't operate on the same selection in both places.
There was a problem hiding this comment.
The TextEditingValue includes the current selection. So a "selection only" change does cause a change to _value or _receivedRemoteTextEditingValue, which then needs to be synced.
There was a problem hiding this comment.
Maybe we're all in agreement here that it needs to be synced? I somehow feel like we're all saying the same thing...
There was a problem hiding this comment.
ah I see, the updateEditingState is sent with the value inside the embedding, so it is already the same. yes, we don't need to sync it in this case.
There was a problem hiding this comment.
Ok, let me try to clarify a bit. I think we are using the word "sync" which has many meanings in this context.
On the framework side, we keep two variables:
_value: The current editing value on the framework side._receivedRemoteTextEditingValue: This is what the framework thinks is the current editing value on the engine side.
Whenever the editing value changes in the engine, it sends an updateEditingValue message to the framework. The framework then sets _receivedRemoteTextEditingValue to the new value received from the engine (this is one meaning of "sync"). Then the framework tries to format the text of the new editing value received from the engine. If the formatting causes changes to the editing value (i.e. _value != _receivedRemoteTextEditingValue) then the framework sends the new (formatted) _value to the engine via _updateRemoteEditingValueIfNeeded (another meaning of "sync").
In the case of this PR, a selection-only change means there'll be no formatting changes, hence _value and _receivedRemoteTextEditingValue are guaranteed to be the same. So there's no need to call _updateRemoteEditingValueIfNeeded to "sync" the editing value back to the engine.
There was a problem hiding this comment.
Thanks for the clarification. Makes sense now. LGTM.
| )); | ||
| // On web, the only way a text field can be updated from the engine is if | ||
| // a keyboard is used. | ||
| expect(selectionCause, SelectionChangedCause.keyboard); |
There was a problem hiding this comment.
should we also verify the handle does not show up?
There was a problem hiding this comment.
how do we verify it? can we add a screenshot test here?
There was a problem hiding this comment.
Something like this should be sufficient
There was a problem hiding this comment.
The thing is, EditableText doesn't show the handles on its own. Instead, it takes a showSelectionHandles property. This test is only making sure that EditableText reports the change correctly. The rest is on SelectableText and material/TextField to take the selection change and pass the correct value for showSelectionHandles (which they already do, and they have tests for that).
There was a problem hiding this comment.
Maybe I'll add a test that includes the entire interaction between EditableText and material/TextField?
There was a problem hiding this comment.
I think you can just use textfield/selectabletext to test directly, after all, we care about they do not show the handle but not how editabletext ineract with textfield to make that happen.
There was a problem hiding this comment.
Thanks for the suggestions! I wrote a test for selectable_text and material/text_field to make sure they don't show handles under these circumstances.
@chunhtai do you think I should remove the editable_text test that I had initially?
mdebbar
force-pushed
the
selection_handles_web
branch
from
1e56797
to
055ac89
Compare
Description
When using keyboard or mouse to change selection in an input field, the handles shouldn't show up. On the web, there's an edge case that showed the handles when expanding the selection using a keyboard. This PR fixes that.
Related Issues
Fixes #64347
Tests
I added the appropriate tests in:
test/widgets/editable_text_test.dart