phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Combo box size in preferences menu doesn't change dynamically #939

Closed Nancy-Salpepi closed 8 months ago

Nancy-Salpepi commented 8 months ago

Test device MacBook Air M1 chip

Operating System 14.0

Browser Safari/Chrome

Problem description While testing https://github.com/phetsims/qa/issues/993, I noticed that if a combo box is open in the preferences menu, it doesn't resize if I adjust my window size. If I adjust the window size first and then open the combo box, it is the correct size.

This doesn't occur with combo boxes in the sim.

Steps to reproduce

  1. In Center and Variability, go to the Localization tab of the Preferences menu and open the Region and Culture combo box.
  2. While open, make the window size smaller --the combo box size doesn't adjust
  3. Close the combo box and then reopen--it is the correct size

Visuals

Screenshot 2023-10-16 at 10 58 19 AM Screenshot 2023-10-16 at 11 13 24 AM
Nancy-Salpepi commented 8 months ago

I also looked at the Voices combo box in Quadrilateral on main and saw these oddities:

Screenshot 2023-10-16 at 11 16 08 AM Screenshot 2023-10-16 at 11 58 00 AM Screenshot 2023-10-16 at 11 58 24 AM
marlitas commented 8 months ago

Doing some investigation to try and figure out when this was introduced, but marked as blocking publication for now.

jessegreenberg commented 8 months ago

Ah, nice find @Nancy-Salpepi, thanks. This has probably been an issue since Preferences was created. There isn't very good support for ComboBox in Dialogs, and I think this is happening because of code like this:

      // phet-io - for when creating the Archetype for the Capsule housing the preferencesDialog, we don't have a sim global.
      // TODO: topLayer should be private, see https://github.com/phetsims/joist/issues/841
      const parent = phet.joist.sim.topLayer || new Node();

      voiceComboBox = new VoiceComboBox( audioModel.voiceProperty, voiceList, parent );
      voiceOptionsContent.addChild( voiceComboBox );

It isn't clear to me how to fix this or if it should be a priority, we should check with a coordinator to see how much time this is worth. Perhaps there is a workaround in ComboBox, but I think the long term solution is a "Pane" system (larger project) described at the end of https://github.com/phetsims/joist/issues/841.

marlitas commented 8 months ago

This does not sound like it is worth delaying center and variability for. I'll ask about it in the planning channel in general though.

samreid commented 8 months ago

@marlitas asked if @jessegreenberg and the team should work on this in the near future. It is unclear how to solve, maybe the long term solution would require a better pane system. @marlitas will talk to @kathy-phet about it. @jessegreenberg adds that it may be problematic during resize or relayout...

@samreid asks: Can this be solved easily with a ManualConstraint? @marlitas and @jonathanolson say the problem is more about the z-ordering.

Maybe we can try ManualConstraint instead of this code and see how it goes: https://github.com/phetsims/sun/blob/30e8b596d89c535af71b6087472164a471ab7c26/js/ComboBox.ts#L326-L341

samreid commented 8 months ago

@marlitas will take a look as part of the region and culture work. Thanks!

marlitas commented 8 months ago

I was able to get the listBox to resize appropriately by simply calling scaleListBox in the existing Multilink.

Subject: [PATCH] scale listBox
---
Index: js/ComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/ComboBox.ts b/js/ComboBox.ts
--- a/js/ComboBox.ts    (revision ec36af93832df90d5dc73e82eeb6059955011bdb)
+++ b/js/ComboBox.ts    (date 1698445092122)
@@ -337,6 +337,8 @@
           else {
             this.listBox.leftTop = matrix.timesVector2( this.button.leftBottom );
           }
+
+          this.scaleListBox();
         }
       } );

This solved most of the window resizing. Using ManualConstraint was complicated because I didn't have easy access to a common parent of the elements. In fact the common element for a comboBox in a preferences panel is phet.jois.sim.topLayer which seems like a less than ideal dependency to put into comboBox.

I am still able to get some funky listBox behavior when rapidly snapping the window size back and forth, but this seems like a much smaller and less noticeable bug. Happy to dig in more with another developer but this was about as far as I could go on my own.

marlitas commented 8 months ago

This above commit was reviewed synchronously with @jonathanolson. This issue is now ready to close.

marlitas commented 8 months ago

Oops. @Nancy-Salpepi should confirm that the fixes are good first.

@Nancy-Salpepi feel free to close again if all looks well.

Nancy-Salpepi commented 8 months ago

This looks good to me on main with mac + safari/chrome.