-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix: Clean up accessibility node hierarchy (experimental) #9449
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
base: add-screen-reader-support-experimental
Are you sure you want to change the base?
Changes from all commits
b1d3d6d
a7f7810
f81e2b2
cc5d002
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 |
|---|---|---|
|
|
@@ -233,9 +233,58 @@ export class BlockSvg | |
| * @internal | ||
| */ | ||
| recomputeAriaLabel() { | ||
| if (this.initialized) { | ||
| const childElemIds: string[] = []; | ||
| for (const input of this.inputList) { | ||
| if (input.isVisible() && input.connection) { | ||
| if (input.connection.type === ConnectionType.NEXT_STATEMENT) { | ||
| let currentBlock: BlockSvg | null = | ||
| input.connection.targetBlock() as BlockSvg | null; | ||
| while (currentBlock) { | ||
| if (currentBlock.canBeFocused()) { | ||
| childElemIds.push(currentBlock.getBlockSvgFocusElem().id); | ||
| } | ||
| currentBlock = currentBlock.getNextBlock(); | ||
| } | ||
| } else if (input.connection.type === ConnectionType.INPUT_VALUE) { | ||
| const inpBlock = input.connection.targetBlock() as BlockSvg | null; | ||
| if (inpBlock && inpBlock.canBeFocused()) { | ||
| childElemIds.push(inpBlock.getBlockSvgFocusElem().id); | ||
| } | ||
| } | ||
| } | ||
| for (const field of input.fieldRow) { | ||
| if (field.getSvgRoot() && field.canBeFocused()) { | ||
| // Only track the field if it's been initialized. | ||
| childElemIds.push(field.getFocusableElement().id); | ||
| } | ||
| } | ||
| for (const icon of this.icons) { | ||
| if (icon.canBeFocused()) { | ||
| childElemIds.push(icon.getFocusableElement().id); | ||
| } | ||
| } | ||
| for (const connection of this.getConnections_(true)) { | ||
| // TODO: Somehow it's possible for a connection to be highlighted but | ||
| // have no focusable element. This might be some sort of race | ||
| // condition or perhaps dispose-esque situation happening. | ||
| if ( | ||
| connection.canBeFocused() && | ||
| connection.isHighlighted() && | ||
| connection.findHighlightSvg() !== null | ||
|
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. 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) |
||
| ) { | ||
| childElemIds.push(connection.getFocusableElement().id); | ||
| } | ||
| } | ||
| } | ||
| aria.setState(this.getBlockSvgFocusElem(), aria.State.OWNS, childElemIds); | ||
| } | ||
|
|
||
| if (this.isSimpleReporter()) { | ||
| const field = Array.from(this.getFields())[0]; | ||
| if (field.isFullBlockField() && field.isCurrentlyEditable()) return; | ||
| if (field && field.isFullBlockField() && field.isCurrentlyEditable()) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| aria.setState( | ||
|
|
@@ -245,6 +294,12 @@ export class BlockSvg | |
| ); | ||
| } | ||
|
|
||
| private getBlockSvgFocusElem(): Element { | ||
| // Note that this deviates from getFocusableElement() to ensure that | ||
| // single field blocks are properly set up in the hierarchy. | ||
| return this.pathObject.svgPath; | ||
| } | ||
|
|
||
| private computeAriaLabel(): string { | ||
| const {blockSummary, inputCount} = buildBlockSummary(this); | ||
| const inputSummary = inputCount | ||
|
|
@@ -353,6 +408,7 @@ export class BlockSvg | |
| this.workspace.getCanvas().appendChild(svg); | ||
| } | ||
| this.initialized = true; | ||
| this.recomputeAriaLabel(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -457,6 +513,12 @@ export class BlockSvg | |
| this.applyColour(); | ||
|
|
||
| this.workspace.recomputeAriaTree(); | ||
| this.recomputeAriaLabelRecursive(); | ||
| } | ||
|
|
||
| private recomputeAriaLabelRecursive() { | ||
| this.recomputeAriaLabel(); | ||
| this.parentBlock_?.recomputeAriaLabelRecursive(); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -362,6 +362,8 @@ export class RenderedConnection | |
| aria.setState(highlightSvg, aria.State.LABEL, 'Open connection'); | ||
| } | ||
| } | ||
|
|
||
| this.sourceBlock_.recomputeAriaLabel(); | ||
|
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. 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? |
||
| } | ||
|
|
||
| /** Remove the highlighting around this connection. */ | ||
|
|
@@ -373,6 +375,8 @@ export class RenderedConnection | |
| if (highlightSvg) { | ||
| highlightSvg.style.display = 'none'; | ||
| } | ||
|
|
||
| this.sourceBlock_.recomputeAriaLabel(); | ||
| } | ||
|
|
||
| /** Returns true if this connection is highlighted, false otherwise. */ | ||
|
|
@@ -688,7 +692,8 @@ export class RenderedConnection | |
| return true; | ||
| } | ||
|
|
||
| private findHighlightSvg(): SVGPathElement | null { | ||
| // TODO: Figure out how to make this private again. | ||
|
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. Please add a tsdoc with |
||
| findHighlightSvg(): SVGPathElement | null { | ||
| // This cast is valid as TypeScript's definition is wrong. See: | ||
| // https://github.com/microsoft/TypeScript/issues/60996. | ||
| return document.getElementById(this.id) as | ||
|
|
||
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.
Can you file an issue for this and link it here so we don't lose track of this?