phetsims / sun

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

Closing a ComboBoxListBox should restore focus to ComboBoxButton before changing Property value. #721

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

Over in https://github.com/phetsims/fourier-making-waves/issues/194#, I ran into a case where making a ComboBox selection results in a Dialog opening (to alert the user to an invalid combination of UI settings). When closing that Dialog, I discovered that focus was not restored to the ComboBoxButton. @jessegreenberg pointed out that the focus had actually been on a ComboBoxListItemNode in the ComboBoxListBox, and we couldn't restore focus to it since the listbox was popped down.

I noted that selecting from a ComboBoxListBox typically restores focus to the ComboBoxButton, and wondered why that wasn't happening here. We discovered that it is due to this bit of code in ComboBoxListBox:

    // Pops down the list box and sets the property.value to match the chosen item.
    const fireAction = new Action( event => {
...
      // set value based on which item was chosen in the list box
      property.value = listItemNode.item.value;
...
      focusButtonCallback();
    } , 
...

My dialog opens in response to the Property value being changed. Because the Property is changed before restoring focus to the ComboBoxButton, the dialog cannot restore focus when it's closed. The solution is to change the order of these 2 operations.

pixelzoom commented 3 years ago

Fixed in the above commits, in master and fourier-making-waves-1.0 branches.

@jessegreenberg would you please review?

jessegreenberg commented 3 years ago

I think this change makes sense and fixes the problem found in https://github.com/phetsims/fourier-making-waves/issues/194. I tested a number of other ComboBoxes just in case but I can't think of anything problematic with this change.