fix: Added comma between labels for fields when composing ARIA label strings#9812
Conversation
lizschwab
changed the title
mikeharv
left a comment
mikeharv
left a comment
There was a problem hiding this comment.
Considering that this is a deliberate change, and the behavior has evidently flipped a couple times already, could we add a test?
| // AriaLabelProvider and without setting the provider (the default label) | ||
| assert.equal(labelA, labelB); | ||
| }); | ||
| test('Field labels are comma separated', function () { |
There was a problem hiding this comment.
Thanks for adding a test. Unfortunately, I think it's giving a false positive as it is currently written. In fact, it passes without your core change. This is because each of your test inputs below only have a single field. It would work better to model a single input with multiple fields, since getLabel() composes from all labels within the input.
For example, you could do:
const input = this.block.appendDummyInput().appendField('first').appendField('second');
assert.include(input.getLabel(), 'first, second');
Using label fields like this also makes it clear that multiple labels being comma-separated is intentional. If we decided to make an exception for these later, we'd want to expand this to several tests covering different combinations of field types on the same input.
| const label = this.block.getAriaLabel(); | ||
| const inputLabels = this.block.inputList.map((input) => input.getLabel()); | ||
|
|
||
| assert.include(label, inputLabels.join(', ')); |
There was a problem hiding this comment.
Using the same .join(', ') here in the test feels like it's going to give a weaker signal than explicit string comparison, since it's very similar to the way we currently implement this in core. If other parts of the implementation broke, this could continue to pass.
| .appendField(new Blockly.FieldTextInput('text'), 'NAME'); | ||
| this.block | ||
| .appendDummyInput('VALUE') | ||
| .appendField(new Blockly.FieldNumber('number'), 'VALUE'); |
There was a problem hiding this comment.
The first constructor arg in Field classes is generally going to be an optional but valid default value. FieldNumber expects a number (or string that can be cast to a number), so this value 'number' ends up equivalent to passing null.
| test('Field labels are comma separated', function () { | ||
| this.block | ||
| .appendDummyInput('NAME') | ||
| .appendField(new Blockly.FieldTextInput('text'), 'NAME'); |
There was a problem hiding this comment.
I think it's fine to omit names for fields and inputs in tests (e.g. 'NAME') if we don't need to reference them again later.
2 participants
The basics
The details
Resolves
Fixes #9791
Proposed Changes
Adds a comma between inputs when joining together the array of input labels.
Screenshot:
Reason for Changes
This change will add a comma between labels for fields when composing the ARIA label string. This signals to the screenreader that it should pause, which gives screenreader users valuable information about the composition of the block.
Test Coverage
N/A