-
Notifications
You must be signed in to change notification settings - Fork 6k
[web] dialog a11y fixes #41681
[web] dialog a11y fixes #41681
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| // 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'; | ||
|
|
||
| /// Provides accessibility for dialogs. | ||
| /// | ||
| /// See also [Role.dialog]. | ||
| class Dialog extends RoleManager { | ||
| Dialog(SemanticsObject semanticsObject) : super(Role.dialog, semanticsObject); | ||
|
|
||
| @override | ||
| void dispose() { | ||
| semanticsObject.element.removeAttribute('aria-label'); | ||
| semanticsObject.clearAriaRole(); | ||
| } | ||
|
|
||
| @override | ||
| void update() { | ||
| final String? label = semanticsObject.label; | ||
| assert(() { | ||
| 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', label ?? ''); | ||
| semanticsObject.setAriaRole('dialog', true); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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<Role, RoleManagerFactory> _roleFactories = <Role, RoleManagerFactory>{ | |||||||||
| 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); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #41681 (comment) |
||||||||||
| _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<int?, SemanticsObject> _attachments = <int?, SemanticsObject>{}; | ||||||||||
| Map<int, SemanticsObject> _attachments = <int, SemanticsObject>{}; | ||||||||||
|
|
||||||||||
| /// 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<SemanticsObject?> _detachments = <SemanticsObject?>[]; | ||||||||||
| List<SemanticsObject> _detachments = <SemanticsObject>[]; | ||||||||||
|
|
||||||||||
| /// 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) { | ||||||||||
| assert(_semanticsTree.containsKey(id)); | ||||||||||
| void _detachObject(int id) { | ||||||||||
| final SemanticsObject? object = _semanticsTree[id]; | ||||||||||
| _detachments.add(object); | ||||||||||
| assert(object != null); | ||||||||||
| if (object != null) { | ||||||||||
| _detachments.add(object); | ||||||||||
| } | ||||||||||
|
Comment on lines
+1534
to
+1536
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are asserting that the tree contains this
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm playing it safe. My worry is that we can get something inconsistent from the framework. Because child nodes are supplied as |
||||||||||
| } | ||||||||||
|
|
||||||||||
| /// 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<SemanticsObject> removals = <SemanticsObject>[]; | ||||||||||
| detachmentRoot.visitDepthFirst((SemanticsObject node) { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #41681 (comment) |
||||||||||
| final SemanticsObject? parent = _attachments[node.id]; | ||||||||||
| if (parent == null) { | ||||||||||
| // Was not reparented and is removed permanently from the tree. | ||||||||||
| removals.add(node); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once you add a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||
| } 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 = <SemanticsObject?>[]; | ||||||||||
| _attachments = <int?, SemanticsObject>{}; | ||||||||||
| _detachments = <SemanticsObject>[]; | ||||||||||
| _attachments = <int, SemanticsObject>{}; | ||||||||||
|
|
||||||||||
| if (_oneTimePostUpdateCallbacks.isNotEmpty) { | ||||||||||
| for (final ui.VoidCallback callback in _oneTimePostUpdateCallbacks) { | ||||||||||
|
|
@@ -1595,7 +1644,7 @@ class EngineSemanticsOwner { | |||||||||
| _gestureMode = GestureMode.pointerEvents; | ||||||||||
| _notifyGestureModeListeners(); | ||||||||||
| } | ||||||||||
| final List<int?> keys = _semanticsTree.keys.toList(); | ||||||||||
| final List<int> keys = _semanticsTree.keys.toList(); | ||||||||||
| final int len = keys.length; | ||||||||||
| for (int i = 0; i < len; i++) { | ||||||||||
| _detachObject(keys[i]); | ||||||||||
|
|
@@ -1828,7 +1877,22 @@ 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<int> liveIds = <int>[]; | ||||||||||
| final SemanticsObject root = _semanticsTree[0]!; | ||||||||||
| root.visitDepthFirst((SemanticsObject child) { | ||||||||||
| liveIds.add(child.id); | ||||||||||
| }); | ||||||||||
| 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) { | ||||||||||
| assert(id == object.id); | ||||||||||
|
|
||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to refactor this right now but... shouldn't this be the responsibility of each specialized class of RoleManager?
Why do we need
isDialogin a SemanticsObject, and an _updateRoles that knows about all possible roles, when we eventually wrap this in aDialogRoleManager that knows about how dialogs should behave?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
roleattribute. 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