search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
http://scenerystack.org/
MIT License
5 stars 12 forks source link

RectangularRadioButtonGroup is setting default accessible names that cannot be overridden. #904

Closed pixelzoom closed 4 weeks ago

pixelzoom commented 1 month ago

Related to https://github.com/phetsims/sun/issues/899 and https://github.com/phetsims/scenery-phet/issues/876. This is blocking gor https://github.com/phetsims/models-of-the-hydrogen-atom/issues/67.

@samreid @zepumph I seem to recall an issue (density? buoyancy?) where you proposed deriving default accessible names from tandem names. Did that get pushed? Did you make it overridable?

Here's more information....

In https://github.com/phetsims/sun/commit/15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9, @samreid added this to RectangularRadioButtonGroup:

assert && assert( _.every( items, item => !item.node.hasPDOMContent ),
      'Accessibility is provided by RectangularRadioButton and RectangularRadioButtonItem.labelContent. ' +
      'Additional PDOM content in the provided Node could break accessibility.' );

... which seems to sugges that I can't set accessibleName or helpText for radio button items.

Additionally, I'm running into a problem in MOTHA where I in fact cannot set accessibleName for the buttons in a RectangularRadioButtonGroup. And the result is a subtle problem, where I appear to be getting default accessible names that are derived from the tandem names. For ModelRadioButtonGroup, here's what I see in the A11y View:

screenshot_3534

My clue that the accessible names are coming from tandem names is "De Broglie". My string Property value is "de Broglie". The tandem name for the radio button is deBroglieRadioButton. So I suspect something is taking deBroglieRadioButton, stripping off the "RadioButton", and converting the first letter to uppercase.

I do not see this problem with AquaRadioButtonGroup, so there is inconsistent behavior that appears to have been added to RectangularRadioButtonGroup. This was very confusing to the MOTHA team.

samreid commented 1 month ago

Looks like the usage sites (2 of them) call Tandem.toAccessibleName and one is in RectangularRadioButton.ts

pixelzoom commented 1 month ago

... Did you make it overridable?

@samreid You added this to RectangularRadioButton:

      accessibleName: Tandem.toAccessibleName( providedOptions, 'RadioButton' ),

... while RectangularRadioButtonGroupItem, which is used to instantiate RectangularRadioButton instances, expects to use labelContent for the accessible name:

export type RectangularRadioButtonGroupItem<T> = {
  ...
  labelContent?: PDOMValueType; // optional label for a11y (description and voicing)
  ...
} & GroupItemOptions;

So the answer to my questions is "no", you did not provide a way to override the default (tandem derived) accessible name for RectangularRadioButtonGroup.

Also pointing out that by deriving accessible name from tandem name, you are ensuring that the accessible name is not translatable.

So I think we should delete this change from RectangularRadioButton, and delete Tandem.toAccessibleName to prevent the creation of more accessible names that are not translatable.

samreid commented 1 month ago

Reverted RectangularRadioButton usage in https://github.com/phetsims/sun/commit/1e2dd5869880fd41672adeeba92aa300f40b410e

Let's check with @zepumph before deleting Tandem.toAccessibleName

pixelzoom commented 1 month ago

@samreid said:

Reverted RectangularRadioButton usage in https://github.com/phetsims/sun/commit/1e2dd5869880fd41672adeeba92aa300f40b410e

And did you investigate whether sims were relying on this behavior? For example, are you now missing accessible names in the Density suite?

Let's check with @zepumph before deleting Tandem.toAccessibleName

My 2 cents is that this is no different than suggesting that the default label for a checkbox should be derived from its tandem name. It's never an appropriate default for a localized application.

zepumph commented 1 month ago

Sounds good. @pixelzoom please note your concerns in https://github.com/phetsims/scenery/issues/1526 and feel free to close that issue. We thought it could be a nice way to provide a first pass, but especially with PhET's push towards translating these, it doesn't seem too valuable since we can't bring the values to production.

pixelzoom commented 1 month ago

@pixelzoom please note your concerns in https://github.com/phetsims/scenery/issues/1526 and feel free to close that issue.

Noted in https://github.com/phetsims/scenery/issues/1526. But why would I close that issue?

No response to my question about whether sims may be relying on the code that @samreid removed from RectangularRadioButton. So back to you again to (maybe) investigate and close.

samreid commented 1 month ago

It seems we need a way to override the labelContent/innerContent/accessibleName for the Rectangular Radio Button. Once we have a way to set that, it is not as problematic to have a default from the tandem. I'm not worried about removing this default from Buoyancy and that suite, we should plan that the next release 1.3 has more thorough and i18nizable a11y text.

pixelzoom commented 1 month ago

Description options (like accessibleName) can be provided via RectangularRadioButtonItems.options. There is no need for labelContent/innerContent/accessibleName to be added to type RectangularRadioButtonItems. Sims should be using accessibleName. Sims should not be using labelContent/innerContent and other APIs identified as "low-level" in ParallelDOM.ts. Where accessibleName is not supported, developers should identify those cases and create GitHub issues to add support, rather than resorting to workarounds that build technical debt. But first there needs to be general consensus/understanding/education about low-level vs high-level API, when it's appropriate to use low vs high, etc.

See https://github.com/phetsims/sun/issues/901 for the path forward on accessibleName, helpText, etc.

See https://github.com/phetsims/sun/issues/899 for the problems with the description API for radio button groups specifically.

pixelzoom commented 1 month ago

@samreid said:

I'm not worried about removing this default from Buoyancy and that suite, we should plan that the next release 1.3 has more thorough and i18nizable a11y text.

Please reread my question:

And did you investigate whether sims were relying on this behavior? For example, are you now missing accessible names in the Density suite?

While you made this change for Density, other sims may now be relying on it. Are there other sims that may be relying on the behavior you removed? Have you even checked?

Re regressions in Density suite... It seems like you'd want to keep it in good shape, or at least document problems you're introducing and not addressing. You spent the time to add accessibleNames, and it has only recently been published. But I'll leave that decision to the responsible devs.

And could we please wrap this up rather than back-and-forth assigning?

pixelzoom commented 1 month ago

I asked a couple of times (and have not received an answer):

Are there other sims that may be relying on the behavior you removed? Have you even checked?

So I decided to investigate...

Regression in density-and-buoyancy-common BlocksModeRadioButtonGroup.

screenshot_3539

In Density 1.2.1 A11y View:

screenshot_3538

In Density main A11y View:

screenshot_3537

Regression in density-and-buoyancy-common BuoyancyApplicationsScreenView:

screenshot_3540

In Buoyancy 1.2.1 A11y View:

screenshot_3541

In Buoyancy main A11y View:

screenshot_3542

Regression in Mean Share and Balance, FairShareNotepadNode. @marlitas FYI.

screenshot_3545

In MSaB 1.1 A11y View:

screenshot_3544

In MSaB main A11y View:

screenshot_3543

Multiple regressions in Color Vision. @marlitas FYI.

1.3.0:

screenshot_3547

main:

screenshot_3548
marlitas commented 1 month ago

I created github issues for MSaB and Color Vision:

samreid commented 1 month ago

I added one for https://github.com/phetsims/density-buoyancy-common/issues/423

samreid commented 1 month ago

I'm very confused about this issue. The claim in the title and top comment is that the default from the tandem could not be overridden. However, following the instructions in https://github.com/phetsims/sun/issues/904#issuecomment-2429456215 I saw that we can have the default from the tandem and override it like so:

```diff Subject: [PATCH] Pass repo to check, see https://github.com/phetsims/perennial/issues/364 --- Index: sun/js/buttons/RectangularRadioButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/RectangularRadioButton.ts b/sun/js/buttons/RectangularRadioButton.ts --- a/sun/js/buttons/RectangularRadioButton.ts (revision 1e2dd5869880fd41672adeeba92aa300f40b410e) +++ b/sun/js/buttons/RectangularRadioButton.ts (date 1729691650307) @@ -94,6 +94,7 @@ containerTagName: 'li', appendDescription: true, appendLabel: true, + accessibleName: Tandem.toAccessibleName( providedOptions, 'RadioButton' ), // phet-io tandem: Tandem.REQUIRED, Index: density-buoyancy-common/js/common/view/BlocksModeRadioButtonGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/common/view/BlocksModeRadioButtonGroup.ts b/density-buoyancy-common/js/common/view/BlocksModeRadioButtonGroup.ts --- a/density-buoyancy-common/js/common/view/BlocksModeRadioButtonGroup.ts (revision 18298ece882d0a8d7dbd3a4bbabf40982124b0f4) +++ b/density-buoyancy-common/js/common/view/BlocksModeRadioButtonGroup.ts (date 1729691451139) @@ -43,7 +43,10 @@ super( modeProperty, [ { value: TwoBlockMode.ONE_BLOCK, createNode: () => BlocksModeRadioButtonGroup.getSingleCuboidIcon(), - tandemName: 'oneBlockRadioButton' + tandemName: 'oneBlockRadioButton', + options: { + accessibleName: 'TESTING' + } }, { value: TwoBlockMode.TWO_BLOCKS, createNode: () => BlocksModeRadioButtonGroup.getDoubleCuboidIcon(), ```
image

I am not involved with the subteam that is working on making the a11y text i18nizable, so I don't know the timeline, strategy, etc or whether we want any support like https://github.com/phetsims/scenery/issues/1526 based on tandems or based on i18nizable labels.

Thanks to @pixelzoom for identifying cases where this tandem default was being used, and disruptions caused by the revert.

And could we please wrap this up rather than back-and-forth assigning?

The parts I still need help on are:

  1. Should I restore the reverted commit (for now)? There are likely to be other affected situations.
  2. It seems https://github.com/phetsims/scenery/issues/1526 may give us a way forward to reuse translatable text.
  3. I don't really follow https://github.com/phetsims/scenery/issues/1526#issuecomment-2200743380 about how some things are overridable and some are not.

I cannot easily tell from Monday which subteam is working on translatable a11y text, so I think the best thing for now is to self-unassign and check in at dev meeting tomorrow. I'll add it as an agenda item, but also happy to discuss on zoom beforehand.

samreid commented 1 month ago

@zepumph @pixelzoom and I checked in about this. We agreed:

  1. We will leave the revert in place.
  2. We will PSA at dev meeting to make sure devs are aware in case of regressions.
  3. We wrote a patch in https://github.com/phetsims/scenery/issues/1526#issuecomment-2432602989 that can be investigated when the time is right.
samreid commented 4 weeks ago

In dev meeting today, @pixelzoom agreed to move forward with the tandem names shown as in the lower picture in https://github.com/phetsims/scenery/issues/1526#issuecomment-1944166933

pixelzoom commented 4 weeks ago

To clarify... https://github.com/phetsims/sun/issues/904#issuecomment-2435966643 was not the final consensus. Consensus was that default accessibleName should be base on a translatable string Property, e.g. by discovery in a Node's content. In the meantime, we decided to

pixelzoom commented 4 weeks ago

I completed the work identified in https://github.com/phetsims/sun/issues/904#issuecomment-2436018783, so I believe this issue can be closed.