-
Notifications
You must be signed in to change notification settings - Fork 6k
[web] Move text editing nodes outside of shadowDOM #39688
Changes from 2 commits
b7f7f22
1aa89a2
f055c0b
d9f1dd1
dfc1bab
23b5b6c
546b92c
9586c58
f74442e
ae00e40
3b6f9eb
4f75b6b
b600dc6
df91ed5
d8f96cd
cc4ed23
de2e916
97555e5
3ed55b8
36a0822
bf7f94e
401bb1c
3815cea
251fa13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,6 +124,9 @@ class FlutterViewEmbedder { | |
| HostNode? get glassPaneShadow => _glassPaneShadow; | ||
| HostNode? _glassPaneShadow; | ||
|
|
||
| DomElement? get textEditingHostNode => _textEditingHostNode; | ||
| DomElement? _textEditingHostNode; | ||
|
|
||
| static const String defaultFontStyle = 'normal'; | ||
| static const String defaultFontWeight = 'normal'; | ||
| static const double defaultFontSize = 14; | ||
|
|
@@ -170,6 +173,9 @@ class FlutterViewEmbedder { | |
| ); | ||
| _glassPaneShadow = glassPaneElementHostNode; | ||
|
|
||
| _textEditingHostNode = | ||
| createTextEditingHostNode(glassPaneElement, defaultCssFont); | ||
|
|
||
| // Don't allow the scene to receive pointer events. | ||
| _sceneHostElement = domDocument.createElement('flt-scene-host') | ||
| ..style.pointerEvents = 'none'; | ||
|
|
@@ -189,7 +195,6 @@ class FlutterViewEmbedder { | |
| .prepareAccessibilityPlaceholder(); | ||
|
|
||
| glassPaneElementHostNode.appendAll(<DomNode>[ | ||
| accessibilityPlaceholder, | ||
| _sceneHostElement!, | ||
|
|
||
| // The semantic host goes last because hit-test order-wise it must be | ||
|
|
@@ -202,9 +207,10 @@ class FlutterViewEmbedder { | |
| // elements transparent. This way, if a platform view appears among other | ||
| // interactive Flutter widgets, as long as those widgets do not intersect | ||
| // with the platform view, the platform view will be reachable. | ||
| semanticsHostElement, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yjbanov would love to get your thoughts on if we can safely move the semantics tree outside of the shadowDOM and into its own node?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the current problematic implementation of semantics, this shouldn't break anything I know of. However, one issue with the current semantics tree is that it does not contain platform views. This creates some issues with traversal order. I was thinking in the future to slice the semantics tree and interleave it with the render tree such that platform views can be embedded into the semantics tree while preserving the paint order. To do that, the semantics tree would have to be inside the shadow root, otherwise the Having said that, I'm willing to cross that bridge when we get there. What's the motivation for moving it out?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Primary motivation to move it out is to fix the password autofill issue for when semantics is enabled, since
htoor3 marked this conversation as resolved.
|
||
| ]); | ||
|
|
||
| _textEditingHostNode?.appendChild(accessibilityPlaceholder); | ||
| _textEditingHostNode?.appendChild(semanticsHostElement); | ||
|
htoor3 marked this conversation as resolved.
Outdated
|
||
| // When debugging semantics, make the scene semi-transparent so that the | ||
| // semantics tree is more prominent. | ||
| if (configuration.debugShowSemanticsNodes) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,11 +110,12 @@ class ShadowDomHostNode implements HostNode { | |
| /// This also calls [applyGlobalCssRulesToSheet], with the [defaultFont] | ||
| /// to be used as the default font definition. | ||
| ShadowDomHostNode(DomElement root, String defaultFont) | ||
| : assert( | ||
| root.isConnected ?? true, | ||
| 'The `root` of a ShadowDomHostNode must be connected to the Document object or a ShadowRoot.' | ||
| ) { | ||
| _shadow = root.attachShadow(<String, dynamic>{ | ||
| : assert(root.isConnected ?? true, | ||
| 'The `root` of a ShadowDomHostNode must be connected to the Document object or a ShadowRoot.') { | ||
| final DomElement element = | ||
| domDocument.createElement('flt-shadow-host-node'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this extra element? What do you think of the following: EDIT: After discussing with @htoor3 it looks like the extra element above the shadow root avoids some issues with event listeners. Please add this explanation in a comment for future reference.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's correct, having a direct sibling to the This is likely due to the entire subtree within Two ways around that:
Edit: Option 2 actually would work and keep our tree slightly flatter if we deem that important. Quick jsfiddle shows that the items rendered via the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, IIRC in the presence of a shadow root direct children of the node are treated quite differently from normal elements. There's an expectation that there will be a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Would
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that makes sense. I pushed up the naming change and moved the semantics tree out of the text-editing host |
||
| root.appendChild(element); | ||
| _shadow = element.attachShadow(<String, dynamic>{ | ||
| 'mode': 'open', | ||
| // This needs to stay false to prevent issues like this: | ||
| // - https://github.com/flutter/flutter/issues/85759 | ||
|
|
@@ -221,6 +222,25 @@ class ElementHostNode implements HostNode { | |
| void appendAll(Iterable<DomNode> nodes) => nodes.forEach(append); | ||
| } | ||
|
|
||
| DomElement createTextEditingHostNode(DomElement root, String defaultFont) { | ||
| const String hostTagName = 'flt-text-editing-host-node'; | ||
| final DomElement domElement = domDocument.createElement(hostTagName); | ||
| final DomHTMLStyleElement styleElement = createDomHTMLStyleElement(); | ||
|
|
||
| styleElement.id = 'flt-text-editing-stylesheet'; | ||
| root.appendChild(styleElement); | ||
| applyGlobalCssRulesToSheet( | ||
| styleElement.sheet! as DomCSSStyleSheet, | ||
| hasAutofillOverlay: browserHasAutofillOverlay(), | ||
| cssSelectorPrefix: hostTagName, | ||
| defaultCssFont: defaultFont, | ||
| ); | ||
|
|
||
| root.appendChild(domElement); | ||
|
|
||
| return domElement; | ||
| } | ||
|
|
||
| // Applies the required global CSS to an incoming [DomCSSStyleSheet] `sheet`. | ||
| void applyGlobalCssRulesToSheet( | ||
| DomCSSStyleSheet sheet, { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ditman I had a discussion with @htoor3 about this and he mentioned that you were planning to remove
host_node.dart. I have a different opinion that I would like to share here and get your input on:The benefit I see in
HostNodeis to encapsulate the high-level DOM structure of the app (including the shadow root). This way, the rest of the engine doesn't have to worry about any of the following:That said, you are more familiar with this area, maybe you have a different vision for this, especially with your work on element embedding? E.g. should
HostNodeand embedding strategy be merged together? Or maybe all of theHostNodeencapsulation should be moved toFlutterViewEmbedder?One thing that may help here is to determine which classes are supposed to be global/singletons and which classes are supposed to be instantiated for each app or view. To clarify: in the future, we are going to support rendering multiple flutter apps on the same page (into different host elements). In that case, is each app going to have its own
FlutterViewEmbedder? Or is that supposed to be global across all apps?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning on removing the
HostNodebecause the original version was only to decide if we were going to render in an Element or the ShadowDOM of an element.We can repurpose the name for any other thing we like.
My idea was to have a FlutterApplicationElement (call it HostNode) that knows how to create all the different elements for a Flutter Web app view (the glass-pane, the different element hosts, etc...), and that can be used later in place of the global "textEditingNode", "glassPaneElement", "semanticsHelper" and family (because it's likely that the global ViewEmbedder needs to go away in a multi-view world)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm glad we are in agreement on the core idea.
I actually don't like the name
HostNodebecause it could easily be confused with the host element for embedding. So a new name is in order.I also looked at flutter/flutter#116204 which indicates that there may be a need to keep the non-shadow implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked to Yegor about it, and there's no need to support the non-shadow implementation moving forward.