[web] dialog a11y fixes #41681

yjbanov · 2023-05-03T01:10:49Z

Fixes two issues in dialog accessibility:

Fixes [web] New routes are not announced in a11y mode flutter#45207 by setting role="dialog" on nodes that have both scopesRoute and namesRoute set. There's no guarantee that this combination of flags is an actual dialog, but it's close enough, and it makes the screen reader announce the appearance of the dialog. Note that scopesRoute alone is not sufficient, because Flutter uses it for overlays that are not semantically dialogs, such as dismiss barriers.
Fixes an issue with focus management, where focus fails to transfer to background content after the dialog is dismissed. This happened because EngineSemanticsOwner._semanticsTree retained descendants of parents that were removed. This is benign in many cases. However, for focus this is problematic because the HTML element can go away and come back (losing focus along the way), but its corresponding SemanticsObject is never marked as "dirty" and fails to update and request focus.

I'm hoping this is sufficient to fix b/251839784 as well. Will work with the relevant team to find out.

jonahwilliams · 2023-05-03T01:19:35Z

Technically the ways these two flags are supposed to work does not require them to be on the same node. All pages, which I suppose probably includes modal dialogs, introduce a scopes route flag. Then walking the tree is depth first order until namesRoute flag determines the announced name

flutter-dashboard · 2023-05-03T21:01:56Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #41681 at sha f4a34c9

yjbanov · 2023-05-03T21:16:23Z

@jonahwilliams That's interesting. It sounds like a scopesRoute widget can be a role="dialog" that points to the namesRoute using aria-labelledby (example). What do you think?

I can add it in a follow-up PR as the case of the two being on the same node vs separate nodes are mutually exclusive and use different ARIA properties.

jonahwilliams · 2023-05-03T21:22:34Z

That sounds like it would work

ditman

If CI is happy, I can't find anything egregious either! LGTM!

ditman · 2023-05-04T22:15:08Z

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

+      if (!_semanticsTree.keys.every(liveIds.contains)) {
+        throw AssertionError(
+          'The semantics node map is inconsistent:\n'
+          '  Nodes in tree: [${liveIds.join(', ')}]\n'
+          '  Nodes in map : [${_semanticsTree.keys.join(', ')}]'
+        );
+      }


Why not use assert(_semanticsTree.keys.every(liveIds.contains), 'The semantics node map...') here as well? (see L1898?)

Because I was under the false impression that assertion error expressions are evaluated eagerly, and this one's really expensive. However, I was proven wrong.

Done.

ditman · 2023-05-04T22:46:46Z

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

+  @override
+  void update() {
+    assert(() {
+      final String? label = semanticsObject.label;


Move this outside of the assert and make it:

final String label = semanticsObject.label ?? '';

Then in the assert you can check only .trim().isEmpty and use label directly in line 35?

ditman · 2023-05-04T22:51:52Z

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

@@ -1241,7 +1264,11 @@ class SemanticsObject {
  /// Detects the roles that this semantics object corresponds to and manages
  /// the lifecycles of [SemanticsObjectRole] objects.
  void _updateRoles() {


Not to refactor this right now but... shouldn't this be the responsibility of each specialized class of RoleManager?

Why do we need isDialog in a SemanticsObject, and an _updateRoles that knows about all possible roles, when we eventually wrap this in a Dialog RoleManager that knows about how dialogs should behave?

It can't be the responsibility of each role manager, because each node may have multiple roles and which ones are assigned depends on adjacent flag values. It's easier to have this logic all in one place. Right now this method is <30 LoC, which is quite manageable.

However, you made me think! I wonder if we could assign a "primary" role, which would be 1:1 with each object, and then the primary role manager can add extra "secondary" roles. One thing that doesn't work well with the current design is it's unclear who is responsible for setting the role attribute. Any role manager can set it, which can conflict with another role manager. But in a primary/secondary model only the primary role manager would be responsible for it.

Filed flutter/flutter#126384

ditman · 2023-05-04T22:57:20Z

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

    assert(_semanticsTree.containsKey(id));
    final SemanticsObject? object = _semanticsTree[id];
-    _detachments.add(object);


I'd flip the assert and the access to the map to do:

final SemanticsObject? object = _semanticsTree[id]; assert(object != null, 'Provide a better error message if you prefer here... or Semantics Tree does not contain key $id');

Didn't add a message since this is an internal assertion. Developers shouldn't ever see it.

ditman · 2023-05-04T22:59:26Z

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

+        final SemanticsObject? parent = _attachments[node.id];
+        if (parent == null) {
+          // Was not reparented and is removed permanently from the tree.
+          removals.add(node);


Once you add a node to removals, do you really need to keep iterating all its children for removal as well?

Yep! That's one of the main issues that I'm fixing. When a parent is removed, we didn't remove children from the map. So next time children were readded (after the the dialog is dismissed), we started with old children containing wrong state information. We have to remove them from the map so next time we create fresh new objects for them.

ditman · 2023-05-04T23:00:51Z

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

+  ///
+  /// Calls the [callback] for `this` node, then for all of its descendants.
+  void visitDepthFirst(void Function(SemanticsObject) callback) {
+    callback(this);


Should callback return a bool so you abort the the drilling into children if callback(this) returns false?

See #41681 (comment)

mdebbar · 2023-05-04T16:21:01Z

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

+    if (object != null) {
+      _detachments.add(object);
+    }


Since we are asserting that the tree contains this id, then we know that object will always be non-null:

Suggested change

if (object != null) {

_detachments.add(object);

}

_detachments.add(object!);

I'm playing it safe. My worry is that we can get something inconsistent from the framework. Because child nodes are supplied as Uint8List of child IDs, and their nodes separately in their own SemanticsUpdate objects, the consistency is merely a hand-shake. The API does not enforce it. An assertion should be effective at keeping most bugs away, but we don't want to crash in production if one sneaked in.

mdebbar · 2023-05-04T23:08:32Z

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

+      // elsewhere. Walk the descendant tree and find all descendants that were
+      // reattached to a parent. Those descendants need to be removed.
+      final List<SemanticsObject> removals = <SemanticsObject>[];
+      detachmentRoot.visitDepthFirst((SemanticsObject node) {


This is traversing the entire tree including all descendants of detached nodes. Do you think it's worth making a short circuit to avoid traversing children of detached nodes?

It can be done by returning a boolean from the callback to indicate whether you want to continue down that subtree or not.

See #41681 (comment)

htoor3 · 2023-05-05T00:12:41Z

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

+      }
+      return true;
+    }());
+    semanticsObject.element.setAttribute('aria-label', semanticsObject.label ?? '');


Do you think we should we have a more descriptive fallback aria-label other than an empty string if no label is provided? Or do we generally defer to the dev via warnings like on line 26?

We can't because labels are part of the UI, which needs to be internationalized. Error messages and warnings in Flutter, OTOH, are all in English. So I think a warning and missing label is probably a better fallback.

htoor3 · 2023-05-05T00:16:11Z

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

+        'Semantic node 0 was assigned dialog role, but is missing a label. A dialog should contain a label so that a screen reader can communicate to the user that a dialog appeared and a user action is requested.',
+      ],
+    );
+


nit: we can also assert that the dialog aria-role was applied here despite the warning

Good catch! Done.

htoor3

lgtm

…sions) (#126455) Manual roll requested by [email protected] Cannot build log URL because revision "892f88d2513f" is invalid: Luci builds of "Linux Fuchsia FEMU" for 892f88d2513f4a60ebd6d1496c4ac272e53c55ba was STARTED 2023-05-10 [email protected] Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 [email protected] Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 [email protected] [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 [email protected] Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 [email protected] Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 [email protected] Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 [email protected] [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 [email protected] Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 [email protected] [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 [email protected] [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 [email protected] Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…126468) flutter/engine@10ac36c...406b354 2023-05-10 [email protected] [web] Re-enable history tests on Safari (flutter/engine#41901) 2023-05-10 [email protected] switch from MockCanvas to DisplayListBuilder in layer unit tests (flutter/engine#41889) 2023-05-10 [email protected] Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 [email protected] Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 [email protected] [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 [email protected] Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 [email protected] Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 [email protected] Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 [email protected] [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 [email protected] Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 [email protected] [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 [email protected] [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 [email protected] Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…sions) (flutter#126455) Manual roll requested by [email protected] Cannot build log URL because revision "892f88d2513f" is invalid: Luci builds of "Linux Fuchsia FEMU" for 892f88d2513f4a60ebd6d1496c4ac272e53c55ba was STARTED 2023-05-10 [email protected] Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 [email protected] Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 [email protected] [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 [email protected] Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 [email protected] Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 [email protected] Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 [email protected] [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 [email protected] Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 [email protected] [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 [email protected] [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 [email protected] Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#126468) flutter/engine@10ac36c...406b354 2023-05-10 [email protected] [web] Re-enable history tests on Safari (flutter/engine#41901) 2023-05-10 [email protected] switch from MockCanvas to DisplayListBuilder in layer unit tests (flutter/engine#41889) 2023-05-10 [email protected] Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 [email protected] Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 [email protected] [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 [email protected] Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 [email protected] Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 [email protected] Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 [email protected] [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 [email protected] Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 [email protected] [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 [email protected] [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 [email protected] Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard bot added the platform-web Code specifically for the web engine label May 3, 2023

yjbanov force-pushed the dialog-a11y branch from 23c84f9 to f4a34c9 Compare May 3, 2023 18:08

yjbanov marked this pull request as ready for review May 3, 2023 20:59

flutter-dashboard bot added the will affect goldens label May 3, 2023

yjbanov mentioned this pull request May 4, 2023

[web:a11y] implement scopesRoute and namesRoute applied to separate nodes flutter/flutter#126030

Closed

2 tasks

ditman approved these changes May 4, 2023

View reviewed changes

mdebbar approved these changes May 4, 2023

View reviewed changes

htoor3 reviewed May 5, 2023

View reviewed changes

htoor3 approved these changes May 5, 2023

View reviewed changes

kevmoo added the accessibility label May 5, 2023

yjbanov force-pushed the dialog-a11y branch from f4a34c9 to 1638808 Compare May 10, 2023 01:30

yjbanov added 5 commits May 9, 2023 20:53

[web] dialog a11y fixes

77f1da5

dartdocs for Dialog

f691fec

licenses_flutter

b7ce35a

add test

db1f4ca

address comments

88bfcfa

yjbanov force-pushed the dialog-a11y branch from 1638808 to 88bfcfa Compare May 10, 2023 03:54

yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label May 10, 2023

auto-submit bot merged commit 9504b30 into flutter:main May 10, 2023

engine-flutter-autoroll mentioned this pull request May 10, 2023

Manual roll Flutter Engine from 10ac36c52f08 to 892f88d2513f (11 revisions) flutter/flutter#126455

Merged

zanderso pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023

9504b3007 [web] dialog a11y fixes (flutter/engine#41681)

f3e67f2

engine-flutter-autoroll mentioned this pull request May 10, 2023

Roll Flutter Engine from 10ac36c52f08 to 406b3546a2ac (13 revisions) flutter/flutter#126468

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023

9504b3007 [web] dialog a11y fixes (flutter/engine#41681)

21d6fba

[web] dialog a11y fixes #41681

[web] dialog a11y fixes #41681

Uh oh!

Conversation

yjbanov commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented May 3, 2023

Uh oh!

flutter-dashboard bot commented May 3, 2023

Uh oh!

yjbanov commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented May 3, 2023

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!

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!

yjbanov May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

htoor3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yjbanov commented May 3, 2023 •

edited

Loading

yjbanov commented May 3, 2023 •

edited

Loading

yjbanov May 10, 2023 •

edited

Loading