phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Add a11y-related assertions for "items" #771

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

In https://github.com/phetsims/sun/issues/768#issuecomment-1165114123, 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 https://github.com/phetsims/sun/issues/768#issuecomment-1165800339, @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 https://github.com/phetsims/sun/blob/d2e9d3c15f8697543213dff713dbc7670b931374/js/buttons/RectangularRadioButtonGroup.ts#L264-L265

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:

jessegreenberg commented 2 years ago

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?

zepumph commented 2 years ago

Awesome! Thanks

pixelzoom commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Yes, I improved the assertion message in ComboBox. The report in https://github.com/phetsims/sun/issues/771#issuecomment-1170198314 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.

pixelzoom commented 2 years ago

👍🏻