phetsims / sun

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

Unable to move through items in a combobox with keyboard nav [safari only] #878

Closed Nancy-Salpepi closed 4 months ago

Nancy-Salpepi commented 4 months ago

Test device MacBook Air M1 chip

Operating System 14.3.1

Browser Safari 17.3.1

Problem description First seen while testing https://github.com/phetsims/qa/issues/1060 and ONLY with safari: I am unable move through items in a combobox with keyboard navigation. The large rectangle that usually appears around the listbox isn't there and I can't arrow through the items. I also saw this on main with Kepler's Laws and MSS. Things work fine on published.

Steps to reproduce Here is an example:

  1. On main: Go to the Lab screen of My Solar System
  2. Tab to the Combobox and press Space/Enter
  3. Try to use arrow keys to navigate through the items

Visuals It looks like this after pressing Space/Enter:

Screenshot 2024-03-20 at 3 30 58 PM

On published it looks correct:

Screenshot 2024-03-20 at 3 32 02 PM
jessegreenberg commented 4 months ago

Thanks for finding this @Nancy-Salpepi. I am able to produce this on my Mac as well. Its happening in main but not a (semi) recent dev version of greenhouse-effect (https://phet-dev.colorado.edu/html/greenhouse-effect/1.3.0-dev.4/phet/greenhouse-effect_all_phet.html) so this was likely introduced recently.

jessegreenberg commented 4 months ago

I tracked down that this was introduced by https://github.com/phetsims/scenery/issues/1606.

This blocks publication for sims that support alt input and have a ComboBox.

jessegreenberg commented 4 months ago

The previous workaround for https://github.com/phetsims/scenery/issues/1606 would force a reflow by updating display, but that also made components non-focusable for an animation frame. That is buggy for cases like this where you synchronously call

node.pdomVisible = true;
node.focus()

A new workaround forces a reflow by temporarily setting the transform instead. I confirmed that this fixes the problem in Safari. I hope that https://github.com/phetsims/scenery/issues/1606 is still fixed but I am not certain.

@Nancy-Salpepi can you please test to make sure that this issue is fixed and that https://github.com/phetsims/scenery/issues/1606 is still fixed as well?

Also, I scanned through QA issues and did not see any new RCs published after Feb 21 that support alt input. That means we shouldn't have to patch this fix into anything. @Nancy-Salpepi does that sound right to you?

Nancy-Salpepi commented 4 months ago

@Nancy-Salpepi can you please test to make sure that this issue is fixed and that https://github.com/phetsims/scenery/issues/1606 is still fixed as well?

This issue is fixed on main and the VO issue still sounds good to me!

Also, I scanned through QA issues and did not see any new RCs published after Feb 21 that support alt input. That means we shouldn't have to patch this fix into anything. @Nancy-Salpepi does that sound right to you?

Yes. That is correct.

I'm going to go ahead and close this. Thanks for getting to this issue so fast @jessegreenberg!