ComboBoxListBox scaling can be buggy with long strings

[zepumph]

This is the general issue from initial investigation over in phetsims/molarity#152.

@twant and I found that when combobox item lengths cause scaling to occur in the combobox button, the Combobox.scaleListBox() function doesn't work 100% of the time. It may be easiest to describe this in terms of examples:

---

Molarity

When using stringTest=long, the listbox would be scaled wrong and as a result the moveListBox function wouldn't work either (it is called right afterwards). See phetsims/molarity#152 (comment)

In this case, we were able to fix it by removing the maxWidth on the listBox parent. We still don't know the underlying cause. Another way that we could solve this, was by adding a maxWidth to each Text passed to a ComboBoxItem. We elected against that though.

---

States of Matter

The ComboBox in states of matter isn't effectedby this bug. We thought that it was because the ComboBoxItem Text has a maxWidth (a solution for Molarity), but alas changing that doesn't matter, even when we set a maxWidth to the ComboBox.

---

Beers Law Lab

In this sim, we also get the bug (try stringTest=long), but this time, removing the maxWidth from the listBox parent did not solve things.

---

@twant also noted that in dev versions before the major ComboBox overhaul last winter, this bug was not present. Assigning to @pixelzoom to see if he has any ideas or wants to collaborate on this.

[zepumph]

Above is the workaround commit that worked for molarity, but not for beers-law-lab.

[pixelzoom]

I'll have a look, but can't get to this until next week.

That workaround in phetsims/molarity@244e74e really doesn't look like something that should be committed to the production version of ComboBox.

[pixelzoom]

OK, now that I'm not trying to look at this on my phone, I see that phetsims/molarity@244e74e is specific to MolarityScreenView, so won't affect other sims.

[pixelzoom]

After backing out phetsims/molarity@244e74e, I can reproduce in Molarity with stringTest=long. Interesting that if you open the listbox 4 times, the problem corrects itself. For the first 3, the listbox is in the wrong position and scaled incorrectly. For >=4 (tested up to 20),the listbox is positioned and scaled correctly.

[pixelzoom]

In Beer's Law Lab with stringTest=long, I see similar behavior. But there's an additional problem that was not reported by @zepumph above. In the Concentration screen, clicking on the ComboBoxButton does not open the listbox until the 3rd time you click it. As in Molarity, the listbox is in the wrong position and scaled incorrectly. On >= 4 clicks, the listbox is scaled and positioned correctly (also as in Molarity).

In the Beer's Law screen (either immediately after starting the sim, or after testing Concentration screen), on the 1st click the listbox opens in the wrong position and is scaled incorrectly. On >= 2nd click, the position and scaling are correct.

[pixelzoom]

About BLL, @zepumph said:

In this sim, we also get the bug (try stringTest=long), but this time, removing the maxWidth from the listBox parent did not solve things.

I suspect that you many have been removing it from the listbox parent for the wrong screen. There are 2 screens in this sim, each with its own listbox parent. If I remove maxWidth for the listbox parent in both screens, the problem is resolved for both screens (pushed in above commit).

[pixelzoom]

Same problem observed in pH Scale, which has 2 listbox parents in 2 screens. Same fix works here, see before and after below. Committed above.

Before - incorrect position and scale

After - correct position and scale

[pixelzoom]

So here's what's going on...

First of all, the listbox parent shouldn't be setting maxWidth, because scaling and position the listbox is the responsibility of the ComboBox. It scales the listbox to match the button, and positions the listbox relative to the button. So setting maxWidth on the listbox parent was (in retrospect) inappropriate (and probably a workaround).

When a maxWidth is specified for the listbox parent, we have 2 things applying transforms to the listbox. The listbox parent and the ComboBox are both scaling the listbox, and that also (indirectly) affects position. So we get the funky scaling and positioning behavior that has been observed.

I don't know why this problem sorts itself out after the listbox is opened N+ times. And I suspect that this is a new problem because of the total rewrite of ComboBox and its subcomponents. The listbox is scaled in ComboBox.scaleListBox, and I don't see anything obviously wrong there. localToGlobalBounds is being used for both the button and the listbox. What I suspect is happening is that the listbox parent is applying a transform after scaleListBox is called. And we have no way to change that.

What I recommend is to examine listbox parents in other sims, identify the ones that are setting maxWidth, and remove maxWidth. I've done that for BLL and pH Scale above. Unclear whether this needs to be patched in release branches, and republished -- that's a question for @ariel-phet.

[pixelzoom]

Regular expression new .*ComboBox\( identifies all instantiations of ComboBox and its subclasses, assuming that the subclass is named with a "ComboBox" suffix. I inspected all of these, and did not identify any additional cases where maxWidth is set for the listbox parent. So this problem appears to be isolated to molarity, beers-law-lab, and ph-scale (all sims that I wrote).

[pixelzoom]

Molarity is already tracked in phetsims/molarity#152.
BLL is now being tracked in phetsims/beers-law-lab#242.
pH scale is now being tracks in phetsims/ph-scale#93.

We can decide what to do with these when the sims are next republished.

Unassigning @ariel-phet.

[pixelzoom]

@zepumph anything else to do here?

[pixelzoom]

Btw... I did investigate whether ComboBox scaleListBox could be changed so that it works correctly if the listbox parent is scaled. I thought I might be using the wrong coordinates transform, so tried replacing localToGlobal with parentToGlobal. The result was even worse. Each time the listbox is opened, it is scaled down more. After 4 clicks, the Molarity combo box looks like this:

[zepumph]

@pixelzoom do you think that an asssertion in ComboBox could be warranted and helpful?

// added for https://github.com/phetsims/sun/issues/542
assert && assert( listParent.maxWidth === null, 'listBox scaling handles i18n. Setting maxWidth is buggy' );

[pixelzoom]

That would be fine, and would catch other cases that I may have missed via manual inspection. But note that there are other ways in which the listbox parent might be scaled that could cause similar buggy behavior. And since this problem is not specific to i18n, I'd make the assertion message something like:

'ComboBox is responsible for scaling listBox. Setting maxWidth for listParent may result in buggy behavior.'

I would have added the assertion now, but I can't stick around to monitor CT. If you want to add it, go for it. Otherwise I'll add it sometime late next week.

[pixelzoom]

I've added the recommended assertion in the above commit, and I'll monitor CT.

[pixelzoom]

Looks OK in CT.

@zepumph anything else here? Feel free to close.