phetsims / sun

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

RectangularRadioButtonGroup highlights need to update with dynamic locales #851

Closed jessegreenberg closed 2 months ago

jessegreenberg commented 11 months ago

Identified in https://github.com/phetsims/greenhouse-effect/issues/349, the radio button groups are not adjusting with dynamic layout. This issue is specifically for the highlights, which aren't updating because the default is overridden. It happens here, so it is likely an issue for mouse/touch areas too.

https://github.com/phetsims/sun/blob/f0b67964a1beedb065ae7f6839943eebaa11f26a/js/buttons/RectangularRadioButtonGroup.ts#L278-L304

jessegreenberg commented 11 months ago

The above is a potential fix, @jbphet can you please review? Specifically, I wasn't sure if this.boundsProperty was the right one to link to for dynamic layout or if each button needed this listener on its changing bounds.

jbphet commented 11 months ago

@jessegreenberg - I started out reviewing this by checking the functionality, and I can see that it's now better for the wider button, but the highlights on the other buttons don't look correct. See the screenshots below. I think the button group should probably update its layout in a case like this, which may make this fix workable, but as of this moment it doesn't seem complete. What's your take?

image

image

FWIW I didn't continue reviewing the code after seeing this.

jbphet commented 11 months ago

I've created a separate issue for addressing the dynamic layout needs of the radio button group, see https://github.com/phetsims/sun/issues/852.

jessegreenberg commented 11 months ago

Hmm, good catch. The same can be seen for the mouse/touch areas:

image

I am not sure how to best handle the layout. I will revert the change, and we can make sure this is fixed after #852.

jonathanolson commented 4 months ago

This looks like it was buggy because RectangularRadioButtonGroup was trying to do its own override for focus highlights. Removing this code fixed it. Can you verify?

jessegreenberg commented 4 months ago

Awesome, thats great! I tested by running greenhouse-effect with ?supportsDynamicLocale=true&locales=*&keyboardLocaleSwitcher and changed the strings a few times. All highlights looked correct, even when the button contents had different sizes like in https://github.com/phetsims/sun/issues/851#issuecomment-1686906201.

Thanks for fixing this. Closing.

jessegreenberg commented 3 months ago

Reopening, @arouinfar @terracoda and I noticed that the focus highlight no longer surrounds the the label Text of the button in the RectangularRadioButtonGroup, if it has one. Ill try to add that back.

jessegreenberg commented 3 months ago

@jonathanolson would you mind reviewing this change?

jessegreenberg commented 2 months ago

Enough time has passed so I think it is OK to close - sims have gone through QA with this change. Closing.