[web] Use viewId for text editing #51099
Conversation
|
Still working on auto-fill tests, but I would appreciate a review of the code! |
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
This looks great!
Left a couple of comments with DOM APIs that might simplify some of our code here, but in general this is sweet!
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
Not a big fan of the looping to find the hostElement of the view to which an element belongs, but IMO not a blocker. LGTM!
| if (view == null) { | ||
| // `viewId` points to a non-existent view, is this even possible? Should we | ||
| // throw? | ||
| return; | ||
| } |
There was a problem hiding this comment.
Maybe convert this in one of those assert(viewId != null, 'This should never happen, create a bug!');, rather than bailing out silently?
| while (current != null) { | ||
| final EngineFlutterView? view = _elementToView[current]; |
There was a problem hiding this comment.
Why this while and not closest? Is iterating our own cache faster than a brower querySelector/closest?
There was a problem hiding this comment.
I don't like the idea of depending on tag name and html attributes. But I'm not feeling strongly about it, so I'll switch to closest since it might be faster.
|
Safari seems unhappy with the change, a couple of these: |
| final DomElement input = textEditing!.strategy.domElement!; | ||
|
|
||
| // Input is appended to the right view. | ||
| expect(view.dom.textEditingHost.contains(input), isTrue); |
There was a problem hiding this comment.
(I think this is the line that safari doesn't like?)
One way of making the text more readable is to add a reason: 'input must be contained in textEditingHost' or similar, so the test failure has some meaning (rather than: "expected true but got false")
There was a problem hiding this comment.
Done. This is because on Safari we do some shady stuff haha. I updated the test to properly work on Safari.
In order for text fields to work correctly in multi-view on the web, we need to have the `viewId` information sent to the engine (`TextInput.setClient`). And while the text field is active, if it somehow moves to a new `View`, we need to inform the engine about such change (`TextInput.updateConfig`). Engine PR: flutter/engine#51099 Fixes #137344
In order for text fields to work correctly in multi-view on the web, we need to have the `viewId` information sent to the engine (`TextInput.setClient`). And while the text field is active, if it somehow moves to a new `View`, we need to inform the engine about such change (`TextInput.updateConfig`). Engine PR: flutter/engine#51099 Fixes flutter#137344
mdebbar
added
the
autosubmit
…146001) flutter/engine@7176173...8480145 2024-03-29 [email protected] Add a minimal example of using `package:test`. (flutter/engine#51726) 2024-03-29 [email protected] Implement `.engine-release.version` files for engine Skia Gold tests (flutter/engine#51739) 2024-03-29 [email protected] [web] Use viewId for text editing (flutter/engine#51099) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] 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
2 participants
Handle
viewIdfor text fields.Part of flutter/flutter#137344