phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

LaserPointerNode: support for `accessibleName` and `helpText` #877

Closed pixelzoom closed 3 weeks ago

pixelzoom commented 1 month ago

Related to https://github.com/phetsims/models-of-the-hydrogen-atom/issues/67 and https://github.com/phetsims/sun/issues/901 ...

LaserPointerNode currently has problems with support of ParallelDOM accessibleName and helpText options. You have to do something like this, and both options are ignored; they do not appear in the A11y View:

    const laserPointerNode = new LaserPointerNode( ... {
      ...
      buttonAccessibleName: 'This is the acessible name.'
      helpText: 'This is the help text.'
    } );

LaserPointerNode has many options related to its button. Instead of using nested options buttonOptions, there are individual options at the top-level of LaserPointerNodeOptions. The first task will be to convert this to nested options.

The second task is to fix the support for accessible name. This diff seems to fix the problem:

- labelContent: options.buttonAccessibleName,
- labelTagName: 'label',
+ accessibleName: options.buttonAccessibleName,
pixelzoom commented 1 month ago

@jessegreenberg I'd be happy to handle this one. But which of these APIs (or something else?) do you think is most appropriate?

// API 1: accessibleName and helpText are specified via nested options to LaserPointerNode's button.
const laserPointerNode = new LaserPointerNode( ..., {
  buttonOptions: {
    accessibleName: ...
    helpText: ...
  }
} );

// API 2: accessibleName and helpText are top-level options, and LaserPointerNode is responsible for propagating them to its button.
const laserPointerNode = new LaserPointerNode( ..., {
  accessibleName: ...
  helpText: ...
} );
pixelzoom commented 1 month ago

I also see LaserPointerNodeOptions has buttonDescriptionContent?: string. Can that be replaced by helpText?

pixelzoom commented 3 weeks ago

@jessegreenberg and I discussed the 2 APIs in https://github.com/phetsims/scenery-phet/issues/877#issuecomment-2436468122, and we think that API 2 might be the better general choice for PhET UI components. My certainty is ~75%.

In the meantime, I made a couple of small fixes and improvements in https://github.com/phetsims/scenery-phet/commit/1b893ec5cf34cac97a76af6bb009cbbb00a0a4f1, so that these features work in MOTHA. The only other sim affected was Rutherford Scattering, and it does not have a complete description implementation.

jessegreenberg commented 3 weeks ago

In the above commits I replaced buttonAccessibleName and buttonHelpText with the accessibleName and helpText implementation. Next Ill take a look at converting the converting the other button options to nested options as part of this issue.

jessegreenberg commented 3 weeks ago

The above commits use nested options for the button of LaserPointerNode. @pixelzoom can you please review https://github.com/phetsims/scenery-phet/commit/1c8b15fb6c0174bae8b4c4781ebea7b5d3e604ea? In particular using combineOptions and type ButtonOptions.

pixelzoom commented 3 weeks ago

This looks great, including ButtonOptions and the use of combineOptions. Thanks for handling this. Closing.