Skip to content

Conversation

@gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Dec 3, 2025

The basics

The details

Resolves

Fixes #9489

Proposed Changes

This PR updates the aria-roledescription to specify the kind of block, rather than simply "block", with the kind specified in the label. This results in less redundant output and uses hopefully clearer names for the different block types. I also refactored the ARIA label generation code since it had become an unmaintainable mess of string manipulation.

@gonfunko gonfunko requested a review from maribethb December 3, 2025 21:18
@github-actions github-actions bot added the PR: feature Adds a feature label Dec 3, 2025
Copy link
Contributor

@maribethb maribethb left a comment

Choose a reason for hiding this comment

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

Thanks, this refactor is great

* @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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature Adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants