Tweaker buttons can be misaligned

[samreid]

Similar to #420 and https://github.com/phetsims/scenery-phet/issues/513 and noted during https://github.com/phetsims/QA/issues/405, the tweaker buttons don't always line up. They are positioned based on the height of the number control which may change based on the height of the text. Hence the "align the bottom" strategy proposed in #420 doesn't also align the tweaker buttons.

I recommend we do not try to solve this as part of the RC, but consider this problem as we move forward for future versions and sims.

[samreid]

@ariel-phet or @arouinfar can you please review the problem mentioned above and comment?

[ariel-phet]

@samreid I am fine with us not solving this as a part of RC, but it should be solved in a general way since again, this layout will occur again in CCK AC (a sim destined to have many many translations).

[samreid]

Tagging for potential work as part of https://github.com/phetsims/circuit-construction-kit-common/issues/449

UPDATE: and https://github.com/phetsims/circuit-construction-kit-common/issues/528

[samreid]

I'd like to mark this for developer meeting to understand the scope of what's requested here and to hear ideas for how to proceed. Just fixing the layout for NumberControl? For NumberControl with certain layoutFunctions? More layout flexibility in general?

[pixelzoom]

I don't understand this issue, because it doesn't identify what the desired alignment is. "the tweaker buttons don't always line up" --- with what?

[samreid]

From the picture in the issue description: the pair of tweaker buttons for the leftmost slider are vertically offset from the pair of tweaker buttons for the rightmost slider.

[pixelzoom]

Yes, I can see that they are vertically offset. That behavior is correct because an HBox with align: 'center' is used for the layout. Is that a problem? If so, when?

What position do you think is desired? You said "They are positioned based on the height of the number control which may change based on the height of the text." but that makes no sense -- they are part of the NumberControl. For example if using createLayoutFunction1 (the default), they are positioned based on the height of the slider:

            new HBox( {
              spacing: options.arrowButtonsXSpacing,
              resize: false, // prevent slider from causing a resize when thumb is at min or max
              children: [ leftArrowButton, slider, rightArrowButton ]
            } )

[jessegreenberg]

Has this since been fixed? Master looks like

EllipseSceneControlPanel aligns the NumberControls by bottom which I was going to recommend.

[samreid]

Aligning at the bottom worked nicely for the diffraction screen context, but doesn't seem fully general, and you can see the tweaker buttons are aligned but the numbers at the top are now misaligned.

[zepumph]

In keyboard help dialogs (KeyboardHelpSection.js), we ran into similar layout issues where we wanted components of different instances to align, but from an OO perspective to be separate. We ended up formulating each section very specifically, keeping track of the two columns per section to align. I think that this level of complexity is generally not desirable in NumberControl, which arguable already has too much in it. My vote, without being part of a larger discussion, is to not try to solve this in common code or fully generally, and instead (1) rely on case by case solutions like you have above, and if that doesn't work (2) create a layout function for each number control that supports their alignment. What more would we gain from added complexity to NumberControl?

[samreid]

@samreid asks would it be possible to address this by using AlignBox? @pixelzoom says he would prefer not to use AlignBox, because it leads to confusing, complicated code. @jonathanolson asks what is better? @pixelzoom says something like Java, like GridBoxLayout, SpringLayout, etc. would be better. @ariel-phet asks whether we can start with a small layout manager, then build on it? @samreid points out this problem occurs when a translated text exceeds maxWidth, and it becomes smaller vertially @ariel-phet says this is a rare condition, translations for those strings shouldn't be too long.

One problem could be solved by aligning the bottom of the tweaker buttons with the bottom of the track. Then changing the tickmark heights won't affect the tweaker button locations. We'll create a new issue for that, and see how far that takes us. @zepumph volunteered to help. But @ariel-phet requested @chrisklus to help on this. Timeline would be within a month or so.

@pixelzoom says: layout managers wouldn't have solved this problem.

@zepumph clarifies that any layout function that has tweakers on the sides should have this change.

@samreid asks: should sim-specific layout functions be updated too?

@ariel-phet says: let's start with the common code.

[zepumph]

There are only 6 custom layout functions currently; searched with: layoutFunction: [^N]

[pixelzoom]

A dev meeting conclusion in https://github.com/phetsims/wave-interference/issues/433#issuecomment-583096399 was:

One problem could be solved by aligning the bottom of the tweaker buttons with the bottom of the track.

But.... This layout occurs in Buoyancy (Explore screen):

When I mentioned this issue and our plan to bottom-align the tweakers and slider track, the design team for that sim felt that would look odd. It would also cause the control to take up more vertical space. So do we need to rethink the decision here? Or would Buoyancy simply use its own layoutFunction?

[jessegreenberg]

Discussed 3/12/20 - We discussed either adding options to the layout functions to support minor changes to the layout, require brand new layout functions if you need to have custom layouts. dding options would complicate NumberControl, but re-creating very similar layout functions doesn't seem maintainable either. We also considered going through NumberControl usages and seeing if any of the layout functions can be removed or collapsed to reduce complexity.

We decided to come back to this when @ariel-phet returns because we aren't sure how much time we should spend on resolving issues involving these layout functions vs just adding what we need to support custom cases.

[zepumph]

@ariel-phet feels like there are bigger fish to fry at the moment, and mentions that this may be needed for CCK AC. We will tackle it then.

[samreid]

One other idea that may be simple (for when we get there in the future). When reducing text width due to maxWidth constraints, scenery could keep the same height on it--I suspect that would fix many layout problems.

[samreid]

@ariel-phet can you please comment on this issue as it pertains to the upcoming publication of CCK-AC?

[ariel-phet]

I think this issue can be dealt with in the future by https://github.com/phetsims/sun/issues/48

Closing this issue as it can be taken care of by layout manager work in the future (and is not pressing currently)