Improvements to popup positioning for ComboBox within HorizontalLayout. (#11846) (#11852)8.9.48.9

Expand ratio and spacing can cause ComboBox to miscalculate its own
position while layouting is still ongoing. Popup should not be
repositioned in such circumstances in order to avoid incorrect
intermediate states.

Continues on #11718

2 files changed, 138 insertions, 8 deletions

diff --git a/client/src/main/java/com/vaadin/client/ui/VComboBox.java b/client/src/main/java/com/vaadin/client/ui/VComboBox.java
index 438234d30b..c7fe9e14be 100644
--- a/client/src/main/java/com/vaadin/client/ui/VComboBox.java
+++ b/client/src/main/java/com/vaadin/client/ui/VComboBox.java
@@ -81,6 +81,7 @@ import com.vaadin.client.ui.aria.HandlesAriaRequired;
 import com.vaadin.client.ui.combobox.ComboBoxConnector;
 import com.vaadin.client.ui.menubar.MenuBar;
 import com.vaadin.client.ui.menubar.MenuItem;
+import com.vaadin.client.ui.orderedlayout.Slot;
 import com.vaadin.shared.AbstractComponentState;
 import com.vaadin.shared.ui.ComponentStateUtil;
 import com.vaadin.shared.util.SharedUtil;
@@ -899,19 +900,27 @@ public class VComboBox extends Composite implements Field, KeyDownHandler,
                 }
             }
 
-            if (offsetWidth + menuMarginBorderPaddingWidth
-                    + left < VComboBox.this.getAbsoluteLeft()
-                            + VComboBox.this.getOffsetWidth()) {
-                // Popup doesn't reach all the way to the end of the input
-                // field, filtering may have changed the popup width.
-                left = VComboBox.this.getAbsoluteLeft();
+            int comboBoxLeft = VComboBox.this.getAbsoluteLeft();
+            int comboBoxWidth = VComboBox.this.getOffsetWidth();
+            if (hasParentWithUnadjustedPositioning()) {
+                // ComboBox itself may be incorrectly positioned, don't adjust
+                // popup position yet. Width calculations must be performed
+                // anyway to avoid flickering.
+                return;
+            }
+            if (left > comboBoxLeft
+                    || offsetWidth + menuMarginBorderPaddingWidth
+                            + left < comboBoxLeft + comboBoxWidth) {
+                // Popup is positioned too far right or doesn't reach all the
+                // way to the end of the input field, filtering may have changed
+                // the popup width.
+                left = comboBoxLeft;
             }
             if (offsetWidth + menuMarginBorderPaddingWidth + left > Window
                     .getClientWidth()) {
                 // Popup doesn't fit the view, needs to be opened to the left
                 // instead.
-                left = VComboBox.this.getAbsoluteLeft()
-                        + VComboBox.this.getOffsetWidth() - offsetWidth
+                left = comboBoxLeft + comboBoxWidth - offsetWidth
                         - (int) menuMarginBorderPaddingWidth;
             }
             if (left < 0) {
@@ -924,6 +933,50 @@ public class VComboBox extends Composite implements Field, KeyDownHandler,
         }
 
         /**
+         * Checks whether there are any {@link VHorizontalLayout}s with
+         * incomplete internal position calculations among this VComboBox's
+         * parents.
+         *
+         * @return {@code true} if unadjusted parents found, {@code false}
+         *         otherwise
+         */
+        private boolean hasParentWithUnadjustedPositioning() {
+            /*
+             * If there are any VHorizontalLayouts among this VComboBox's
+             * parents, any spacing or expand ratio may cause incorrect
+             * intermediate positioning. The status of the layout's internal
+             * positioning can be checked from the first slot's margin-left
+             * style, which will be set to 0px if no spacing or expand ratio
+             * adjustments are needed, and to a negative pixel amount if they
+             * are. If the style hasn't been set at all, calculations are still
+             * underway. Popup position shouldn't be adjusted before such
+             * calculations have been finished.
+             *
+             * VVerticalLayout has the same logic but it only affects the
+             * vertical positioning, which is irrelevant for the calculations
+             * here.
+             */
+            Widget toCheck = VComboBox.this;
+            while (toCheck != null && !(toCheck.getParent() instanceof VUI)) {
+                toCheck = toCheck.getParent();
+                if (toCheck instanceof VHorizontalLayout) {
+                    VHorizontalLayout hLayout = (VHorizontalLayout) toCheck;
+                    // because hLayout is a parent it must have at least one
+                    // child widget
+                    Widget slot = hLayout.getWidget(0);
+                    if (slot instanceof Slot && slot.getElement().getStyle()
+                            .getMarginLeft().isEmpty()) {
+                        // margin hasn't been set, layout's internal positioning
+                        // is still being adjusted and ComboBox's position may
+                        // not be final
+                        return true;
+                    }
+                }
+            }
+            return false;
+        }
+
+        /**
          * Adds in-line CSS rules to the DOM according to the
          * suggestionPopupWidth field
          *
diff --git a/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxWithinHorizontalLayout.java b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxWithinHorizontalLayout.java
new file mode 100644
index 0000000000..4ae8977a8b
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxWithinHorizontalLayout.java
@@ -0,0 +1,77 @@
+package com.vaadin.tests.components.combobox;
+
+import java.util.Arrays;
+import java.util.Iterator;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Alignment;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.ComboBox;
+import com.vaadin.ui.Component;
+import com.vaadin.ui.HorizontalLayout;
+
+public class ComboBoxWithinHorizontalLayout extends AbstractTestUI {
+
+    private ComboBox<String> comboBox;
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        comboBox = new ComboBox<>();
+        comboBox.setWidth("100%");
+        comboBox.setPopupWidth(null);
+        populateWithShortItems();
+
+        HorizontalLayout content = new HorizontalLayout(new Button("1"),
+                new Button("2"), comboBox, new Button("3"));
+        content.setWidth("100%");
+        content.setExpandRatio(comboBox, 2.0f);
+        Iterator<Component> i = content.iterator();
+        while (i.hasNext()) {
+            content.setComponentAlignment(i.next(), Alignment.BOTTOM_RIGHT);
+        }
+
+        getLayout().setSpacing(true);
+        addComponent(content);
+        addComponent(new Button("Toggle items", e -> {
+            if ("Short items".equals(comboBox.getCaption())) {
+                populateWithLongItems();
+            } else {
+                populateWithShortItems();
+            }
+        }));
+        addComponent(new Button("Toggle spacing", e -> {
+            content.setSpacing(!content.isSpacing());
+        }));
+        addComponent(new Button("Toggle expand ratio", e -> {
+            if (content.getExpandRatio(comboBox) > 0.0f) {
+                content.setExpandRatio(comboBox, 0.0f);
+            } else {
+                content.setExpandRatio(comboBox, 2.0f);
+            }
+        }));
+    }
+
+    private void populateWithLongItems() {
+        comboBox.setCaption("Long items");
+        comboBox.setItems(Arrays.asList(
+                "First very, very, very, very, very long item to add",
+                "Second very long item to add", "Third very long item to add"));
+    }
+
+    private void populateWithShortItems() {
+        comboBox.setCaption("Short items");
+        comboBox.setItems(Arrays.asList("short1", "short2", "short3"));
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 11718;
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "ComboBox within HorizontalLayout should not get incorrect "
+                + "intermediate popup positions that cause flickering.";
+    }
+}