From 77f1da59fafb5701df9896db58c45f4c5cece489 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 2 May 2023 18:02:23 -0700 Subject: [PATCH 1/5] [web] dialog a11y fixes --- lib/web_ui/lib/src/engine.dart | 1 + .../lib/src/engine/semantics/dialog.dart | 35 ++++++ .../lib/src/engine/semantics/semantics.dart | 103 ++++++++++++++---- .../test/engine/semantics/semantics_test.dart | 88 ++++++++++++++- 4 files changed, 207 insertions(+), 20 deletions(-) create mode 100644 lib/web_ui/lib/src/engine/semantics/dialog.dart diff --git a/lib/web_ui/lib/src/engine.dart b/lib/web_ui/lib/src/engine.dart index f49c42cce499d..c523692e7b152 100644 --- a/lib/web_ui/lib/src/engine.dart +++ b/lib/web_ui/lib/src/engine.dart @@ -133,6 +133,7 @@ export 'engine/rrect_renderer.dart'; export 'engine/safe_browser_api.dart'; export 'engine/semantics/accessibility.dart'; export 'engine/semantics/checkable.dart'; +export 'engine/semantics/dialog.dart'; export 'engine/semantics/image.dart'; export 'engine/semantics/incrementable.dart'; export 'engine/semantics/label_and_value.dart'; diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart new file mode 100644 index 0000000000000..9b13dbbe1f140 --- /dev/null +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -0,0 +1,35 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import '../dom.dart'; +import '../semantics.dart'; +import '../util.dart'; + +class Dialog extends RoleManager { + Dialog(SemanticsObject semanticsObject) : super(Role.dialog, semanticsObject); + + @override + void dispose() { + semanticsObject.element.removeAttribute('aria-label'); + semanticsObject.clearAriaRole(); + } + + @override + void update() { + assert(() { + final String? label = semanticsObject.label; + if (label == null || label.trim().isEmpty) { + printWarning( + 'Semantic node ${semanticsObject.id} 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.' + ); + } + return true; + }()); + semanticsObject.element.setAttribute('aria-label', semanticsObject.label ?? ''); + semanticsObject.setAriaRole('dialog', true); + } +} diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 62f89313f30ca..a8a274de9c96e 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -17,6 +17,7 @@ import '../platform_dispatcher.dart'; import '../util.dart'; import '../vector_math.dart'; import 'checkable.dart'; +import 'dialog.dart'; import 'image.dart'; import 'incrementable.dart'; import 'label_and_value.dart'; @@ -356,6 +357,18 @@ enum Role { /// with this role, they will be able to get the assistive technology's /// attention right away. liveRegion, + + /// Adds the "dialog" ARIA role to the node. + /// + /// This corresponds to a semantics node that has both `scopesRoute` and + /// `namesRoute` bits set. While in Flutter a named route is not necessarily a + /// dialog, this is the closest analog on the web. + /// + /// Why is `scopesRoute` alone not sufficient? Because Flutter can create + /// routes that are not logically dialogs and there's nothing interesting to + /// announce to the user. For example, a modal barrier has `scopesRoute` set + /// but marking it as a dialog would be wrong. + dialog, } /// A function that creates a [RoleManager] for a [SemanticsObject]. @@ -370,6 +383,7 @@ final Map _roleFactories = { Role.checkable: (SemanticsObject object) => Checkable(object), Role.image: (SemanticsObject object) => ImageRoleManager(object), Role.liveRegion: (SemanticsObject object) => LiveRegion(object), + Role.dialog: (SemanticsObject object) => Dialog(object), }; /// Provides the functionality associated with the role of the given @@ -845,6 +859,15 @@ class SemanticsObject { !hasAction(ui.SemanticsAction.tap) && !hasFlag(ui.SemanticsFlag.isButton); + /// Whether this node should be treated as an ARIA dialog. + /// + /// See also [Role.dialog]. + bool get isDialog { + final bool scopesRoute = hasFlag(ui.SemanticsFlag.scopesRoute); + final bool namesRoute = hasFlag(ui.SemanticsFlag.namesRoute); + return scopesRoute && namesRoute; + } + /// Whether this object carry enabled/disabled state (and if so whether it is /// enabled). /// @@ -1241,7 +1264,11 @@ class SemanticsObject { /// Detects the roles that this semantics object corresponds to and manages /// the lifecycles of [SemanticsObjectRole] objects. void _updateRoles() { - _updateRole(Role.labelAndValue, (hasLabel || hasValue || hasTooltip) && !isTextField && !isVisualOnly); + // Some role managers manage labels themselves for various role-specific reasons. + final bool managesOwnLabel = isTextField || isDialog || isVisualOnly; + _updateRole(Role.labelAndValue, (hasLabel || hasValue || hasTooltip) && !managesOwnLabel); + + _updateRole(Role.dialog, isDialog); _updateRole(Role.textField, isTextField); final bool shouldUseTappableRole = @@ -1393,6 +1420,16 @@ class SemanticsObject { } } + /// Recursively visits the tree rooted at `this` node in depth-first fashion. + /// + /// Calls the [callback] for `this` node, then for all of its descendants. + void visitDepthFirst(void Function(SemanticsObject) callback) { + callback(this); + _currentChildrenInRenderOrder?.forEach((SemanticsObject child) { + child.visitDepthFirst(callback); + }); + } + @override String toString() { if (assertionsEnabled) { @@ -1468,7 +1505,7 @@ class EngineSemanticsOwner { /// Map [SemanticsObject.id] to parent [SemanticsObject] it was attached to /// this frame. - Map _attachments = {}; + Map _attachments = {}; /// Declares that the [child] must be attached to the [parent]. /// @@ -1484,17 +1521,19 @@ class EngineSemanticsOwner { /// /// The objects in this list will be detached permanently unless they are /// reattached via the [_attachObject] method. - List _detachments = []; + List _detachments = []; /// Declares that the [SemanticsObject] with the given [id] was detached from /// its current parent object. /// /// The object will be detached permanently unless it is reattached via the /// [_attachObject] method. - void _detachObject(int? id) { + void _detachObject(int id) { assert(_semanticsTree.containsKey(id)); final SemanticsObject? object = _semanticsTree[id]; - _detachments.add(object); + if (object != null) { + _detachments.add(object); + } } /// Callbacks called after all objects in the tree have their properties @@ -1513,20 +1552,30 @@ class EngineSemanticsOwner { /// the one-time callbacks scheduled via the [addOneTimePostUpdateCallback] /// method. void _finalizeTree() { - for (final SemanticsObject? object in _detachments) { - final SemanticsObject? parent = _attachments[object!.id]; - if (parent == null) { - // Was not reparented and is removed permanently from the tree. - _semanticsTree.remove(object.id); - object._parent = null; - object.element.remove(); - } else { - assert(object._parent == parent); - assert(object.element.parentNode == parent._childContainerElement); + for (final SemanticsObject detachmentRoot in _detachments) { + // A detached node may or may not have some of its descendants reattached + // elsewhere. Walk the descendant tree and find all descendants that were + // reattached to a parent. Those descendants need to be removed. + final List removals = []; + detachmentRoot.visitDepthFirst((SemanticsObject node) { + final SemanticsObject? parent = _attachments[node.id]; + if (parent == null) { + // Was not reparented and is removed permanently from the tree. + removals.add(node); + } else { + assert(node._parent == parent); + assert(node.element.parentNode == parent._childContainerElement); + } + }); + + for (final SemanticsObject removal in removals) { + _semanticsTree.remove(removal.id); + removal._parent = null; + removal.element.remove(); } } - _detachments = []; - _attachments = {}; + _detachments = []; + _attachments = {}; if (_oneTimePostUpdateCallbacks.isNotEmpty) { for (final ui.VoidCallback callback in _oneTimePostUpdateCallbacks) { @@ -1595,7 +1644,7 @@ class EngineSemanticsOwner { _gestureMode = GestureMode.pointerEvents; _notifyGestureModeListeners(); } - final List keys = _semanticsTree.keys.toList(); + final List keys = _semanticsTree.keys.toList(); final int len = keys.length; for (int i = 0; i < len; i++) { _detachObject(keys[i]); @@ -1828,7 +1877,23 @@ class EngineSemanticsOwner { assert(_semanticsTree.containsKey(0)); // must contain root node assert(() { - // Validate tree + // Validate that the node map only contains live elements, i.e. descendants + // of the root node. If a node is not reachable from the root, it should + // have been removed from the map. + final List liveIds = []; + final SemanticsObject root = _semanticsTree[0]!; + root.visitDepthFirst((SemanticsObject child) { + liveIds.add(child.id); + }); + 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(', ')}]' + ); + } + + // Validate that each node in the final tree is self-consistent. _semanticsTree.forEach((int? id, SemanticsObject object) { assert(id == object.id); diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index e4da65ad22d69..2da8ffea3544a 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -847,6 +847,92 @@ void _testContainer() { semantics().semanticsEnabled = false; }); + + test('descendant nodes are removed from the node map, unless reparented', () async { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + { + final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder(); + updateNode( + builder, + childrenInTraversalOrder: Int32List.fromList([1, 2]), + childrenInHitTestOrder: Int32List.fromList([1, 2]), + ); + updateNode( + builder, + id: 1, + childrenInTraversalOrder: Int32List.fromList([3, 4]), + childrenInHitTestOrder: Int32List.fromList([3, 4]), + ); + updateNode( + builder, + id: 2, + childrenInTraversalOrder: Int32List.fromList([5, 6]), + childrenInHitTestOrder: Int32List.fromList([5, 6]), + ); + updateNode(builder, id: 3); + updateNode(builder, id: 4); + updateNode(builder, id: 5); + updateNode(builder, id: 6); + + semantics().updateSemantics(builder.build()); + expectSemanticsTree(''' + + + + + + + + + + + + + + + + '''); + + expect(EngineSemanticsOwner.instance.debugSemanticsTree!.keys.toList(), unorderedEquals([0, 1, 2, 3, 4, 5, 6])); + } + + // Remove node #2 => expect nodes #2 and #5 to be removed and #6 reparented. + { + final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder(); + updateNode( + builder, + childrenInTraversalOrder: Int32List.fromList([1]), + childrenInHitTestOrder: Int32List.fromList([1]), + ); + updateNode( + builder, + id: 1, + childrenInTraversalOrder: Int32List.fromList([3, 4, 6]), + childrenInHitTestOrder: Int32List.fromList([3, 4, 6]), + ); + + semantics().updateSemantics(builder.build()); + expectSemanticsTree(''' + + + + + + + + + + + '''); + + expect(EngineSemanticsOwner.instance.debugSemanticsTree!.keys.toList(), unorderedEquals([0, 1, 3, 4, 6])); + } + + semantics().semanticsEnabled = false; + }); } void _testVerticalScrolling() { @@ -951,7 +1037,7 @@ void _testVerticalScrolling() { childrenInTraversalOrder: Int32List.fromList([1, 2, 3]), ); - for (int id = 1; id <= 5; id++) { + for (int id = 1; id <= 3; id++) { updateNode( builder, id: id, From f691fec8e3efc5db513074787ec67a1ccb6fabc1 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 2 May 2023 18:10:33 -0700 Subject: [PATCH 2/5] dartdocs for Dialog --- lib/web_ui/lib/src/engine/semantics/dialog.dart | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index 9b13dbbe1f140..bc1dc4e24d461 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -6,6 +6,9 @@ import '../dom.dart'; import '../semantics.dart'; import '../util.dart'; +/// Provides accessibility for dialogs. +/// +/// See also [Role.dialog]. class Dialog extends RoleManager { Dialog(SemanticsObject semanticsObject) : super(Role.dialog, semanticsObject); From b7ce35a84c7665f9b47bbb942fc537b5a6a04187 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 2 May 2023 20:35:51 -0700 Subject: [PATCH 3/5] licenses_flutter --- ci/licenses_golden/licenses_flutter | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 6ff4b90ad871e..4a12e0362a33a 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1976,6 +1976,7 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/safe_browser_api.dart + ../.. ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/accessibility.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/checkable.dart + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/dialog.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/image.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/incrementable.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/label_and_value.dart + ../../../flutter/LICENSE @@ -4590,6 +4591,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/safe_browser_api.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/accessibility.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/checkable.dart +FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/dialog.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/image.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/incrementable.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/label_and_value.dart From db1f4cac8cb6c3e4310cd542ec0fbabeac0fc242 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Wed, 3 May 2023 11:07:32 -0700 Subject: [PATCH 4/5] add test --- .../test/engine/semantics/semantics_test.dart | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 2da8ffea3544a..b7fbe9284f8d6 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -82,6 +82,9 @@ void runSemanticsTests() { group('group', () { _testGroup(); }); + group('dialog', () { + _testDialog(); + }); } void _testEngineAccessibilityBuilder() { @@ -2172,6 +2175,73 @@ void _testGroup() { }); } +void _testDialog() { + test('renders named and labeled routes', () { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder(); + updateNode( + builder, + label: 'this is a dialog label', + flags: 0 | ui.SemanticsFlag.scopesRoute.index | ui.SemanticsFlag.namesRoute.index, + transform: Matrix4.identity().toFloat64(), + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + childrenInHitTestOrder: Int32List.fromList([1]), + childrenInTraversalOrder: Int32List.fromList([1]), + ); + updateNode( + builder, + id: 1, + transform: Matrix4.identity().toFloat64(), + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ); + + semantics().updateSemantics(builder.build()); + expectSemanticsTree(''' + +'''); + + semantics().semanticsEnabled = false; + }); + + test('warns about missing label', () { + final List warnings = []; + printWarning = warnings.add; + + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder(); + updateNode( + builder, + flags: 0 | ui.SemanticsFlag.scopesRoute.index | ui.SemanticsFlag.namesRoute.index, + transform: Matrix4.identity().toFloat64(), + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + childrenInHitTestOrder: Int32List.fromList([1]), + childrenInTraversalOrder: Int32List.fromList([1]), + ); + updateNode( + builder, + id: 1, + transform: Matrix4.identity().toFloat64(), + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ); + + semantics().updateSemantics(builder.build()); + expect( + warnings, + [ + '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.', + ], + ); + + semantics().semanticsEnabled = false; + }); +} + /// A facade in front of [ui.SemanticsUpdateBuilder.updateNode] that /// supplies default values for semantics attributes. // TODO(yjbanov): move this to TestSemanticsBuilder From 88bfcfafdb30266a8eb38345248a5f0ba3705480 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 9 May 2023 18:30:03 -0700 Subject: [PATCH 5/5] address comments --- lib/web_ui/lib/src/engine/semantics/dialog.dart | 4 ++-- .../lib/src/engine/semantics/semantics.dart | 15 +++++++-------- .../test/engine/semantics/semantics_test.dart | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index bc1dc4e24d461..5f9c8bb869b2c 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -20,8 +20,8 @@ class Dialog extends RoleManager { @override void update() { + final String? label = semanticsObject.label; assert(() { - final String? label = semanticsObject.label; if (label == null || label.trim().isEmpty) { printWarning( 'Semantic node ${semanticsObject.id} was assigned dialog role, but ' @@ -32,7 +32,7 @@ class Dialog extends RoleManager { } return true; }()); - semanticsObject.element.setAttribute('aria-label', semanticsObject.label ?? ''); + semanticsObject.element.setAttribute('aria-label', label ?? ''); semanticsObject.setAriaRole('dialog', true); } } diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index a8a274de9c96e..94cfdc536d939 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -1529,8 +1529,8 @@ class EngineSemanticsOwner { /// The object will be detached permanently unless it is reattached via the /// [_attachObject] method. void _detachObject(int id) { - assert(_semanticsTree.containsKey(id)); final SemanticsObject? object = _semanticsTree[id]; + assert(object != null); if (object != null) { _detachments.add(object); } @@ -1885,13 +1885,12 @@ class EngineSemanticsOwner { root.visitDepthFirst((SemanticsObject child) { liveIds.add(child.id); }); - 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(', ')}]' - ); - } + assert( + _semanticsTree.keys.every(liveIds.contains), + 'The semantics node map is inconsistent:\n' + ' Nodes in tree: [${liveIds.join(', ')}]\n' + ' Nodes in map : [${_semanticsTree.keys.join(', ')}]' + ); // Validate that each node in the final tree is self-consistent. _semanticsTree.forEach((int? id, SemanticsObject object) { diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index b7fbe9284f8d6..3f8e72158ab57 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -2203,6 +2203,11 @@ void _testDialog() { '''); + expect( + semantics().debugSemanticsTree![0]!.debugRoleManagerFor(Role.dialog), + isA(), + ); + semantics().semanticsEnabled = false; }); @@ -2238,6 +2243,16 @@ void _testDialog() { ], ); + // But still sets the dialog role. + expectSemanticsTree(''' + +'''); + + expect( + semantics().debugSemanticsTree![0]!.debugRoleManagerFor(Role.dialog), + isA(), + ); + semantics().semanticsEnabled = false; }); }