[web] Move text editing nodes outside of shadowDOM

[htoor3]

Password input autofill + suggest password functionality is broken for password inputs inside of a shadowDOM on chrome. This experiment is testing to see if we can lift text editing nodes outside of the shadowDOM while maintaining functionality.

1. Create a new host node outside of the shadowDOM for text-editing + accessibility
2. Attach all text editing nodes, accessibility placeholders, and semantics tree into that node
3. Ensure text editing + accessibility global styles are applied to new host node

Tree goes from:

                flt-glass-pane
                      |
                 #shadow-root
                      |
                 text nodes

to:

                flt-glass-pane
           -------------------------
           /            |            \
  flt-render-host text-editing-host  semantics-host
          |             |                  |
     #shadow-root   text nodes       semantics nodes

Some additional changes:

• Update semantic + host node tests to account for new tree
• Append platform views to flt-render-host (previously being appended to flt-glass-pane)

Simple form w/code changes

Fixes flutter/flutter#87735

[mdebbar]

Do we need this extra element? What do you think of the following:

                flt-glass-pane
           -------------------------
           /          |            \
#shadow-root    text-editing-host  semantics-host
                      |                  |
                  text nodes       semantics nodes

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.

[htoor3]

Yeah that's correct, having a direct sibling to the #shadow-root caused some rendering issues. I noticed the input listeners on the active element weren't being attached, and the inputs weren't functioning correctly.

This is likely due to the entire subtree within flt-glass-pane getting replaced whenever attachShadow gets called on it - which is standard shadowDOM behavior.

Two ways around that:

1. Contain the #shadow-root within its own node, which is what I did above. When attachShadow gets called on that node, only that node's children will get replaced, which allows the text-editing nodes to render correctly since they exist outside of the #shadow-root's parent node.
2. We can use <slot>s within our #shadow-root to render any of its siblings. However, it's unclear whether text inputs rendered via <slot>s will face the same Chrome password autofill issues, since the <slot>s are technically within the shadowDOM.

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 <slot> aren't affected by the Chrome autofill bug.

[yjbanov]

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 <slot> inside the shadow root to project direct children into, because the elements are laid out based on constraints provided to the slot. Without one I'm not even sure how the element would be rendered. So moving text-editing-host into a separate branch is probably safer.

[yjbanov]

nit: Would <flt-render-host> be a better name? It seems we're moving all the non-rendering things out of it, and it's used purely for positioning, laying out, and painting, which are all rendering aspects. Also, the -node suffix doesn't add useful information.

[htoor3]

I think that makes sense. I pushed up the naming change and moved the semantics tree out of the text-editing host

[mdebbar]

@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 HostNode is 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:

1. Does the glass pane have a shadow root attached?
2. Is the semantics tree rendered behind or in front of the scene host?
3. Are text fields inside or outside the shadow root?

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 HostNode and embedding strategy be merged together? Or maybe all of the HostNode encapsulation should be moved to FlutterViewEmbedder?

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?

[ditman]

I was planning on removing the HostNode because 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)

[mdebbar]

Ok, I'm glad we are in agreement on the core idea.

I actually don't like the name HostNode because 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?

[ditman]

I also looked at flutter/flutter#116204 which indicates that there may be a need to keep the non-shadow implementation?

Talked to Yegor about it, and there's no need to support the non-shadow implementation moving forward.

[htoor3]

@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?

[yjbanov]

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 <slot> elements won't work.

Having said that, I'm willing to cross that bridge when we get there. What's the motivation for moving it out?

[htoor3]

Primary motivation to move it out is to fix the password autofill issue for when semantics is enabled, since <input>s will be rendered in the semantics tree in that case and they'll face the same shadowDOM issues. AFAIK, the 2 nodes that host text editing elements would be whatever host we specify in text_editing.dart and the semantics tree.

[ditman]

How hard would it be to create a minimal repro case of this, and send it to Chrome for them to fix (if they don't have it already)? This looks like a big change for a browser workaround :/

[mdebbar]

How hard would it be to create a minimal repro case of this, and send it to Chrome for them to fix (if they don't have it already)?

@htoor3 already added some details and a jsfiddle repro to the existing chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1378402#c8

This looks like a big change for a browser workaround :/

Unfortunately, we don't know when Chrome will fix this bug, and this is a very popular issue in flutter web that has been open for a while. I would say let's bite the bullet and do the workaround.

[ditman]

Unfortunately, we don't know when Chrome will fix this bug, and this is a very popular issue in flutter web that has been open for a while. I would say let's bite the bullet and do the workaround.

Note that since Safari Tech Preview 162, this seems like a Chrome-only bug.

The only drawback I see to this fix is that with the custom element embedding, things outside of the shadow DOM can be very easily affected by the surrounding page's CSS. Luckily not many people are using that embedding mode yet, so I guess it's not as problematic as password autofills (yet).

[htoor3]

Unfortunately, we don't know when Chrome will fix this bug, and this is a very popular issue in flutter web that has been open for a while. I would say let's bite the bullet and do the workaround.

Note that since Safari Tech Preview 162, this seems like a Chrome-only bug.

The only drawback I see to this fix is that with the custom element embedding, things outside of the shadow DOM can be very easily affected by the surrounding page's CSS. Luckily not many people are using that embedding mode yet, so I guess it's not as problematic as password autofills (yet).

Are the issues mainly due to styling collisions? I wonder if generating unique classnames for nodes outside of the shadowDOM or using the flt-glass-pane prefix along with our own CSS reset can protect against that?

[ditman]

Are the issues mainly due to styling collisions?

The ones I can think off the top of my head: yes.

I wonder if generating unique classnames for nodes outside of the shadowDOM or using the flt-glass-pane prefix along with our own CSS reset can protect against that?

Specificity is good, maybe with a bunch of !important and/or inline styles we're probably going to be ok.

[ditman]

test/text_editing_test.dart:94:37:
Error: The getter 'activeElement' isn't defined for the class 'DomElement'.
 - 'DomElement' is from 'package:ui/src/engine/dom.dart' ('lib/src/engine/dom.dart').
      expect(defaultTextEditingRoot.activeElement, null);
                                    ^^^^^^^^^^^^^

activeElement comes from the Document that contains the defaultTextEditingRoot. It should be easily accessible via defaultTextEditingRoot.ownerDocument, I think?

[mdebbar]

Overall looks good to me with some minor comments.

[htoor3]

@ditman is the conditional order ok to change here? Running into a TalkBack issue where the offsets aren't being calculated correctly for pointer events. The problem is arising because any event that crosses into the shadowDOM has a target of whatever the shadowDOM's host is (glass pane). With the semantic tree moved outside of the shadowDOM, the target is now the actual element being clicked, which was causing us to branch into the platform view offset conditional, since target and actualTarget won't both be the glass pane. If we move the TalkBack offset computation conditional first, it fixes the issue.

cc @yjbanov this change fixes the TalkBack issue you discovered in the Gallery app.

[ditman]

I think this change makes sense. I'm not sure I have any special criteria when I added the ordering in the first place, but in this case it seems we're going from "most special corner case" to "least special case", so I think this change makes a lot of sense (also, glad that the change is simple and not something involving changing the measurements/algos :P)

[yjbanov]

Spoke with @htoor3 over a VC. Some notes:

• It seems the event.target == actualTarget is never true because actualTarget is <flt-glass-pane>, but the shadow root host is the new <flt-render-host>.
• Simply changing to <flt-render-host> may not be sufficient because the semantics tree now behaves like a platform view (it's outside the shadow root) and may require similar logic to _computeOffsetOnPlatformView.

[htoor3]

Update on this:

• To your first point, this logic actually still does work because <flt-render-host> is invisible. So when we hover/click on anything in the app, the event.target is still <flt-glass-pane> and the offset logic works correctly.
• Since we're removing text and semantic nodes outside of the shadowDOM, those offsets now need to be calculated with the same logic as how platform view offsets were being calculated. This was already happening but it was a bit confusing as to why. The event.target != actualTarget expression is actually a good check for this because it doesn't seem like we need to differentiate between nodes that are platform views and other non-shadowDOM nodes - we just need to determine if any given target is outside the shadowDOM. If so, we need to recalculate the offsets.
• To make this clearer, I renamed the boolean to check for this to isTargetOutsideOfShadowDOM and the function to a more generalized name _computeOffsetRelativeToActualTarget since it's computing offsets for more than just platform views now.
• Tested this in various corner cases and the offsets are working correctly for:
  • Text nodes
  • Platform views
  • Semantic tree nodes
  • When the page is scrolled
  • When the app is embedded into a <div>
  • Using screen readers (both VoiceOver and Talkback)

[htoor3]

@eyebrowsoffire this is the change I've made, with the firefox tests passing - however, this is dependent on a few other changes in this PR, mainly defaultTextEditingRoot being changed, so it might not be super easy to drop these changes in.

Oddly enough, I tried running the same tests on the latest version of main branch and they still pass with the --browser=firefox flag

[mdebbar]

The problem might be with the way Firefox reports activeElement when the focus is inside the shadow dom. Now that text editing elements moved out of the shadow dom, the issue is fixed.