Skip to content

Commit 7288860

Browse files
BenHenningBenHenning
authored
fix: Clean up accessibility node hierarchy (experimental) (#9449)
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #9304 ### Proposed Changes Fixes non-visual parent roles and rendered connection navigation policy. ### 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. It was discovered during the development of the PR that setting `aria-owns` properties to ensure that all focusable accessibility nodes have the correct parent/child relationships (particularly for blocks) isn't actually viable per the analysis summarized in this comment: #9449 (comment). At a high level introducing these relationships seems to actually cause problems in both ChromeVox and Voiceover. Part of the analysis discovered that nodes set with the `presentation` role aren't going to behave correctly due to the spec ignoring that role if any children of such elements are focusable, so this PR does change those over to `generic` which is more correct. They are still missing in Chrome's accessibility node viewer, and `generic` _seems_ to introduce slightly better `group` behaviors on VoiceOver (that is, it seems to reduce some of the `group` announcements which VoiceOver is known for over-specifying). Note that some tests needed to be updated to ensure that they were properly rendering blocks (in order for `RenderedConnection.canBeFocused()` to behave correctly) in the original implementation of the PR. Only one test actually changed in behavior because it seemed like it was incorrect before--the particular connection being tested wasn't actually navigable and the change to `canBeFocused` actually enforces that. These changes were kept even though the behaviors weren't needed anymore since it's still a bit more correct than before. Overall, #9304 is closed here because the tree seems to be about as good as it can get with current knowledge (assuming no other invalid roles need to be fixed, but that can be addressed in separate issues as needed). ### Test Coverage No automated tests are needed for this since it's experimental but it has been manually tested with both ChromeVox and Voiceover. ### 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). The analysis linked above also considered introducing a top-level `application` role which might change some of the automated behaviors of certain roles but this only seemed to worsen local testing with ChromeVox so it was excluded.
1 parent bb342f9 commit 7288860

File tree

6 files changed

+84
-56
lines changed

6 files changed

+84
-56
lines changed

‎core/block_svg.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ export class BlockSvg
365365
this.workspace.getCanvas().appendChild(svg);
366366
}
367367
this.initialized = true;
368+
this.recomputeAriaLabel();
368369
}
369370

370371
/**

‎core/rendered_connection.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -688,7 +688,7 @@ export class RenderedConnection
688688

689689
/** See IFocusableNode.canBeFocused. */
690690
canBeFocused(): boolean {
691-
return true;
691+
return this.findHighlightSvg() !== null;
692692
}
693693

694694
private findHighlightSvg(): SVGPathElement | null {

‎core/utils/dom.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export function createSvgElement<T extends SVGElement>(
6464
opt_parent.appendChild(e);
6565
}
6666
if (name === Svg.SVG || name === Svg.G) {
67-
aria.setRole(e, aria.Role.PRESENTATION);
67+
aria.setRole(e, aria.Role.GENERIC);
6868
}
6969
return e;
7070
}

‎tests/mocha/cursor_test.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,7 @@ suite('Cursor', function () {
342342
});
343343
suite('one empty block', function () {
344344
setup(function () {
345-
this.blockA = this.workspace.newBlock('empty_block');
345+
this.blockA = createRenderedBlock(this.workspace, 'empty_block');
346346
});
347347
teardown(function () {
348348
this.workspace.clear();
@@ -359,7 +359,7 @@ suite('Cursor', function () {
359359

360360
suite('one stack block', function () {
361361
setup(function () {
362-
this.blockA = this.workspace.newBlock('stack_block');
362+
this.blockA = createRenderedBlock(this.workspace, 'stack_block');
363363
});
364364
teardown(function () {
365365
this.workspace.clear();
@@ -376,7 +376,7 @@ suite('Cursor', function () {
376376

377377
suite('one row block', function () {
378378
setup(function () {
379-
this.blockA = this.workspace.newBlock('row_block');
379+
this.blockA = createRenderedBlock(this.workspace, 'row_block');
380380
});
381381
teardown(function () {
382382
this.workspace.clear();
@@ -392,7 +392,7 @@ suite('Cursor', function () {
392392
});
393393
suite('one c-hat block', function () {
394394
setup(function () {
395-
this.blockA = this.workspace.newBlock('c_hat_block');
395+
this.blockA = createRenderedBlock(this.workspace, 'c_hat_block');
396396
});
397397
teardown(function () {
398398
this.workspace.clear();

‎tests/mocha/navigation_test.js‎

Lines changed: 75 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
*/
66

77
import {assert} from '../../node_modules/chai/index.js';
8+
import {createRenderedBlock} from './test_helpers/block_definitions.js';
89
import {
910
sharedTestSetup,
1011
sharedTestTeardown,
11-
workspaceTeardown,
1212
} from './test_helpers/setup_teardown.js';
1313

1414
suite('Navigation', function () {
@@ -89,13 +89,28 @@ suite('Navigation', function () {
8989
]);
9090
this.workspace = Blockly.inject('blocklyDiv', {});
9191
this.navigator = this.workspace.getNavigator();
92-
const statementInput1 = this.workspace.newBlock('input_statement');
93-
const statementInput2 = this.workspace.newBlock('input_statement');
94-
const statementInput3 = this.workspace.newBlock('input_statement');
95-
const statementInput4 = this.workspace.newBlock('input_statement');
96-
const fieldWithOutput = this.workspace.newBlock('field_input');
97-
const doubleValueInput = this.workspace.newBlock('double_value_input');
98-
const valueInput = this.workspace.newBlock('value_input');
92+
const statementInput1 = createRenderedBlock(
93+
this.workspace,
94+
'input_statement',
95+
);
96+
const statementInput2 = createRenderedBlock(
97+
this.workspace,
98+
'input_statement',
99+
);
100+
const statementInput3 = createRenderedBlock(
101+
this.workspace,
102+
'input_statement',
103+
);
104+
const statementInput4 = createRenderedBlock(
105+
this.workspace,
106+
'input_statement',
107+
);
108+
const fieldWithOutput = createRenderedBlock(this.workspace, 'field_input');
109+
const doubleValueInput = createRenderedBlock(
110+
this.workspace,
111+
'double_value_input',
112+
);
113+
const valueInput = createRenderedBlock(this.workspace, 'value_input');
99114

100115
statementInput1.nextConnection.connect(statementInput2.previousConnection);
101116
statementInput1.inputList[0].connection.connect(
@@ -355,13 +370,25 @@ suite('Navigation', function () {
355370
'helpUrl': '',
356371
},
357372
]);
358-
const noNextConnection = this.workspace.newBlock('top_connection');
359-
const fieldAndInputs = this.workspace.newBlock('fields_and_input');
360-
const twoFields = this.workspace.newBlock('two_fields');
361-
const fieldAndInputs2 = this.workspace.newBlock('fields_and_input2');
362-
const noPrevConnection = this.workspace.newBlock('start_block');
363-
const hiddenField = this.workspace.newBlock('hidden_field');
364-
const hiddenInput = this.workspace.newBlock('hidden_input');
373+
const noNextConnection = createRenderedBlock(
374+
this.workspace,
375+
'top_connection',
376+
);
377+
const fieldAndInputs = createRenderedBlock(
378+
this.workspace,
379+
'fields_and_input',
380+
);
381+
const twoFields = createRenderedBlock(this.workspace, 'two_fields');
382+
const fieldAndInputs2 = createRenderedBlock(
383+
this.workspace,
384+
'fields_and_input2',
385+
);
386+
const noPrevConnection = createRenderedBlock(
387+
this.workspace,
388+
'start_block',
389+
);
390+
const hiddenField = createRenderedBlock(this.workspace, 'hidden_field');
391+
const hiddenInput = createRenderedBlock(this.workspace, 'hidden_input');
365392
this.blocks.noNextConnection = noNextConnection;
366393
this.blocks.fieldAndInputs = fieldAndInputs;
367394
this.blocks.twoFields = twoFields;
@@ -373,28 +400,47 @@ suite('Navigation', function () {
373400
hiddenField.inputList[0].fieldRow[1].setVisible(false);
374401
hiddenInput.inputList[1].setVisible(false);
375402

376-
const dummyInput = this.workspace.newBlock('dummy_input');
377-
const dummyInputValue = this.workspace.newBlock('dummy_inputValue');
378-
const fieldWithOutput2 = this.workspace.newBlock('field_input');
403+
const dummyInput = createRenderedBlock(this.workspace, 'dummy_input');
404+
const dummyInputValue = createRenderedBlock(
405+
this.workspace,
406+
'dummy_inputValue',
407+
);
408+
const fieldWithOutput2 = createRenderedBlock(
409+
this.workspace,
410+
'field_input',
411+
);
379412
this.blocks.dummyInput = dummyInput;
380413
this.blocks.dummyInputValue = dummyInputValue;
381414
this.blocks.fieldWithOutput2 = fieldWithOutput2;
382415

383-
const secondBlock = this.workspace.newBlock('input_statement');
384-
const outputNextBlock = this.workspace.newBlock('output_next');
416+
const secondBlock = createRenderedBlock(
417+
this.workspace,
418+
'input_statement',
419+
);
420+
const outputNextBlock = createRenderedBlock(
421+
this.workspace,
422+
'output_next',
423+
);
385424
this.blocks.secondBlock = secondBlock;
386425
this.blocks.outputNextBlock = outputNextBlock;
387426

388-
const buttonBlock = this.workspace.newBlock('buttons', 'button_block');
389-
const buttonInput1 = this.workspace.newBlock(
427+
const buttonBlock = createRenderedBlock(
428+
this.workspace,
429+
'buttons',
430+
'button_block',
431+
);
432+
const buttonInput1 = createRenderedBlock(
433+
this.workspace,
390434
'field_input',
391435
'button_input1',
392436
);
393-
const buttonInput2 = this.workspace.newBlock(
437+
const buttonInput2 = createRenderedBlock(
438+
this.workspace,
394439
'field_input',
395440
'button_input2',
396441
);
397-
const buttonNext = this.workspace.newBlock(
442+
const buttonNext = createRenderedBlock(
443+
this.workspace,
398444
'input_statement',
399445
'button_next',
400446
);
@@ -420,15 +466,6 @@ suite('Navigation', function () {
420466
this.workspace.cleanUp();
421467
});
422468
suite('Next', function () {
423-
setup(function () {
424-
this.singleBlockWorkspace = new Blockly.Workspace();
425-
const singleBlock = this.singleBlockWorkspace.newBlock('two_fields');
426-
this.blocks.singleBlock = singleBlock;
427-
});
428-
teardown(function () {
429-
workspaceTeardown.call(this, this.singleBlockWorkspace);
430-
});
431-
432469
test('fromPreviousToBlock', function () {
433470
const prevConnection = this.blocks.statementInput1.previousConnection;
434471
const nextNode = this.navigator.getNextSibling(prevConnection);
@@ -471,8 +508,6 @@ suite('Navigation', function () {
471508
});
472509
test('fromFieldToNestedBlock', function () {
473510
const field = this.blocks.statementInput1.inputList[0].fieldRow[1];
474-
const inputConnection =
475-
this.blocks.statementInput1.inputList[0].connection;
476511
const nextNode = this.navigator.getNextSibling(field);
477512
assert.equal(nextNode, this.blocks.fieldWithOutput);
478513
});
@@ -576,7 +611,6 @@ suite('Navigation', function () {
576611
const prevNode = this.navigator.getPreviousSibling(
577612
this.blocks.fieldWithOutput,
578613
);
579-
const outputConnection = this.blocks.fieldWithOutput.outputConnection;
580614
assert.equal(prevNode, [...this.blocks.statementInput1.getFields()][1]);
581615
});
582616
test('fromNextToBlock', function () {
@@ -617,14 +651,12 @@ suite('Navigation', function () {
617651
assert.isNull(prevNode);
618652
});
619653
test('fromFieldToInput', function () {
620-
const outputBlock = this.workspace.newBlock('field_input');
654+
const outputBlock = createRenderedBlock(this.workspace, 'field_input');
621655
this.blocks.fieldAndInputs2.inputList[0].connection.connect(
622656
outputBlock.outputConnection,
623657
);
624658

625659
const field = this.blocks.fieldAndInputs2.inputList[1].fieldRow[0];
626-
const inputConnection =
627-
this.blocks.fieldAndInputs2.inputList[0].connection;
628660
const prevNode = this.navigator.getPreviousSibling(field);
629661
assert.equal(prevNode, outputBlock);
630662
});
@@ -701,18 +733,13 @@ suite('Navigation', function () {
701733
});
702734

703735
suite('In', function () {
704-
setup(function () {
705-
this.emptyWorkspace = Blockly.inject(document.createElement('div'), {});
706-
});
707-
teardown(function () {
708-
workspaceTeardown.call(this, this.emptyWorkspace);
709-
});
710-
711736
test('fromInputToOutput', function () {
737+
// The first child is the connected block since the connection itself
738+
// cannot be navigated to directly.
712739
const input = this.blocks.statementInput1.inputList[0];
713740
const inNode = this.navigator.getFirstChild(input.connection);
714-
const outputConnection = this.blocks.fieldWithOutput.outputConnection;
715-
assert.equal(inNode, outputConnection);
741+
const connectedBlock = this.blocks.fieldWithOutput;
742+
assert.equal(inNode, connectedBlock);
716743
});
717744
test('fromInputToNull', function () {
718745
const input = this.blocks.statementInput2.inputList[0];

‎tests/mocha/test_helpers/block_definitions.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -196,8 +196,8 @@ export function createTestBlock() {
196196
};
197197
}
198198

199-
export function createRenderedBlock(workspaceSvg, type) {
200-
const block = workspaceSvg.newBlock(type);
199+
export function createRenderedBlock(workspaceSvg, type, opt_id) {
200+
const block = workspaceSvg.newBlock(type, opt_id);
201201
block.initSvg();
202202
block.render();
203203
return block;

0 commit comments

Comments
 (0)