[web:a11y] fix traversal and hit-test orders by yjbanov · Pull Request #32712 · flutter/engine

yjbanov · 2022-04-15T21:06:41Z

Fix traversal and hit test orders for web.

Problems with the previous approach:

The ARIA semantics tree was rendered behind the scene host. This was done to allow platform views be reachable using assistive technology. Putting semantics on top used to obscure the underlying platform views. However, now we ended up with the opposite problem: platform views obscure semantics. For example, a pointer interceptor that's meant to simply forward pointer events to the framework can frequently stretch across the whole screen, so no pointer events could land on the semantics tree at all. This is not a problem when users use accessibility shortcuts, but it is a problem when users use touch to explore UI.
- Fix: move the semantics tree on top of the scene host. So the platform views are still reachable, make container nodes transparent hit-test wise using CSS pointer-events: none.
Child semantics nodes were always rendered in traversal order. Hit test order was ignored. This became problematic when the framework started customizing hit test order (e.g. Reverse the semantics order of modal barrier and modal scope flutter#59290).
- Fix: use z-index CSS property to implement child hit test order.
We used aria-hidden: true on the <flt-scene-host> element, but because that property applies to the whole subtree and platform views are composited under it, platform views were hidden from accessibility too, even though they still received regular pointer and keyboard events.
- Fix: do not hide <flt-scene-host>, but hide <flt-picture> elements instead. We are only interested in hiding DOM used to render pixels for widgets, so hiding just the pictures is sufficient.

chunhtai · 2022-04-15T21:17:09Z

lib/web_ui/lib/src/engine/embedder.dart

      _accessibilityPlaceholder,
      _sceneHostElement!,
+
+      // The semantic host goes last because hit-test order-wise we want it to


nit: avoid we

here and everywhere, we may refer to different entity based on the reader's perspective.

chunhtai · 2022-04-15T21:51:13Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

+    }
+
+    // At this point we're guaranteed to have had a non-empty previous child list.
+    final List<SemanticsObject> previousChildrenInRenderOrder = _currentChildrenInRenderOrder!;


Is goal for the rest of this function is too detach old children and attach new children? It feels quite complex as is? the intersectionIndicesNew can probably be removed since it is not used

maybe just insert old list to a set to store old list, and loop through the new list and see if it is in the set. If it is not, attach the element. If it is, remove from the set. At the end just detach remaining element it the set.

The most complex part (starting with the "Both non-empty case" comment) tries to deal with the child order. When DOM nodes move unnecessarily ATs lose focus. So what this function does is compute the longest chain of existing nodes that did not move relative to each other. It keeps those nodes stationary, and moves/adds/removes the nodes around them. This covers most scenarios (scrolls in either direction, insertions, deletions, drag'n'drop, and more). I don't think this happens in native accessibility APIs because they can track moving nodes so long as their IDs are stable.

I am going to leave a comment explaining this. Would that help?

Good catch about intersectionIndicesNew! It's unnecessary. Removed.

ah so the attach and detach order matter. SGTM then

chunhtai

I nitpicked some doc string styling, since they are not introduced in this pr, I will leave decision to you whether you want to fix them. Overall this looks good. just one concern about missing test for glassPaneElementHostNode element order

chunhtai · 2022-04-15T22:57:01Z

lib/web_ui/lib/src/engine/embedder.dart

@@ -99,7 +99,7 @@ class FlutterViewEmbedder {
  html.Element? _sceneElement;

  /// This is state persistent across hot restarts that indicates what


nit: doc string should start with a brief sentence, or convert this to //

Doc comments under engine are not public (the dart:_engine library is private). We only use /// to get dartdoc linking (// will lose the links). We do not aim for public dartdoc level of quality. But thanks for setting a high standard!

chunhtai · 2022-04-15T22:57:34Z

lib/web_ui/lib/src/engine/embedder.dart

  }

-  /// We don't want to unnecessarily move DOM nodes around. If a DOM node is
+  /// Don't unnecessarily move DOM nodes around. If a DOM node is


nit: same as above

chunhtai · 2022-04-15T22:58:31Z

lib/web_ui/lib/src/engine/embedder.dart

@@ -361,7 +369,7 @@ class FlutterViewEmbedder {
  }

  /// The framework specifies semantics in physical pixels, but CSS uses


nit: same as above

chunhtai · 2022-04-15T23:00:08Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

    // DOM nodes created for semantics objects are positioned absolutely using
    // transforms.
    element.style.position = 'absolute';
+    element.setAttribute('id', 'flt-semantic-node-$id');


Probably not a big deal: Is this test only? if so, it can be put into assert

Yeah, accessibility node count tends to be small enough for this extra info to not matter in terms of performance. We also aggressively reuse accessibility nodes, so this constructor is only called once for the entire lifetime of a node with an ID. It is useful for debugging production apps, so I'd like to keep it. If this becomes a performance problem, it's easy enough to remove. It's not used for anything.

chunhtai · 2022-04-15T23:04:12Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

+    }
+
+    // At this point we're guaranteed to have had a non-empty previous child list.
+    final List<SemanticsObject> previousChildrenInRenderOrder = _currentChildrenInRenderOrder!;


ah so the attach and detach order matter. SGTM then

chunhtai · 2022-04-15T23:07:45Z

lib/web_ui/test/engine/semantics/semantics_test.dart

      childrenInHitTestOrder: Int32List.fromList(<int>[1]),
      childrenInTraversalOrder: Int32List.fromList(<int>[1]),
    );
+    updateNode(


Maybe add a comment about why this additional update is needed

It was just a bug in the previous code. It pretended to have a child, but didn't send child info. I added more asserts in the SemanticsOwner, which uncovered the bug. I don't think a comment is necessary because this is now enforced everywhere (I'd have to write many comments like that, everywhere there are child nodes), and the assertion failure should make it obvious what's wrong if someone mistakenly removes it.

chunhtai · 2022-04-15T23:10:24Z

lib/web_ui/lib/src/engine/embedder.dart

+      // 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,


Is there a test coverage for this?

In the past I could find a way to write a test that would emulate browser's hit test. But I just found this: https://developer.mozilla.org/en-US/docs/web/api/document/elementsfrompoint. I'm going to try.

elementsFromPoint worked! I added a test. Also added CanvasKit mode to that test (since platform views are composited differently in CanvasKit), and while doing that discovered that offset was ignored in addPlatformView, so fixed that along the way.

ditman

This change is only ~20% scary, but the code makes sense, and the tests are logical. LGTM!

ditman · 2022-04-16T00:26:41Z

lib/web_ui/lib/src/engine/embedder.dart

+      semanticsHostElement,
    ]);

    // When debugging semantics, make the scene semi-transparent so that the


Now that the accessibility tree renders on top of the scene host, this is probably not required anymore :P

Good point. Strictly speaking, no. But reducing the transparency of the scene makes the semantics overlay more visible, making it easier to work with. I'll keep the transparency, but I will update the comment.

chunhtai

LGTM

…32712)

* [web:a11y] implement traversal and hit-test orders * remove unused intersectionIndicesNew * canvaskit: fix platform view offset and scene host initialization * remove "we" in a bunch of comments

…es (#32867) * [web] do not allocate canvases just for text (#30804) * [web:a11y] fix traversal and hit-test orders (#32712) * [web:a11y] implement traversal and hit-test orders * remove unused intersectionIndicesNew * canvaskit: fix platform view offset and scene host initialization * remove "we" in a bunch of comments Co-authored-by: Yegor <yjbanov@google.com>

[web:a11y] implement traversal and hit-test orders

3b7f799

flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 15, 2022

yjbanov added the accessibility label Apr 15, 2022

chunhtai reviewed Apr 15, 2022

View reviewed changes

yjbanov added 2 commits April 15, 2022 15:45

remove "we" in a bunch of places

a63bdd7

remove unused intersectionIndicesNew

2052773

chunhtai reviewed Apr 15, 2022

View reviewed changes

ditman approved these changes Apr 16, 2022

View reviewed changes

add hit test test; fix platform view offset and scene host init

52076d3

yjbanov requested a review from harryterkelsen April 18, 2022 18:03

harryterkelsen approved these changes Apr 18, 2022

View reviewed changes

chunhtai approved these changes Apr 18, 2022

View reviewed changes

yjbanov force-pushed the dialog-a11y branch 2 times, most recently from 56b84e9 to 23ba1f8 Compare April 18, 2022 22:30

browser-specific fixes

0ff84fc

yjbanov force-pushed the dialog-a11y branch from 23ba1f8 to 0ff84fc Compare April 18, 2022 23:08

yjbanov merged commit bd93f6c into flutter:main Apr 18, 2022

engine-flutter-autoroll mentioned this pull request Apr 19, 2022

Roll Engine from 7a7910583821 to ff0d539fd168 (21 revisions) flutter/flutter#102146

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 19, 2022

bd93f6c [web:a11y] fix traversal and hit-test orders (flutter/engine#…

edb1f14

…32712)

CaseyHillers mentioned this pull request Apr 22, 2022

[flutter_releases] Cherrypick web engine accessibility and canvas fixes #32867

Merged

yjbanov mentioned this pull request Apr 29, 2022

[pointer_interceptor] fix integration test flutter/packages#1675

Merged

yjbanov mentioned this pull request Sep 7, 2023

[web] Semantic tree traversal order flutter/flutter#45206

Closed

		@@ -99,7 +99,7 @@ class FlutterViewEmbedder {
		html.Element? _sceneElement;

		/// This is state persistent across hot restarts that indicates what

		@@ -361,7 +369,7 @@ class FlutterViewEmbedder {
		}

		/// The framework specifies semantics in physical pixels, but CSS uses

Conversation

yjbanov commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yjbanov commented Apr 15, 2022 •

edited

Loading