Skip to content
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

Add a11y-related assertions for "items" #771

Closed
3 tasks done
pixelzoom opened this issue Jun 27, 2022 · 6 comments
Closed
3 tasks done

Add a11y-related assertions for "items" #771

pixelzoom opened this issue Jun 27, 2022 · 6 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 27, 2022

In #768 (comment), I identified this assertion in ComboBoxItem, which was subsequently relocated to ComboBox:

assert && assert( !node.hasPDOMContent, 'pdomContent is set by ComboBox, use options.a11yLabel' );

In #768 (comment), @zepumph said:

@jessegreenberg and I just discussed this a bit more, and we feel totally fine about using types instead of classes. We do like that assertion though, and wished that it was in other item-like spots, for example over in

for ( i = 0; i < items.length; i++ ) {
const item = items[ i ];

We think we should add that check where other a11y items objects are turned into the item classes that the view components use.

So assertions related to "items" should likely be added to the follow:

  • AquaRadioButtonGroup
  • RectangularRadioButtonGroup
  • VerticalCheckboxGroup
@jessegreenberg
Copy link
Contributor

Assertions added, they caught one case of improper markup in greenhouse-effect where a slider was nested under a radio button. Im surprised that didn't mess with the traversal order or screen reader experience, but good to prevent.

@zepumph are you OK with this change?

@jessegreenberg jessegreenberg removed their assignment Jun 28, 2022
@zepumph
Copy link
Member

zepumph commented Jun 28, 2022

Awesome! Thanks

@zepumph zepumph closed this as completed Jun 28, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 29, 2022

Reopening.

I like the new assertion:

assert && assert( !item.node.hasPDOMContent,
         'Accessibility is provided by Checkbox and VerticalCheckboxGroupItem.options. ' +
         'Additional PDOM content in the provided Node could break accessibility.' );

Much clearer than what formerly appeared in ComboBoxItem and was moved to ComboBox:

      assert && assert( !item.node.hasPDOMContent, 'pdomContent is set by ComboBox, use options.a11yLabel' );

Can we use the same (new) assertion in ComboBox?

@pixelzoom pixelzoom reopened this Jun 29, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 29, 2022

Also reopening because this is apparently causing a sim problem. In Slack#developer, @AgustinVallejo asked:

Hey everyone! Any idea on why this assertion might have popped up after a pull (and how to fix it)? It was working yesterday
'Accessibility is provided by RectangularRadioButton and RectangularRadioButtonItem.labelContent. ' +
'Additional PDOM content in the provided Node could break accessibility.'

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jun 29, 2022

Yes, I improved the assertion message in ComboBox. The report in #771 (comment) is the assertion working as hoped and is the case we are trying to prevent. Sorry I didn't catch it first with local testing.

@jessegreenberg jessegreenberg assigned pixelzoom and unassigned zepumph Jun 29, 2022
@pixelzoom
Copy link
Contributor Author

👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants