Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic layout problem with AquaRadioButtonGroup #832

Closed
pixelzoom opened this issue Mar 3, 2023 · 7 comments
Closed

Dynamic layout problem with AquaRadioButtonGroup #832

pixelzoom opened this issue Mar 3, 2023 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 3, 2023

I encountered this while working on phetsims/reactants-products-and-leftovers#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
Copy link
Contributor Author

pixelzoom commented Mar 3, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

@pixelzoom pixelzoom removed their assignment Mar 8, 2023
@jonathanolson
Copy link
Contributor

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:

sun/js/AquaRadioButton.ts

Lines 133 to 137 in 3fb63c5

// Add an invisible Node to make sure the layout for selected vs deselected is the same
const background = new Rectangle( selectedNode.bounds.union( deselectedNode.bounds ) );
selectedNode.pickable = deselectedNode.pickable = false; // the background rectangle suffices
this.addChild( background );

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
Copy link
Contributor

I added an issue to track this, #834

@jonathanolson
Copy link
Contributor

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

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 14, 2023

Working great in RPAL, thanks! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants