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 14 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
30 changes: 28 additions & 2 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,7 +195,6 @@ 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.
Expand All @@ -200,9 +205,9 @@ 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,
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
]);

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 +398,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 @@ -588,7 +588,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
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