phetsims / sun

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

Dynamic layout problem with AquaRadioButtonGroup #832

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

I encountered this while working on https://github.com/phetsims/reactants-products-and-leftovers/issues/79.

In AquaRadioButtonGroup, this bit of code is causing a problem with dynamic layout:

      // Content for the radio button.
      // For vertical orientation, add an invisible strut, so that buttons have uniform width.
      const content = ( options.orientation === 'vertical' ) ?
                      new Node( { children: [ new HStrut( maxItemWidth ), node ] } ) :
                      node;

The width of the HStrut never changes, so the content can never get narrower than its initial width.

For example, in RPAL Sandwiches screen (running with ?dev&stringTest=dynamic), the radio buttons look like this initially:

screenshot_2335

The buttons are supposed to be right justified with the layoutBounds. If the strings get longer, everything is OK:

screenshot_2336

But if the strings get shorter than they were initially, they are no longer right justified:

screenshot_2334

The solution is to get rid of the HStrut, and use AlignBox.

pixelzoom commented 1 year ago

After replacing HStrut with AlignBox, I'm still seeing the problem. Running RPAL with this URL:

http://localhost:8080/reactants-products-and-leftovers/reactants-products-and-leftovers_en.html?brand=phet&ea&debugger&stringTest=dynamic&dev&showPointerAreas

Here's what the radio buttons initially look like on the Sandwiches screen:

screenshot_2337

Pressing Option left-arrow once to shorten the strings looks like this:

screenshot_2338

I don't understand this behavior. The buttons labels all share the same AlignGroup, so how could they have different bounds?

@jonathanolson could you please take a look at this? Is it possible that there's a bug in AlignBox? Or maybe I have a problem in ReactionRadioButtonGroup.

jonathanolson commented 1 year ago

I don't see why an AlignGroup is needed here with the new layout features. Checkbox is a sizable component that can resize its content (the label) up when its preferred size changes. If you just give layoutOptions: { stretch: true } in the layout container (when it's orientation:vertical), they should expand out naturally, and nothing else is needed.

For reference, the very first example in Layout Exemplars (http://localhost:8080/phet-lib/doc/layout-exemplars.html) demos this (but doesn't show the touch areas - they are there).

Happy to look into what's going on (but out of steam for the night), leaving assigned for that.

pixelzoom commented 1 year ago

I understand that layoutOptions: { stretch: true } is an alternative. But AlignBox should work also, should it not? I'm not real excited about reimplementing AquaRadioButtonGroup. And I've been experiencing weirdness elsewhere with AlignBox (sorry not to be specific...)

So yes, please investigate.

jonathanolson commented 1 year ago

Whoops, of course this doesn't use Checkbox (as I mentioned earlier).

Looks like AquaRadioButton... really doesn't support dynamic layout at all. I should probably add this.

This is what gives us all the trouble: https://github.com/phetsims/sun/blob/3fb63c55260d2915bc7da51db8e47692bfadea1d/js/AquaRadioButton.ts#L133-L137

AlignGroup/AlignBox working great. You create/add the first radio button, and it properly sizes the content. The first radio button gets a permanent minimum size due to this. When you create the second radio button, it's larger, so the AlignBoxes increase in size. Then it gets a larger permanent minimum size. When you later size things down, the AquaRadioButtons go down to the size of the content on initialization.

Leaving self-assigned to handle this.

jonathanolson commented 1 year ago

I added an issue to track this, https://github.com/phetsims/sun/issues/834

jonathanolson commented 1 year ago

I believe this should be patched in the issue, and working correctly in this case. Can you verify?

pixelzoom commented 1 year ago

Working great in RPAL, thanks! Closing.