phetsims / sun

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

Combo box listbox can become detached from button #812

Open pixelzoom opened 1 year ago

pixelzoom commented 1 year ago

If a combo box moves because of layout changes, its listbox can become detaches from the combo box button.

For example, here's the Collision Counter in Gas Properties:

screenshot_363

Change the local a few times with the listbox popped up, and the listbox becomes detached:

screenshot_362

marlitas commented 10 months ago

I'm wondering if this got fixed in https://github.com/phetsims/joist/issues/939. Self assigning to explore and test a bit.

marlitas commented 10 months ago

I'm not seeing it detach anymore, but I did see this:

image
marlitas commented 10 months ago

This seems to be happening because of the highlightRectangle in ComboBoxListItemNode. The highlightRectangle is causing layout to behave weirdly in certain situations.

image

I confirmed this by removing the highlight rectangle and seeing that the bug no longer occurs. Further investigation incoming...

marlitas commented 10 months ago

This is only happening when the align option in comboBox is set to right.

marlitas commented 10 months ago

It seems like there is a layout update that is overwriting the work being done in:

 const updateItemLayout = () => {
      if ( options.align === 'left' ) {
        itemNodeWrapper.left = highlightRectangle.left + options.xMargin;
      }
      else if ( options.align === 'right' ) {
        itemNodeWrapper.right = highlightRectangle.right - options.xMargin;
      }
      else {
        itemNodeWrapper.centerX = highlightRectangle.centerX;
      }
      itemNodeWrapper.centerY = highlightRectangle.centerY;
    };

By mutating the options to be align: 'left' before the super call I am able to fix the bug.

I see no layout constraints when I try to follow the class hierarchy. I was also not able to find a layoutConstraint for the activeParent. This is the extent of my ability to explore this issue on my own and the below patch is just a hacky workaround. This bug will not affect preferences work that led me here, so it is not a priority to continue working on this. Removing my assignment and reassigning to @jonathanolson in case he wants to keep this on his radar.

``` Subject: [PATCH] Update TODO issue, see: https://github.com/phetsims/dot/issues/96 --- Index: js/ComboBoxListItemNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ComboBoxListItemNode.ts b/js/ComboBoxListItemNode.ts --- a/js/ComboBoxListItemNode.ts (revision ba1de4ec6995cb9bbd5c52de45a5bd671fb9f879) +++ b/js/ComboBoxListItemNode.ts (date 1698710478219) @@ -145,6 +145,7 @@ options.children = [ highlightRectangle, itemNodeWrapper ]; + options.align = 'left'; super( options ); this._supplyOpenResponseOnNextFocus = false; ```
marlitas commented 1 month ago

Apparently I never reassigned this to @jonathanolson in case he wants to take a look.

jonathanolson commented 1 month ago

I'd like to get this figured out, added as a goal on Monday.