[web:a11y] introduce primary role responsible for ARIA roles #43159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
mdebbar
approved these changes
Jun 27, 2023
Comment on lines
+546
to
+552
| if (!semanticsObject.hasLabel) { | ||
| // The node didn't get a more specific role, and it has no label. It is | ||
| // likely that this node is simply there for positioning its children and | ||
| // has no other role for the screen reader to be aware of. In this case, | ||
| // the element does not need a `role` attribute at all. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is it possible for a node to have children but no label?
There was a problem hiding this comment.
Yep. That's actually a very common case. We have container nodes whose job is to position their children (e.g. apply offsets and other transforms), but otherwise they have no semantic role to play. In that case they don't get any role attribute at all.
|
Thanks for the cleanup! |
yjbanov
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jun 27, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jun 27, 2023
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Jun 27, 2023
…129678) flutter/engine@f320b8c...7c7c45d 2023-06-27 flar@google.com Update skia includes to be more specific (flutter/engine#43284) 2023-06-27 yjbanov@google.com [web:a11y] introduce primary role responsible for ARIA roles (flutter/engine#43159) 2023-06-27 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from ytzCCSvHY1lHWEDM9... to sBFKkha8HNLZpTNwv... (flutter/engine#43277) 2023-06-27 skia-flutter-autoroll@skia.org Roll ANGLE from 9faf7059f9ef to 113f847be69f (2 revisions) (flutter/engine#43278) 2023-06-27 jacksongardner@google.com Initialize skwasm codecs before handing them back to the user. (flutter/engine#43274) 2023-06-27 skia-flutter-autoroll@skia.org Roll ANGLE from 02292814a9d3 to 9faf7059f9ef (7 revisions) (flutter/engine#43272) 2023-06-27 15619084+vashworth@users.noreply.github.com Update Xcode to 14.3.1 (flutter/engine#42930) 2023-06-27 jonahwilliams@google.com [Impeller] Add Vulkan allocator traces. (flutter/engine#43215) 2023-06-27 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from bj_X2Se1zObk_l_CC... to Bvv7TyHm_VHUkndFx... (flutter/engine#43270) 2023-06-27 chinmaygarde@google.com [Impeller] Report pipeline creation feedback to logs and traces. (flutter/engine#43227) 2023-06-27 jonahwilliams@google.com [Impeller] Give Impeller a dedicated raster priority level worker loop. (flutter/engine#43166) 2023-06-27 jason-simmons@users.noreply.github.com [Impeller] Fixes for GLES color mask setup (flutter/engine#43225) 2023-06-27 skia-flutter-autoroll@skia.org Roll ANGLE from cba77bceb26c to 02292814a9d3 (1 revision) (flutter/engine#43224) 2023-06-27 skia-flutter-autoroll@skia.org Roll Skia from 370132bcadb1 to 5209dc7702d0 (1 revision) (flutter/engine#43223) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from bj_X2Se1zObk to Bvv7TyHm_VHU fuchsia/sdk/core/mac-amd64 from ytzCCSvHY1lH to sBFKkha8HNLZ 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 jimgraham@google.com,rmistry@google.com,zra@google.com 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
kjlubick
pushed a commit
to kjlubick/engine
that referenced
this pull request
Jul 14, 2023
…#43159) This PR fixes flutter/flutter#128468 by changing the relationship between semantics nodes and their roles from this: ``` SemanticsNode one-to-many RoleManager ``` To this: ``` SemanticsNode one-to-one PrimaryRoleManager one-to-many RoleManager ``` Previously a node would simply have multiple role managers, some of which would be responsible for setting the `role` attribute. It wasn't clear which role manager should be doing this. It also wasn't clear which role managers were safe to reuse across multiple types of nodes. This led to the unfortunate situation in flutter/flutter#128468 where `LabelAndValue` ended up overriding the role assigned by `Checkable`. With this PR, a `SemanticsNode` has exactly one `PrimaryRoleManager`. A primary role manager is responsible for setting the `role` attribute, and importantly, it's the _only_ thing responsible for it. It's _not safe_ to share primary role managers across different kinds of nodes. They are meant to provide very specific functionality for the widget's main role. OTOH, a non-primary `RoleManager` provides a piece of functionality that's safe to share. A `Checkable` is a `PrimaryRoleManager` and is the only thing that decides on the `role` attribute. `LabelAndValue` is now a `RoleManager` that's not responsible for setting the role. It's only responsible for `aria-label`. No more confusion. This also drastically simplifies the logic for role assignment. There's no more [logical soup](https://github.com/flutter/engine/blob/eca910dd5e3f1d8e18b10f3a46ce8d1454a232c8/lib/web_ui/lib/src/engine/semantics/semantics.dart#L1340) attempting to find a good subset of roles to assign to a node. [Finding](https://github.com/yjbanov/engine/blob/93df91df9575f8fc212aac115ccccc23f8fba26f/lib/web_ui/lib/src/engine/semantics/semantics.dart#L1477) and [instantiating](https://github.com/yjbanov/engine/blob/93df91df9575f8fc212aac115ccccc23f8fba26f/lib/web_ui/lib/src/engine/semantics/semantics.dart#L1498) primary roles are very linear steps, as is [assigning a set of secondary roles](https://github.com/yjbanov/engine/blob/93df91df9575f8fc212aac115ccccc23f8fba26f/lib/web_ui/lib/src/engine/semantics/image.dart#L16).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes flutter/flutter#128468 by changing the relationship between semantics nodes and their roles from this:
To this:
Previously a node would simply have multiple role managers, some of which would be responsible for setting the
roleattribute. It wasn't clear which role manager should be doing this. It also wasn't clear which role managers were safe to reuse across multiple types of nodes. This led to the unfortunate situation in flutter/flutter#128468 whereLabelAndValueended up overriding the role assigned byCheckable.With this PR, a
SemanticsNodehas exactly onePrimaryRoleManager. A primary role manager is responsible for setting theroleattribute, and importantly, it's the only thing responsible for it. It's not safe to share primary role managers across different kinds of nodes. They are meant to provide very specific functionality for the widget's main role. OTOH, a non-primaryRoleManagerprovides a piece of functionality that's safe to share.A
Checkableis aPrimaryRoleManagerand is the only thing that decides on theroleattribute.LabelAndValueis now aRoleManagerthat's not responsible for setting the role. It's only responsible foraria-label. No more confusion.This also drastically simplifies the logic for role assignment. There's no more logical soup attempting to find a good subset of roles to assign to a node. Finding and instantiating primary roles are very linear steps, as is assigning a set of secondary roles.