ComboBox listbox does not resize when last item is made invisible

[pixelzoom]

Over in phetsims/joist#608, option excludeInvisibleChildrenFromBounds was added to LayoutBox, and the default was set to true. This makes LayoutBox effectively resize when the visibility of children change.

The first usage of this was in ComboBox, where the listbox changes size. Apparently this was not tested for the case where the list item is the selected item. In that case, the listbox does not resize. An example occurs in ph-scale. Steps to reproduce and screenshot below.

Steps:

1. start ph-scale in Studio
2. go to Macro screen, note that 'Water' is the default solute.
3. Pop up the Solute listbox
4. Set visibleProperty to false for phScale.macroScreen.view.soluteComboBox.listBox.water.visibleProperty.
5. Open the Solute listbox. Note that the listbox has not resized, and there is still a gap where 'Water' would be.

Assigning to @samreid and @jonathanolson, since they worked on this feature. High priority, since this impacts all sims with a ComboBox, including sims that are about to be published (e.g. ph-scale, ph-scale-basics).

[samreid]

It would be best for @jonathanolson to answer these questions, unassigning for now. Please reassign me if I should assist.

[pixelzoom]

This is not related to the selected item, it's related to the last item in the list.

1. start ph-scale in Studio
2. go to Macro screen
3. set Solute combo box to 'Blood'
4. Pop up the Solute listbox
5. Set visibleProperty to false for phScale.macroScreen.view.soluteComboBox.listBox.blood.visibleProperty. Note that 'Blood' disappears from the list and the listbox resizes. 'Blood' is still the selected item, which may or may not be problematic.
6. Set visibleProperty to false for phScale.macroScreen.view.soluteComboBox.listBox.water.visibleProperty. Note that the listbox does not resize, and there is a gap where 'Water' would be. Open and close the listbox to verify that the gap persists.

[pixelzoom]

If the combo selection is changed in either of the above 2 scenarios, then the listbox resizes to eliminate the gap where 'Water' was.

[jonathanolson]

I believe the above fix results in the correct behavior happening here. Testing locally, there isn't a gap anymore:

Sorry about the regression! Can you verify?

[pixelzoom]

The above fix looks good, verified in ph-scale.

But to clarify... There are 2 problems to address here:
(1) The listbox should resize if any item in the list is made invisible.
(2) Is it a problem when the selected item is made invisible in the list?

(1) is fixed.

Should we do anything about (2)? Adding @samreid and @arouinfar for their opinion.

To summarize, the remaining question is whether it's OK that the selected item does not appear in the list. It seems very odd to me, but I don't have a proposal for addressing it. And it doesn't seem to cause a problem.

[pixelzoom]

Also adding @jessegreenberg for an a11y perspective, in case there's an issue that I'm not aware of. Please see summary in previous comment.

[samreid]

It may be acceptable if this happens during customization in studio. However, what if a client develops a wrapper that removes a UI component that has focus or is being used?

[pixelzoom]

Given that iO provides access to potentially any Node, I would hope that scenery's input system would interrupt any interaction that is in progress when the target Node becomes invisible. @jonathanolson is that the case?

[jessegreenberg]

Is it a problem when the selected item is made invisible in the list?

It seems problematic to me as well. I think it would be best to not allow the selected item to be removed from the list (if possible). But if this behavior sticks around we will want to support it with keyboard focus. ComboBoxListBox puts focus on the selected item when opened, which cannot happen if the item isn't in the list.

[pixelzoom]

Hmmm... This is going to be tricky. The phetioReadOnly metadata of an iO element's visibleProperty cannot be conditionally controlled. So we can't prevent changing a list item's visibleProperty when it's the selected item, and allow it at other times.

[jonathanolson]

Given that iO provides access to potentially any Node, I would hope that scenery's input system would interrupt any interaction that is in progress when the target Node becomes invisible. @jonathanolson is that the case?

Currently no visibility listeners are added, although this would be possible to implement (interaction with a target node temporarily adds a listener so that if visibility is toggled to false it interrupts). I could imagine cases where we could be doing this unintentionally, but that's unlikely and could be worked around.

Thoughts on adding that feature to PressListener?

[arouinfar]

To summarize, the remaining question is whether it's OK that the selected item does not appear in the list. It seems very odd to me, but I don't have a proposal for addressing it. And it doesn't seem to cause a problem.

It also seems a bit odd to me, but I think it is acceptable since it doesn't break the sim or cause it to crash. PhET-iO is not foolproof, and there are myriad ways that clients can screw things up when customizing a sim. Our stance so far has been to put the burden on the client, and I don't really see how this case is any different.

[pixelzoom]

I've moved the remaining question to #591.

Closing this issue.