Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 40 additions & 35 deletions core/block_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,73 +243,78 @@ export class BlockSvg
}

private computeAriaLabel(): string {
const {commaSeparatedSummary, inputCount} = buildBlockSummary(this);
let inputSummary = '';
if (inputCount > 1) {
inputSummary = 'has inputs';
} else if (inputCount === 1) {
inputSummary = 'has input';
}

let blockTypeText = 'block';
if (this.isShadow()) {
blockTypeText = 'replaceable block';
} else if (this.outputConnection) {
blockTypeText = 'input block';
} else if (this.statementInputCount) {
blockTypeText = 'C-shaped block';
}
const labelComponents = [];

const modifiers = [];
if (!this.isEnabled()) {
modifiers.push('disabled');
}
if (this.isCollapsed()) {
modifiers.push('collapsed');
}
if (modifiers.length) {
blockTypeText = `${modifiers.join(' ')} ${blockTypeText}`;
if (this.getRootBlock() === this) {
labelComponents.push('Begin stack');
}

let prefix = '';
const parentInput = (
this.previousConnection ?? this.outputConnection
)?.targetConnection?.getParentInput();
if (parentInput && parentInput.type === inputTypes.STATEMENT) {
prefix = `Begin ${parentInput.getFieldRowLabel()}, `;
labelComponents.push(`Begin ${parentInput.getFieldRowLabel()}`);
} else if (
parentInput &&
parentInput.type === inputTypes.VALUE &&
this.getParent()?.statementInputCount
) {
prefix = `${parentInput.getFieldRowLabel()} `;
labelComponents.push(`${parentInput.getFieldRowLabel()}`);
}

if (this.getRootBlock() === this) {
prefix = 'Begin stack, ' + prefix;
const {commaSeparatedSummary, inputCount} = buildBlockSummary(this);
labelComponents.push(commaSeparatedSummary);

if (!this.isEnabled()) {
labelComponents.push('disabled');
}
if (this.isCollapsed()) {
labelComponents.push('collapsed');
}

let additionalInfo = blockTypeText;
if (inputSummary) {
additionalInfo = `${additionalInfo}, ${inputSummary}`;
if (inputCount > 1) {
labelComponents.push('has inputs');
} else if (inputCount === 1) {
labelComponents.push('has input');
}

return prefix + commaSeparatedSummary + ', ' + additionalInfo;
return labelComponents.join(', ');
}

private computeAriaRole() {
if (this.workspace.isFlyout) {
aria.setRole(this.pathObject.svgPath, aria.Role.TREEITEM);
} else {
const roleDescription = this.getAriaRoleDescription();
aria.setState(
this.pathObject.svgPath,
aria.State.ROLEDESCRIPTION,
'block',
roleDescription,
);
aria.setRole(this.pathObject.svgPath, aria.Role.FIGURE);
}
}

/**
* Returns the ARIA role description for this block.
*
* Block definitions may override this method via a mixin to customize
* their role description.
*
* @returns The ARIA roledescription for this block.
*/
protected getAriaRoleDescription() {
if (this.isShadow()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if replaceable should be added to the specific type? Like "replaceable value block"? Or should replaceable stay in the aria label and not in the role description?

cc @BenHenning for thoughts

Copy link
Collaborator

Choose a reason for hiding this comment

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

Replaceable was meant to hint at it being a shadow more than its type, so I think I agree with this. If custom block types facilitate special labels then it could still work for shadow blocks (if that ever actually makes sense).

Separately, should this be protected? I assumed we never designed Block functionality for inheritance since Blockly doesn't support custom BlockSvg class implementations. I'm wondering if this needs to be something overridable in the block definition instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great callout Maribeth, and thanks for confirming this makes sense to you as well Ben. To clarify, do we want "replaceable" in the label or the role description? I'm inclined towards the label personally, in the same part as we call out the disabled/collapsed modifiers since it seems to be the same kind of thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think the label makes sense alongside the other modifiers, but defer to Ben if he thinks differently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to mention it in the label. @BenHenning PTAL as well. Also I think block definitions are able to override protected methods via mixins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to go ahead and merge, but if you have concerns Ben please do flag/reopen.

return 'replaceable block';
} else if (this.outputConnection) {
return 'value block';
} else if (this.statementInputCount) {
return 'container block';
} else {
return 'statement block';
}
}

/**
* Create and initialize the SVG representation of the block.
* May be called more than once.
Expand Down
Loading