phetsims / sun

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

Instrument ToggleSwitch with PDOM #696

Closed jessegreenberg closed 1 year ago

jessegreenberg commented 3 years ago

The ToggleSwitch has a requested PDOM representation as

<button aria-pressed="false" role="switch" aria-checked="false">Inner Content</button>

This much is easy enough. But there is an issue with activating the ToggleSwitch because it uses DragListener and click is specifically disabled with that listener at https://github.com/phetsims/scenery/blob/5120fcc377304ca15532e2ee867c24c87448e71a/js/listeners/DragListener.js#L314-L316 because of https://github.com/phetsims/scenery/issues/903. It seems like we could either a) Optionally return false in DragListener.canClick, so that this DragListener and others like it can respond to click. b) Add a new input listener to ToggleSwitch that toggles the Property on click. c) Revisit a different solution to https://github.com/phetsims/scenery/issues/903?

I think I am leaning toward (a) because it is more general and could be helpful in the future. But the desire to use click with something that uses DragListener is also very rare, so maybe it isn't worth the option added to DragListener.

jessegreenberg commented 3 years ago

Another issue is that DragListener functionality is generally implemented with start and end options, which currently are not called with PressListener's click. To correctly respond, DragListener needs to override click and call these functions.

jessegreenberg commented 3 years ago

I did this in the above commits. @zepumph could you please review? In https://github.com/phetsims/scenery/commit/4dd8aa3ad49a9109c7578a6124968df56665bd08 I added click support to DragListener by adding an option called allowClick and overriding click to support DragListener callbacks. In https://github.com/phetsims/sun/commit/6e99b23a34650ddf7c9152ebd36de3cc332a7d37 I added PDOM to ToggleSwitch and used the new allowClick option.

zepumph commented 3 years ago

Review:

Otherwise looks good! Thanks for taking this on.

zepumph commented 2 years ago

Hey, just a ping here, this has been sitting for quite some time.

zepumph commented 2 years ago

Subset of https://github.com/phetsims/scenery/issues/1298

jessegreenberg commented 1 year ago

Would PressListener.click() return false if this.canClick() was false?

Yes. DragListener calls super.click(), which calls this.canClick() and click is overridden in the subtype. So DragListener.click() will correctly return false.

Or maybe just name it to be the same as the function canClick: true. defaults to false.

I like that and will go with that. I also considered allowPDOMClick so it is clearly about that feature. I think canClick to match the function and documentation describing the need makes sense.

jessegreenberg commented 1 year ago

OK, with the rename that was recommended and the note

Otherwise looks good!

I think this can be closed.