Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web:a11y] LabelAndValue role overrides Checkable role #128468

Closed
yjbanov opened this issue Jun 7, 2023 · 1 comment · Fixed by flutter/engine#43159
Closed

[web:a11y] LabelAndValue role overrides Checkable role #128468

yjbanov opened this issue Jun 7, 2023 · 1 comment · Fixed by flutter/engine#43159
Assignees
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list platform-web Web applications specifically

Comments

@yjbanov
Copy link
Contributor

yjbanov commented Jun 7, 2023

When the framework merges Checkbox semantics with Text semantics into a single node, the LabelAndValue role manager can take precedence and end up assigning role="text" to the HTML DOM element, which confuses screen readers.

Sample widget structure that leads to this situation:

  Widget build(BuildContext context) {
    return ListView(
      children: <Widget>[
        Row(
          children: [
            Checkbox(
              autofocus: true,
              value: _value,
              onChanged: (value) {
                setState(() {
                  _value = value!;
                });
              },
            ),
            const Text('this is a label'),
          ],
        ),
      ],
    );
  }
@yjbanov yjbanov added engine flutter/engine repository. See also e: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) platform-web Web applications specifically P1 High-priority issues at the top of the work list labels Jun 7, 2023
@yjbanov yjbanov self-assigned this Jun 7, 2023
auto-submit bot pushed a commit to flutter/engine that referenced this issue Jun 27, 2023
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).
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2023
kjlubick pushed a commit to kjlubick/engine that referenced this issue 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list platform-web Web applications specifically
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant