Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web] Move text editing nodes outside of shadowDOM #39688

Merged
merged 24 commits into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b7f7f22
Add a text editing host node, attach all text editing nodes to it
htoor3 Feb 16, 2023
1aa89a2
whitespace
htoor3 Feb 16, 2023
f055c0b
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Feb 27, 2023
d9f1dd1
Move semantics tree outside of text editing host, change naming
htoor3 Feb 27, 2023
dfc1bab
Refactor
htoor3 Feb 27, 2023
23b5b6c
fix text editing tests
htoor3 Mar 1, 2023
546b92c
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 1, 2023
9586c58
Fix issues on semantic mode, fix tests, move platform views to new re…
htoor3 Mar 3, 2023
f74442e
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 3, 2023
ae00e40
Fix text field tests
htoor3 Mar 3, 2023
3b6f9eb
Remove unused imports
htoor3 Mar 3, 2023
4f75b6b
Fix host node tests
htoor3 Mar 3, 2023
b600dc6
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 3, 2023
df91ed5
Refactor, fix safari text field tests
htoor3 Mar 3, 2023
d8f96cd
Refactor, fix indentation, remove comments
htoor3 Mar 7, 2023
cc4ed23
Trailing whitespace
htoor3 Mar 7, 2023
de2e916
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 8, 2023
97555e5
Move conditional to compute talkback events when semantics enabled
htoor3 Mar 20, 2023
3ed55b8
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 20, 2023
36a0822
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 20, 2023
bf7f94e
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 27, 2023
401bb1c
Change function naming and comments for offset computations
htoor3 Mar 28, 2023
3815cea
Whitespace
htoor3 Mar 28, 2023
251fa13
Merge remote-tracking branch 'upstream/main' into pass-autofill-exper…
htoor3 Mar 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions lib/web_ui/lib/src/engine/embedder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ class FlutterViewEmbedder {
HostNode get glassPaneShadow => _glassPaneShadow;
late HostNode _glassPaneShadow;

DomElement get textEditingHostNode => _textEditingHostNode;
late DomElement _textEditingHostNode;

static const String defaultFontStyle = 'normal';
static const String defaultFontWeight = 'normal';
static const double defaultFontSize = 14;
Expand Down Expand Up @@ -168,6 +171,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';
Expand All @@ -189,20 +195,20 @@ class FlutterViewEmbedder {
glassPaneElementHostNode.appendAll(<DomNode>[
accessibilityPlaceholder,
_sceneHostElement!,

// The semantic host goes last because hit-test order-wise it must be
// first. If semantics goes under the scene host, platform views will
// obscure semantic elements.
//
// You may be wondering: wouldn't semantics obscure platform views and
// make then not accessible? At least with some careful planning, that
// should not be the case. The semantics tree makes all of its non-leaf
// 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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 <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.

htoor3 marked this conversation as resolved.
Show resolved Hide resolved
]);

// The semantic host goes last because hit-test order-wise it must be
// first. If semantics goes under the scene host, platform views will
// obscure semantic elements.
//
// You may be wondering: wouldn't semantics obscure platform views and
// make then not accessible? At least with some careful planning, that
// should not be the case. The semantics tree makes all of its non-leaf
// 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.
glassPaneElement.appendChild(semanticsHostElement);

// When debugging semantics, make the scene semi-transparent so that the
// semantics tree is more prominent.
if (configuration.debugShowSemanticsNodes) {
Expand Down Expand Up @@ -393,3 +399,24 @@ FlutterViewEmbedder? _flutterViewEmbedder;
FlutterViewEmbedder ensureFlutterViewEmbedderInitialized() =>
_flutterViewEmbedder ??=
FlutterViewEmbedder(hostElement: configuration.hostElement);

/// Creates a node to host text editing elements and applies a stylesheet
/// to Flutter nodes that exist outside of the shadowDOM.
DomElement createTextEditingHostNode(DomElement root, String defaultFont) {
final DomElement domElement =
domDocument.createElement('flt-text-editing-host');
final DomHTMLStyleElement styleElement = createDomHTMLStyleElement();

styleElement.id = 'flt-text-editing-stylesheet';
root.appendChild(styleElement);
applyGlobalCssRulesToSheet(
styleElement.sheet! as DomCSSStyleSheet,
hasAutofillOverlay: browserHasAutofillOverlay(),
cssSelectorPrefix: FlutterViewEmbedder.glassPaneTagName,
defaultCssFont: defaultFont,
);

root.appendChild(domElement);

return domElement;
}
17 changes: 12 additions & 5 deletions lib/web_ui/lib/src/engine/host_node.dart
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ abstract class HostNode {
/// See:
/// * [Document.querySelectorAll](https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll)
Iterable<DomElement> querySelectorAll(String selectors);

DomElement get renderHost;
}

/// A [HostNode] implementation, backed by a [DomShadowRoot].
Expand All @@ -110,11 +112,10 @@ 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.') {
root.appendChild(renderHost);
_shadow = renderHost.attachShadow(<String, dynamic>{
'mode': 'open',
// This needs to stay false to prevent issues like this:
// - https://github.com/flutter/flutter/issues/85759
Expand All @@ -135,6 +136,9 @@ class ShadowDomHostNode implements HostNode {

late DomShadowRoot _shadow;

@override
final DomElement renderHost = domDocument.createElement('flt-render-host');

@override
DomElement? get activeElement => _shadow.activeElement;

Expand Down Expand Up @@ -191,6 +195,9 @@ class ElementHostNode implements HostNode {

late DomElement _element;

@override
final DomElement renderHost = domDocument.createElement('flt-render-host');

@override
DomElement? get activeElement => _element.ownerDocument?.activeElement;

Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/platform_dispatcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher {
_platformViewMessageHandler ??= PlatformViewMessageHandler(
contentManager: platformViewManager,
contentHandler: (DomElement content) {
flutterViewEmbedder.glassPaneElement.append(content);
flutterViewEmbedder.glassPaneShadow.renderHost.append(content);
},
);
_platformViewMessageHandler!.handlePlatformViewCall(data, callback!);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ class PlatformViewManager {
}

_ensureContentCorrectlySized(content, viewType);
wrapper.append(content);

return wrapper..append(content);
return wrapper;
htoor3 marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ import '../semantics.dart' show EngineSemanticsOwner;
/// It also takes into account semantics being enabled to fix the case where
/// offsetX, offsetY == 0 (TalkBack events).
ui.Offset computeEventOffsetToTarget(DomMouseEvent event, DomElement actualTarget) {
// On top of a platform view
if (event.target != actualTarget) {
return _computeOffsetOnPlatformView(event, actualTarget);
}
// On a TalkBack event
if (EngineSemanticsOwner.instance.semanticsEnabled && event.offsetX == 0 && event.offsetY == 0) {
Copy link
Contributor Author

@htoor3 htoor3 Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return _computeOffsetForTalkbackEvent(event, actualTarget);
}
// On top of a platform view
if (event.target != actualTarget) {
return _computeOffsetOnPlatformView(event, actualTarget);
}
// Return the offsetX/Y in the normal case.
// (This works with 3D translations of the parent element.)
return ui.Offset(event.offsetX, event.offsetY);
Expand Down
5 changes: 2 additions & 3 deletions lib/web_ui/lib/src/engine/semantics/text_field.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import 'package:ui/ui.dart' as ui;

import '../browser_detection.dart';
import '../dom.dart';
import '../embedder.dart';
import '../platform_dispatcher.dart';
import '../safe_browser_api.dart';
import '../text_editing/text_editing.dart';
Expand Down Expand Up @@ -424,14 +423,14 @@ class TextField extends RoleManager {
..height = '${semanticsObject.rect!.height}px';

if (semanticsObject.hasFocus) {
if (flutterViewEmbedder.glassPaneShadow.activeElement !=
if (domDocument.activeElement !=
activeEditableElement) {
semanticsObject.owner.addOneTimePostUpdateCallback(() {
activeEditableElement.focus();
});
}
SemanticsTextEditingStrategy._instance?.activate(this);
} else if (flutterViewEmbedder.glassPaneShadow.activeElement ==
} else if (domDocument.activeElement ==
activeEditableElement) {
if (!isIosSafari) {
SemanticsTextEditingStrategy._instance?.deactivate(this);
Expand Down
3 changes: 2 additions & 1 deletion lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ void _emptyCallback(dynamic _) {}

/// The default [HostNode] that hosts all DOM required for text editing when a11y is not enabled.
@visibleForTesting
HostNode get defaultTextEditingRoot => flutterViewEmbedder.glassPaneShadow;
DomElement get defaultTextEditingRoot =>
flutterViewEmbedder.textEditingHostNode;

/// These style attributes are constant throughout the life time of an input
/// element.
Expand Down
9 changes: 5 additions & 4 deletions lib/web_ui/test/engine/host_node_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,20 @@ void testMain() {

group('ShadowDomHostNode', () {
final HostNode hostNode = ShadowDomHostNode(rootNode, '14px monospace');
final DomElement renderHost = domDocument.querySelector('flt-render-host')!;

test('Initializes and attaches a shadow root', () {
expect(domInstanceOfString(hostNode.node, 'ShadowRoot'), isTrue);
expect((hostNode.node as DomShadowRoot).host, rootNode);
expect(hostNode.node, rootNode.shadowRoot);
expect((hostNode.node as DomShadowRoot).host, renderHost);
expect(hostNode.node, renderHost.shadowRoot);

// The shadow root should be initialized with correct parameters.
expect(rootNode.shadowRoot!.mode, 'open');
expect(renderHost.shadowRoot!.mode, 'open');
if (browserEngine != BrowserEngine.firefox &&
browserEngine != BrowserEngine.webkit) {
// Older versions of Safari and Firefox don't support this flag yet.
// See: https://caniuse.com/mdn-api_shadowroot_delegatesfocus
expect(rootNode.shadowRoot!.delegatesFocus, isFalse);
expect(renderHost.shadowRoot!.delegatesFocus, isFalse);
}
});

Expand Down
Loading