fix: Clean up accessibility node hierarchy (experimental) #9449
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
BenHenning
changed the base branch from
develop
to
add-screen-reader-support-experimental
|
Looks like there's some CI failures to address. Moreover I haven't completed in-depth testing of the hierarchy--I want to check more block and fields types before considering this ready for review. |
Addresses some observed gaps in behaviors with connections. This is unfortunately mainly a patch without fully understanding the deep issue being avoided but it seems reasonable for the experimental branch.
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Quick spot check on the code.
|
CI failures seem to be existing and unrelated to this change. |
|
I think I'm happy with considering #9304 resolved with this PR. It's certainly possible we'll have deviations that will require future work, but as of now the tree is quite clean and I think most remaining filed work is largely on the label side and unlikely to impact the hierarchical relationships. Adding new focusable nodes to the graph might impact it, but that's not imminently planned. PTAL @maribethb. |
| } | ||
| } | ||
| for (const connection of this.getConnections_(true)) { | ||
| // TODO: Somehow it's possible for a connection to be highlighted but |
There was a problem hiding this comment.
Can you file an issue for this and link it here so we don't lose track of this?
| if ( | ||
| connection.canBeFocused() && | ||
| connection.isHighlighted() && | ||
| connection.findHighlightSvg() !== null |
There was a problem hiding this comment.
Do you need to check this? What happens if you say aria-owns with an id that doesn't exist in the dom?
If you need to keep it, can you add the same bug ID in the todo that makes it public so that we can make it private again when it's no longer needed (I'm assuming if this bug didn't exist you wouldn't need to check this)
| } | ||
|
|
||
| private findHighlightSvg(): SVGPathElement | null { | ||
| // TODO: Figure out how to make this private again. |
There was a problem hiding this comment.
Please add a tsdoc with @internal so it doesn't get made part of our public API, and add same bug id from previous comment
| } | ||
| } | ||
|
|
||
| this.sourceBlock_.recomputeAriaLabel(); |
There was a problem hiding this comment.
It seems like a lot of extra work to recompute the entire aria label which includes searching through all of the inputs, child blocks, etc. just to remove one specific ID for the connection highlight, which we already know the ID of.
Could you keep track of the childElemIds as a set and just remove or add this connection ID to the set, and then update the aria-owns attribute with the set of IDs instead of recalculating the entire set of IDs on each highlight?
2 participants
The basics
The details
Resolves
Fixes #9304
Proposed Changes
Adds relevant
aria-ownsproperties to ensure that all focusable accessibility nodes have the correct parent/child relationships (particularly for blocks).Reason for Changes
Other PRs have made progress on removing extraneous accessibility nodes with #9446 being essentially the last of these. Ensuring that parent/child relationships are correct is the last step in ensuring that the entirety of the accessibility node graph is correctly representing the DOM and navigational structure of Blockly.
This can have implications and (ideally) improvements for certain screen reader modes that provide higher-level summarization and sometimes navigation (bypassing Blockly's keyboard navigation) since it avoids an incorrect flat node structure and instead ensures correct hierarchy and ordering.
Test Coverage
No automated tests are needed for this since it's experimental. However, manual testing involved adding various types of blocks and fields plus icons and mutators to ensure that relevant elements were correctly owned.
Testing findings: I used the following workspace to demonstrate the before and after accessibility graphs per these changes:
The node graph with these changes for those blocks:
The node graph without these changes for those blocks:
A more complex workspace that I checked (node tree not included):
I also checked against a random generation of blocks. Everything seems reasonably linked, sorted, and represented in the node tree among these different workspace configurations.
Documentation
No documentation changes are needed for these experimental changes.
Additional Information
Note that there are some limitations with this approach: text editors and listboxes (e.g. for comboboxes) are generally outside of the hierarchy represented by the Blockly workspace. This is an existing issue that remains unaffected by these changes, and fixing it to be both ARIA compliant and consistent with the DOM may not be possible (though it doesn't seem like there's a strong requirement to maintain DOM and accessibility node tree hierarchical relationships).
This is likely the cause of certain "region end"-esque announcements and may require additional investigation and work. It's considered outside the scope of this PR and #9304.
Finally, the changes here are rather ugly and would benefit from some more careful long-term technical design considerations (a la #9307).