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

Fix double speaking label in angular checkbox #10552

Merged
merged 5 commits into from
Dec 3, 2019

Conversation

feerrenrut
Copy link
Contributor

Link to issue number:

https://bugs.chromium.org/p/chromium/issues/detail?id=995603

Summary of the issue:

The HTML for checkboxes on https://dart-lang.github.io/angular_components/#/material_checkbox have aria-labelledby pointing to a sub-element of the checkbox. Normally, we announce the accessible name as we descend then read the child elements. This results in double speaking the name.

Description of how this pull request fixes the issue:

When rendering the virtual buffer, track the ID's of the nodes. If this node has a label, check if the ID for the label node matches any sub-node then set an attribute labelledByContents. Then in speech.py don't speak the name for a controlField if it has this attribute.

Testing performed:

Simplified test case:

data:text/html,<button>begin</button><div tabindex="0" role="checkbox" aria-labelledby="inner-label"><div style="display:inline" id="inner-label">Simulate evil cat</div></div>

Known issues with pull request:

Change log entry:

Section: New features, Changes, Bug fixes

@feerrenrut feerrenrut self-assigned this Nov 27, 2019
feerrenrut added a commit that referenced this pull request Nov 27, 2019
Example from "Fix double speaking label in angular checkbox" #10552
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get what parentIDs does? I get that outIds stores all descendant IDs, and that if the labelledID appears in the list, then you mark it as the label coming from content.
An out of the box idea though:
Since the buffer already stores nodes by ID, rather than using your own ID lists, why not just make use of what the buffer has already. So call GetControlfieldNodeByIdentifier with the label ID, once before all the possible recursions, and then once after, at the end. If a node did not exist before, but does after, than the node is a descendant. Note that getControlFieldNodeByIdentifier is very cheep as it itself just looks up the ID in a map.

@feerrenrut
Copy link
Contributor Author

I don't quite get what parentIDs does?

While developing this I wasn't sure if I would need to check parent ID's on the child side, and left it in for the symmetry.

I see what you are suggesting, with getControlFieldNodeWithIdentifier, it's a clever approach, but I think it makes the code harder to maintain, since it implicitly depends on how that map is built. Though, looking at how that map is built (via the calls to addControlFieldNode), in the same place I'll build an ID tree.

During addControlFieldNode a mapping
of parent->list[immediate children IDs] is built.

This can then be used to get all the children for a node, or all parents
for a node if necessary.
The node tree already has all the information necessary, so removing
the node ID map.
@feerrenrut feerrenrut merged commit 12aef7b into master Dec 3, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Dec 3, 2019
feerrenrut added a commit that referenced this pull request Dec 3, 2019
feerrenrut added a commit that referenced this pull request Dec 4, 2019
- Also fix a typo in #10552
feerrenrut added a commit that referenced this pull request Dec 4, 2019
- Also fix a typo in #10552
feerrenrut added a commit that referenced this pull request Dec 4, 2019
@feerrenrut feerrenrut deleted the fixDoubleSpeakingMaterialUICheckbox branch January 17, 2020 08:55
@maniekins
Copy link

I noticed, the aria-labelledby is not working with inner elements after 2019.3 version and it is probably related with this changes. I tested example 2 from https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-labelledby_attribute , added some button next to header, and when we are focusing on that button, main is vocalized but without h1 label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants