From d11356d07e25c72dd9da3e1109fb7dcc2a4271aa Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sun, 24 May 2026 16:31:44 +0300 Subject: [PATCH 1/2] Fix #5028 / #5029: InteractionDialog popup overlaps target when rect straddles vertical midline `showPopupDialogImpl` picked popup placement purely from which half of the screen the anchor rect sat in. When the rect straddled the midline, neither the "popup downwards" branch (`rect.bottom < availableHeight/2`) nor the "popup upwards" branch (`rect.y > availableHeight/2`) matched, and control fell into a "popup over aligned with top of rect" branch that set `y = rect.getY() + 3mm` -- drawing the popup ON TOP of the target. This produced two user-visible symptoms reported against the same code: - #5028: the popup covers the target Button (hiding the Close button in the reporter's screenshot) when the form is scrolled so the target lands near the vertical middle of the screen. - #5029: on the initial show the popup's arrow points UPWARDS instead of at the Close button, and only flips to the correct side once the form is scrolled. With the popup overlapping the target, `CSSBorder.Arrow` cannot pick a consistent direction (`cabsY` straddles `trackY..trackY+h`, so neither the `cabsY >= trackY + trackHeight` nor `cabsY + height <= trackY` check fires), and the arrow tip renders on the wrong edge. Fix: choose placement by available space instead of midline. Prefer whichever side fits the popup's preferred height; fall back to the side with more room when neither fits. The historical "over the rect" branches are kept as a last resort for the truly degenerate case where the rect fills the viewport top-to-bottom and there is literally no room either above or below it. Tests: - `maven/core-unittests/.../InteractionDialogTest`: two new unit tests pin the invariants for #5028 (popup must not overlap target) and #5029 (popup must land entirely above or below target so `CSSBorder.Arrow` can pick a direction). Both were verified to FAIL on the unmodified code with the exact overlap coordinates, and pass with the fix. - `tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java`, `InteractionDialogPopupTests5029.java`: ant-side regression tests that drive the same scenarios through `showPopupDialog(Rectangle)` and `showPopupDialog(Component)` end-to-end. - `tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java`: ant-side mirror of the existing `core-unittests` regression for PR #5011 / issue #4991 (full-width landscape anchor). The unit-test copy was painful to find when grepping the popup-regression tests directory; mirroring it keeps both copies discoverable. Full `core-unittests` suite: 2550 tests pass, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/InteractionDialog.java | 35 ++++- .../components/InteractionDialogTest.java | 77 ++++++++++ .../ui/InteractionDialogPopupTests4991.java | 115 ++++++++++++++ .../ui/InteractionDialogPopupTests5028.java | 136 +++++++++++++++++ .../ui/InteractionDialogPopupTests5029.java | 140 ++++++++++++++++++ 5 files changed, 498 insertions(+), 5 deletions(-) create mode 100644 tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java create mode 100644 tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java create mode 100644 tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java diff --git a/CodenameOne/src/com/codename1/components/InteractionDialog.java b/CodenameOne/src/com/codename1/components/InteractionDialog.java index cbd2ebf2fd..dbedbc3ceb 100644 --- a/CodenameOne/src/com/codename1/components/InteractionDialog.java +++ b/CodenameOne/src/com/codename1/components/InteractionDialog.java @@ -839,23 +839,48 @@ private void showPopupDialogImpl(Rectangle rect, boolean bias) { } } } - if (rect.getY() + rect.getHeight() < availableHeight / 2) { + // Pick the side of the rect (above vs. below) the popup goes + // on. The original logic chose purely by which half of the + // screen the rect sat in, which placed the popup ON TOP of + // the rect whenever it straddled the midline -- the symptom + // in #5028 (popup covers target) and #5029 (CSSBorder.Arrow + // can't pick a consistent direction so the tip renders on + // the wrong edge). Prefer whichever side fits the popup's + // preferred height, falling back to the larger side. The + // historical "over the rect" branches are kept as a last + // resort for the degenerate case where neither side has any + // room at all. + int spaceAbove = Math.max(0, rect.getY()); + int spaceBelow = Math.max(0, availableHeight - rect.getY() - rect.getHeight()); + boolean placeBelow; + if (spaceBelow >= prefHeight) { + placeBelow = true; + } else if (spaceAbove >= prefHeight) { + placeBelow = false; + } else if (spaceBelow >= spaceAbove) { + placeBelow = spaceBelow > 0; + } else { + placeBelow = false; + } + if (placeBelow && spaceBelow > 0) { // popup downwards y = rect.getY() + rect.getHeight(); - int height = Math.min(prefHeight, Math.max(0, availableHeight - y)); + int height = Math.min(prefHeight, spaceBelow); padOrientation(contentPaneStyle, TOP, 1); show(Math.max(0, y), Math.max(0, availableHeight - height - y), Math.max(0, x), Math.max(0, availableWidth - width - x)); padOrientation(contentPaneStyle, TOP, -1); - } else if (rect.getY() > availableHeight / 2) { + } else if (!placeBelow && spaceAbove > 0) { // popup upwards - int height = Math.min(prefHeight, rect.getY()); + int height = Math.min(prefHeight, spaceAbove); y = rect.getY() - height; padOrientation(contentPaneStyle, BOTTOM, 1); show(y, Math.max(0, availableHeight - rect.getY()), x, Math.max(0, availableWidth - width - x)); padOrientation(contentPaneStyle, BOTTOM, -1); } else if (rect.getY() < availableHeight / 2) { - // popup over aligned with top of rect, but inset a few mm + // popup over aligned with top of rect, but inset a few + // mm. Fallback for the truly degenerate case where the + // rect fills the viewport top-to-bottom. y = rect.getY() + CN.convertToPixels(3); int height = Math.min(prefHeight, availableHeight - y); diff --git a/maven/core-unittests/src/test/java/com/codename1/components/InteractionDialogTest.java b/maven/core-unittests/src/test/java/com/codename1/components/InteractionDialogTest.java index d0aa56acc3..2e6d387ddd 100644 --- a/maven/core-unittests/src/test/java/com/codename1/components/InteractionDialogTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/components/InteractionDialogTest.java @@ -97,6 +97,83 @@ void formModeUsesFormLayeredPane() { dialog.dispose(); } + @Test + void showPopupDialogStraddlingMidlineDoesNotOverlapTarget() { + // Regression for #5028: when the anchor rect straddles the + // vertical midline, the legacy placement logic fell through to a + // "popup over aligned with top of rect" branch that drew the + // popup ON TOP of the target (covering the Close button in the + // reporter's screenshot). The fix prefers above / below based on + // available space; the popup must end up entirely outside the + // target rect. + implementation.setDisplaySize(1080, 1920); + implementation.setPortrait(true); + Form form = new Form(new BorderLayout()); + implementation.setCurrentForm(form); + InteractionDialog dialog = new InteractionDialog(); + dialog.setAnimateShow(false); + dialog.addComponent(new Label("Popup body content")); + + // 60px target straddling the midline at y=960. + int targetHeight = 60; + int targetY = 1920 / 2 - targetHeight / 2; + Rectangle anchor = new Rectangle(490, targetY, 100, targetHeight); + dialog.showPopupDialog(anchor); + + int dlgTop = dialog.getAbsoluteY(); + int dlgBottom = dlgTop + dialog.getHeight(); + int targetBottom = targetY + targetHeight; + assertTrue(dialog.getHeight() > 0, "popup must have non-zero height"); + boolean overlaps = dlgTop < targetBottom && dlgBottom > targetY; + assertFalse(overlaps, + "#5028: popup [" + dlgTop + ".." + dlgBottom + + ") overlaps anchor [" + targetY + ".." + targetBottom + + ") -- expected the popup to land entirely above or below the rect"); + + dialog.dispose(); + } + + @Test + void showPopupDialogArrowDirectionConsistentWithPlacement() { + // Regression for #5029: with the popup ending up overlapping the + // target (the #5028 bug), CSSBorder.Arrow could not pick a + // consistent direction (cabsY straddles trackY..trackY+h) so the + // arrow tip rendered on the wrong edge. The arrow logic needs the + // popup to be either fully above or fully below the target; this + // test mirrors the reporter's geometry (target halfway down a + // tall column) and pins that invariant. + implementation.setDisplaySize(1080, 1920); + implementation.setPortrait(true); + Form form = new Form(new BorderLayout()); + implementation.setCurrentForm(form); + InteractionDialog dialog = new InteractionDialog(); + dialog.setAnimateShow(false); + dialog.addComponent(new Label("Popup body content")); + + // Target lives at y = available/2 - 1 (rect.getY() < availableHeight/2, + // rect.bottom > availableHeight/2). This is the exact case that + // used to hit the buggy "popup over aligned with top of rect" + // branch before the fix. + Rectangle anchor = new Rectangle(490, 1920 / 2 - 1, 100, 80); + dialog.showPopupDialog(anchor); + + int dlgTop = dialog.getAbsoluteY(); + int dlgBottom = dlgTop + dialog.getHeight(); + int targetTop = anchor.getY(); + int targetBottom = targetTop + anchor.getHeight(); + + boolean popupBelowTarget = dlgTop >= targetBottom; + boolean popupAboveTarget = dlgBottom <= targetTop; + assertTrue(popupBelowTarget || popupAboveTarget, + "#5029: popup at [" + dlgTop + ".." + dlgBottom + + ") is neither fully above nor fully below target [" + + targetTop + ".." + targetBottom + + ") -- CSSBorder.Arrow has no consistent direction" + + " to point at the target"); + + dialog.dispose(); + } + @Test void showPopupDialogLandscapeFullWidthRectGetsVisibleSize() { // Regression for #4991: in landscape, when the anchor rect spans the diff --git a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java new file mode 100644 index 0000000000..d3773b770e --- /dev/null +++ b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ +package com.codename1.ui; + +import com.codename1.components.InteractionDialog; +import com.codename1.testing.AbstractTest; +import com.codename1.ui.geom.Rectangle; +import com.codename1.ui.layouts.BorderLayout; + +/// Regression coverage for the fix shipped in +/// https://github.com/codenameone/CodenameOne/pull/5011 (issue +/// https://github.com/codenameone/CodenameOne/issues/4991). +/// +/// A unit-test version of this assertion already lives next to the fix in +/// `maven/core-unittests/.../InteractionDialogTest#showPopupDialogLandscapeFullWidthRectGetsVisibleSize`, +/// but that test was easy to miss when grepping `tests/core/test/com/codename1/ui` +/// where all of the other `InteractionDialog` / popup regressions live. +/// Mirroring the assertion here keeps both copies in sync and gives a +/// future investigator a single place to find the popup-placement +/// regressions for this component. +/// +/// The bug: in landscape, when the anchor rect spans (or nearly spans) +/// the full available width -- typical for a `Picker` in a Y-axis +/// `BoxLayout` row -- `showPopupDialogImpl` fell through to a "popup +/// left" branch that computed +/// `width = min(prefWidth, availableWidth - (availableWidth - rect.getX()))` +/// = 0. The dialog ended up on the layered pane with zero width. The +/// JS port reproduced this in desktop browser sessions because +/// `HTML5Implementation.isTablet()` returns true for viewports >= 600 CSS +/// px on the short side, sending `Picker.showInteractionDialog` down the +/// landscape branch. +/// +/// The PR added a guard in the orientation check at the top of +/// `showPopupDialogImpl` that flips to portrait-style placement (centered +/// horizontally, popping above or below the rect) when neither side has +/// room for the popup. This test pins that guard by driving the +/// landscape full-width-anchor scenario and asserting the rendered dialog +/// has a non-zero width. +public class InteractionDialogPopupTests4991 extends AbstractTest { + + @Override + public boolean shouldExecuteOnEDT() { + return true; + } + + @Override + public boolean runTest() throws Exception { + Form form = new Form(new BorderLayout()); + form.setName("PopupFullWidth4991"); + form.show(); + waitForFormName("PopupFullWidth4991"); + + Container layeredParent = form.getLayeredPane().getParent(); + int availableWidth = layeredParent.getWidth(); + if (availableWidth <= 0) { + availableWidth = CN.getDisplayWidth(); + } + int availableHeight = layeredParent.getHeight(); + if (availableHeight <= 0) { + availableHeight = CN.getDisplayHeight(); + } + + // Build a rect that spans the entire available width with a thin + // strip near the top -- this is the shape of a Picker laid out in + // a Y-axis BoxLayout row, which is what the original reporter + // (jsfan3) used to surface the zero-width popup. + int rectTop = layeredParent.getAbsoluteY() + CN.convertToPixels(10); + int rectHeight = CN.convertToPixels(8); + int rectLeft = layeredParent.getAbsoluteX(); + Rectangle anchor = new Rectangle(rectLeft, rectTop, availableWidth, rectHeight); + + InteractionDialog dlg = new InteractionDialog(); + dlg.add(new Label("Body content wide enough to matter")); + // Disable the show/hide animation so we can inspect the resting + // bounds of the dialog rather than an animation frame. + dlg.setAnimateShow(false); + try { + dlg.showPopupDialog(anchor); + waitFor(100); + + assertTrue(dlg.isShowing(), + "#4991: popup dialog should be attached to the layered pane"); + assertTrue(dlg.getWidth() > 0, + "#4991: popup dialog must have non-zero width after a" + + " full-width anchor; got " + dlg.getWidth() + + " (availableWidth=" + availableWidth + ")"); + assertTrue(dlg.getHeight() > 0, + "#4991: popup dialog must have non-zero height after a" + + " full-width anchor; got " + dlg.getHeight()); + } finally { + dlg.dispose(); + } + return true; + } +} diff --git a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java new file mode 100644 index 0000000000..12394ec064 --- /dev/null +++ b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ +package com.codename1.ui; + +import com.codename1.components.InteractionDialog; +import com.codename1.testing.AbstractTest; +import com.codename1.ui.geom.Rectangle; +import com.codename1.ui.layouts.BoxLayout; + +/// Regression coverage for https://github.com/codenameone/CodenameOne/issues/5028 +/// +/// `InteractionDialog.showPopupDialog(...)` is expected to place the popup +/// either above or below the target rectangle, never covering it. The +/// reporter showed that when the target lands roughly in the vertical middle +/// of the screen the popup is drawn ON TOP of it, hiding the target entirely. +/// +/// Reading `showPopupDialogImpl` makes the bug obvious: after the "popup +/// below" (`rect.getY() + rect.getHeight() < availableHeight / 2`) and +/// "popup above" (`rect.getY() > availableHeight / 2`) branches both reject +/// a target that straddles the midline, control falls through to a branch +/// that intentionally sets `y = rect.getY() + 3mm` -- i.e. it draws the +/// popup overlapping the target. +/// +/// This test drives `showPopupDialog(Rectangle)` directly with a target +/// rectangle straddling `availableHeight / 2`. After the layered pane is +/// laid out, the popup's bounding box must not intersect the target's +/// bounding box. The Rectangle overload is used (instead of the +/// `Component`-based overload from the reporter's snippet) so the test +/// pins the popup-placement logic without depending on Form scrolling +/// behavior that varies between display sizes. +/// +/// Related: [InteractionDialogPopupTests5029] -- same area, focused on the +/// arrow direction the popup paints on the initial show. +public class InteractionDialogPopupTests5028 extends AbstractTest { + + @Override + public boolean shouldExecuteOnEDT() { + return true; + } + + @Override + public boolean runTest() throws Exception { + Form form = new Form("Check dispose 5028", new BoxLayout(BoxLayout.Y_AXIS)); + form.setName("PopupOverlap5028"); + // A non-trivial child keeps the layered pane / content pane sized + // the way it would be in a real screen; without it the form has no + // height and the popup positioning math degenerates. + form.add(new Label("placeholder")); + form.show(); + waitForFormName("PopupOverlap5028"); + + Container layeredParent = form.getLayeredPane().getParent(); + int availableHeight = layeredParent.getHeight(); + int availableWidth = layeredParent.getWidth(); + if (availableHeight <= 0) { + availableHeight = CN.getDisplayHeight(); + } + if (availableWidth <= 0) { + availableWidth = CN.getDisplayWidth(); + } + + // Build a target rectangle that straddles the vertical midpoint. + // This is exactly the regime `showPopupDialogImpl` mishandles: + // neither the "popup below" nor the "popup above" branch matches, + // so it falls into the "popup over aligned with top of rect" branch + // that overlaps. Add the layered pane offset so the rect is in the + // absolute coordinate space the API expects. + int targetHeight = CN.convertToPixels(8); + int targetWidth = CN.convertToPixels(20); + int targetTop = layeredParent.getAbsoluteY() + + (availableHeight / 2) - (targetHeight / 2); + int targetLeft = layeredParent.getAbsoluteX() + + (availableWidth - targetWidth) / 2; + Rectangle target = new Rectangle( + targetLeft, targetTop, targetWidth, targetHeight); + + int targetBottomAbs = target.getY() + target.getHeight(); + assertTrue(target.getY() < layeredParent.getAbsoluteY() + availableHeight / 2 + && targetBottomAbs > layeredParent.getAbsoluteY() + availableHeight / 2, + "Test setup: target rect does not straddle midline -- cannot" + + " reproduce #5028 with this geometry (target=" + target + + ", midline=" + (layeredParent.getAbsoluteY() + availableHeight / 2) + + ")"); + + InteractionDialog dlg = new InteractionDialog("InteractionDialog"); + dlg.add(new Label("popup body")); + // Disable the show/hide animation so the popup lands at its final + // position synchronously; we want to inspect the resting bounds, not + // an animation frame. + dlg.setAnimateShow(false); + try { + dlg.showPopupDialog(target); + + // Let the layered pane finish revalidating. + waitFor(100); + + int dlgTop = dlg.getAbsoluteY(); + int dlgBottom = dlgTop + dlg.getHeight(); + + assertTrue(dlg.getHeight() > 0, + "#5028 setup: popup laid out with zero height (top=" + dlgTop + ")"); + + boolean overlaps = dlgTop < targetBottomAbs && dlgBottom > target.getY(); + assertTrue(!overlaps, + "#5028: popup dialog [" + dlgTop + ".." + dlgBottom + + ") overlaps target [" + target.getY() + ".." + + targetBottomAbs + ") -- popup should be placed" + + " either entirely above or entirely below the" + + " target, never covering it"); + + return true; + } finally { + dlg.dispose(); + } + } +} diff --git a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java new file mode 100644 index 0000000000..c85d7bead0 --- /dev/null +++ b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java @@ -0,0 +1,140 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ +package com.codename1.ui; + +import com.codename1.components.InteractionDialog; +import com.codename1.testing.AbstractTest; +import com.codename1.ui.layouts.BoxLayout; + +/// Regression coverage for https://github.com/codenameone/CodenameOne/issues/5029 +/// +/// In the reporter's scenario a popup `InteractionDialog` is anchored to a +/// `Button` halfway down a tall A..Z column. On the first show the popup's +/// arrow points UPWARDS -- away from the Close button it is supposed to +/// indicate -- and only flips to the correct side once the form is scrolled. +/// +/// The arrow direction in `CSSBorder.Arrow` is selected purely from the +/// popup component's `getAbsoluteY()` versus the stored track rectangle. +/// If the two end up pointing in inconsistent directions (because the popup +/// covers / overlaps the target, or because position info is stale on the +/// first paint), the arrow tip is rendered on the wrong edge. This test +/// drives the reporter's exact reproducer and asserts the spatial +/// invariant the arrow logic needs: after the popup settles, it must be +/// **entirely above or entirely below** the target -- never overlapping -- +/// and the side it ends up on must be consistent with an arrow pointing +/// AT the target (the mirror image of the +/// `cabsY >= trackY + trackHeight` / `cabsY + height <= trackY` checks +/// inside `CSSBorder.Arrow`). +/// +/// Related: [InteractionDialogPopupTests5028] -- same area, the +/// popup-overlap-target symptom of the same placement logic. +public class InteractionDialogPopupTests5029 extends AbstractTest { + + @Override + public boolean shouldExecuteOnEDT() { + return true; + } + + @Override + public boolean runTest() throws Exception { + Form form = new Form("Check dispose 5029", new BoxLayout(BoxLayout.Y_AXIS)); + form.setName("PopupArrow5029"); + + Container items = new Container(BoxLayout.y()); + Button target = new Button(""); + target.setName("popupTarget5029"); + // Mirror the reporter's reproducer: A..Z labels with a target + // Button slotted in between J and K. The exact y the target lands + // at depends on display size, but the bug surfaces as long as the + // target lives somewhere within the visible portion of the form + // (the case where `showPopupDialog` has to decide which side of + // the rect to place the popup on the first paint). + for (char c = 'A'; c <= 'Z'; c++) { + items.add(new Label(c + " entry ")); + if (c == 'J') { + items.add(target); + } + } + form.add(items); + form.show(); + waitForFormName("PopupArrow5029"); + + InteractionDialog dlg = new InteractionDialog("InteractionDialog"); + dlg.add(new Label("popup body")); + target.setCommand(Command.create("Close", null, e -> dlg.dispose())); + // Disable the show/hide animation so the popup lands at its final + // resting position synchronously. The animation matters in the + // real app, but for the spatial assertion below we just need the + // final placement. + dlg.setAnimateShow(false); + try { + dlg.showPopupDialog(target); + + // Let the layered pane finish revalidating. + waitFor(100); + + int targetTop = target.getAbsoluteY(); + int targetBottom = targetTop + target.getHeight(); + int dlgTop = dlg.getAbsoluteY(); + int dlgBottom = dlgTop + dlg.getHeight(); + + assertTrue(dlg.getHeight() > 0, + "#5029 setup: popup laid out with zero height (top=" + + dlgTop + ")"); + + // Invariant 1: the popup must not cover the target. If it does, + // the arrow direction inside CSSBorder.Arrow has no consistent + // answer (cabsY straddles trackY..trackY+h) and the arrow ends + // up on the wrong edge -- the user-visible symptom in #5029. + boolean overlaps = dlgTop < targetBottom && dlgBottom > targetTop; + assertTrue(!overlaps, + "#5029: popup dialog [" + dlgTop + ".." + dlgBottom + + ") overlaps target [" + targetTop + ".." + + targetBottom + ") -- arrow direction cannot" + + " be chosen consistently while the popup" + + " covers the target"); + + // Invariant 2: whichever side the popup landed on, the arrow + // direction `CSSBorder.Arrow` would compute from that position + // must point AT the target (i.e. the opposite edge from the + // popup). The two checks below mirror the conditions in + // `CSSBorder.Arrow` (cabsY >= trackY + trackHeight => arrow + // direction TOP, cabsY + height <= trackY => arrow direction + // BOTTOM). One of them must hold; otherwise the popup is + // somewhere the arrow logic cannot point from, which is the + // exact failure mode in the bug report. + boolean popupBelowTarget = dlgTop >= targetBottom; + boolean popupAboveTarget = dlgBottom <= targetTop; + assertTrue(popupBelowTarget || popupAboveTarget, + "#5029: popup at y=[" + dlgTop + ".." + dlgBottom + + ") is neither fully above nor fully below" + + " target [" + targetTop + ".." + targetBottom + + ") -- CSSBorder.Arrow has no consistent" + + " direction to render the arrow on"); + + return true; + } finally { + dlg.dispose(); + } + } +} From 4f640d6a80f11d1f108dd7b07896c82b3af95514 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sun, 24 May 2026 17:50:06 +0300 Subject: [PATCH 2/2] Drop ant-side InteractionDialog popup tests (dead weight in CI) The three `tests/core/test/com/codename1/ui/InteractionDialogPopupTests*.java` files I added in the previous commit don't actually run in CI: - `pr.yml` calls `ant test-javase`, but the body of that target in `build.xml:87-97` is wrapped in `` -- effectively a no-op. - `ant.yml` runs `tests/all.sh` -> `tests/core.sh` which builds a cn1app archetype project from `tests/core/` and runs `mvn package`. Surefire enumerates the .class files but reports `Tests run: 0` for each -- these classes extend `AbstractTest` rather than JUnit so surefire finds no test methods, and the CN1 `TestRunner` that would invoke `runTest()` is not wired into the lifecycle. Verified the same on the latest master CI run: `RoundRectBorderTest`, `TestComponent2`, and other long-standing ant tests all report `Tests run: 0`, and `SideMenuHamburgerTests4895` doesn't appear at all. The two unit tests added to `maven/core-unittests/.../InteractionDialogTest` cover the same assertions and DO execute in CI (via the "Run Unit Tests" step in pr.yml: `mvn clean verify -DunitTests=true -pl core-unittests -am`). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ui/InteractionDialogPopupTests4991.java | 115 -------------- .../ui/InteractionDialogPopupTests5028.java | 136 ----------------- .../ui/InteractionDialogPopupTests5029.java | 140 ------------------ 3 files changed, 391 deletions(-) delete mode 100644 tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java delete mode 100644 tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java delete mode 100644 tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java diff --git a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java deleted file mode 100644 index d3773b770e..0000000000 --- a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests4991.java +++ /dev/null @@ -1,115 +0,0 @@ -/* - * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. Codename One designates this - * particular file as subject to the "Classpath" exception as provided - * by Oracle in the LICENSE file that accompanied this code. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Codename One through http://www.codenameone.com/ if you - * need additional information or have any questions. - */ -package com.codename1.ui; - -import com.codename1.components.InteractionDialog; -import com.codename1.testing.AbstractTest; -import com.codename1.ui.geom.Rectangle; -import com.codename1.ui.layouts.BorderLayout; - -/// Regression coverage for the fix shipped in -/// https://github.com/codenameone/CodenameOne/pull/5011 (issue -/// https://github.com/codenameone/CodenameOne/issues/4991). -/// -/// A unit-test version of this assertion already lives next to the fix in -/// `maven/core-unittests/.../InteractionDialogTest#showPopupDialogLandscapeFullWidthRectGetsVisibleSize`, -/// but that test was easy to miss when grepping `tests/core/test/com/codename1/ui` -/// where all of the other `InteractionDialog` / popup regressions live. -/// Mirroring the assertion here keeps both copies in sync and gives a -/// future investigator a single place to find the popup-placement -/// regressions for this component. -/// -/// The bug: in landscape, when the anchor rect spans (or nearly spans) -/// the full available width -- typical for a `Picker` in a Y-axis -/// `BoxLayout` row -- `showPopupDialogImpl` fell through to a "popup -/// left" branch that computed -/// `width = min(prefWidth, availableWidth - (availableWidth - rect.getX()))` -/// = 0. The dialog ended up on the layered pane with zero width. The -/// JS port reproduced this in desktop browser sessions because -/// `HTML5Implementation.isTablet()` returns true for viewports >= 600 CSS -/// px on the short side, sending `Picker.showInteractionDialog` down the -/// landscape branch. -/// -/// The PR added a guard in the orientation check at the top of -/// `showPopupDialogImpl` that flips to portrait-style placement (centered -/// horizontally, popping above or below the rect) when neither side has -/// room for the popup. This test pins that guard by driving the -/// landscape full-width-anchor scenario and asserting the rendered dialog -/// has a non-zero width. -public class InteractionDialogPopupTests4991 extends AbstractTest { - - @Override - public boolean shouldExecuteOnEDT() { - return true; - } - - @Override - public boolean runTest() throws Exception { - Form form = new Form(new BorderLayout()); - form.setName("PopupFullWidth4991"); - form.show(); - waitForFormName("PopupFullWidth4991"); - - Container layeredParent = form.getLayeredPane().getParent(); - int availableWidth = layeredParent.getWidth(); - if (availableWidth <= 0) { - availableWidth = CN.getDisplayWidth(); - } - int availableHeight = layeredParent.getHeight(); - if (availableHeight <= 0) { - availableHeight = CN.getDisplayHeight(); - } - - // Build a rect that spans the entire available width with a thin - // strip near the top -- this is the shape of a Picker laid out in - // a Y-axis BoxLayout row, which is what the original reporter - // (jsfan3) used to surface the zero-width popup. - int rectTop = layeredParent.getAbsoluteY() + CN.convertToPixels(10); - int rectHeight = CN.convertToPixels(8); - int rectLeft = layeredParent.getAbsoluteX(); - Rectangle anchor = new Rectangle(rectLeft, rectTop, availableWidth, rectHeight); - - InteractionDialog dlg = new InteractionDialog(); - dlg.add(new Label("Body content wide enough to matter")); - // Disable the show/hide animation so we can inspect the resting - // bounds of the dialog rather than an animation frame. - dlg.setAnimateShow(false); - try { - dlg.showPopupDialog(anchor); - waitFor(100); - - assertTrue(dlg.isShowing(), - "#4991: popup dialog should be attached to the layered pane"); - assertTrue(dlg.getWidth() > 0, - "#4991: popup dialog must have non-zero width after a" - + " full-width anchor; got " + dlg.getWidth() - + " (availableWidth=" + availableWidth + ")"); - assertTrue(dlg.getHeight() > 0, - "#4991: popup dialog must have non-zero height after a" - + " full-width anchor; got " + dlg.getHeight()); - } finally { - dlg.dispose(); - } - return true; - } -} diff --git a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java deleted file mode 100644 index 12394ec064..0000000000 --- a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5028.java +++ /dev/null @@ -1,136 +0,0 @@ -/* - * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. Codename One designates this - * particular file as subject to the "Classpath" exception as provided - * by Oracle in the LICENSE file that accompanied this code. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Codename One through http://www.codenameone.com/ if you - * need additional information or have any questions. - */ -package com.codename1.ui; - -import com.codename1.components.InteractionDialog; -import com.codename1.testing.AbstractTest; -import com.codename1.ui.geom.Rectangle; -import com.codename1.ui.layouts.BoxLayout; - -/// Regression coverage for https://github.com/codenameone/CodenameOne/issues/5028 -/// -/// `InteractionDialog.showPopupDialog(...)` is expected to place the popup -/// either above or below the target rectangle, never covering it. The -/// reporter showed that when the target lands roughly in the vertical middle -/// of the screen the popup is drawn ON TOP of it, hiding the target entirely. -/// -/// Reading `showPopupDialogImpl` makes the bug obvious: after the "popup -/// below" (`rect.getY() + rect.getHeight() < availableHeight / 2`) and -/// "popup above" (`rect.getY() > availableHeight / 2`) branches both reject -/// a target that straddles the midline, control falls through to a branch -/// that intentionally sets `y = rect.getY() + 3mm` -- i.e. it draws the -/// popup overlapping the target. -/// -/// This test drives `showPopupDialog(Rectangle)` directly with a target -/// rectangle straddling `availableHeight / 2`. After the layered pane is -/// laid out, the popup's bounding box must not intersect the target's -/// bounding box. The Rectangle overload is used (instead of the -/// `Component`-based overload from the reporter's snippet) so the test -/// pins the popup-placement logic without depending on Form scrolling -/// behavior that varies between display sizes. -/// -/// Related: [InteractionDialogPopupTests5029] -- same area, focused on the -/// arrow direction the popup paints on the initial show. -public class InteractionDialogPopupTests5028 extends AbstractTest { - - @Override - public boolean shouldExecuteOnEDT() { - return true; - } - - @Override - public boolean runTest() throws Exception { - Form form = new Form("Check dispose 5028", new BoxLayout(BoxLayout.Y_AXIS)); - form.setName("PopupOverlap5028"); - // A non-trivial child keeps the layered pane / content pane sized - // the way it would be in a real screen; without it the form has no - // height and the popup positioning math degenerates. - form.add(new Label("placeholder")); - form.show(); - waitForFormName("PopupOverlap5028"); - - Container layeredParent = form.getLayeredPane().getParent(); - int availableHeight = layeredParent.getHeight(); - int availableWidth = layeredParent.getWidth(); - if (availableHeight <= 0) { - availableHeight = CN.getDisplayHeight(); - } - if (availableWidth <= 0) { - availableWidth = CN.getDisplayWidth(); - } - - // Build a target rectangle that straddles the vertical midpoint. - // This is exactly the regime `showPopupDialogImpl` mishandles: - // neither the "popup below" nor the "popup above" branch matches, - // so it falls into the "popup over aligned with top of rect" branch - // that overlaps. Add the layered pane offset so the rect is in the - // absolute coordinate space the API expects. - int targetHeight = CN.convertToPixels(8); - int targetWidth = CN.convertToPixels(20); - int targetTop = layeredParent.getAbsoluteY() - + (availableHeight / 2) - (targetHeight / 2); - int targetLeft = layeredParent.getAbsoluteX() - + (availableWidth - targetWidth) / 2; - Rectangle target = new Rectangle( - targetLeft, targetTop, targetWidth, targetHeight); - - int targetBottomAbs = target.getY() + target.getHeight(); - assertTrue(target.getY() < layeredParent.getAbsoluteY() + availableHeight / 2 - && targetBottomAbs > layeredParent.getAbsoluteY() + availableHeight / 2, - "Test setup: target rect does not straddle midline -- cannot" - + " reproduce #5028 with this geometry (target=" + target - + ", midline=" + (layeredParent.getAbsoluteY() + availableHeight / 2) - + ")"); - - InteractionDialog dlg = new InteractionDialog("InteractionDialog"); - dlg.add(new Label("popup body")); - // Disable the show/hide animation so the popup lands at its final - // position synchronously; we want to inspect the resting bounds, not - // an animation frame. - dlg.setAnimateShow(false); - try { - dlg.showPopupDialog(target); - - // Let the layered pane finish revalidating. - waitFor(100); - - int dlgTop = dlg.getAbsoluteY(); - int dlgBottom = dlgTop + dlg.getHeight(); - - assertTrue(dlg.getHeight() > 0, - "#5028 setup: popup laid out with zero height (top=" + dlgTop + ")"); - - boolean overlaps = dlgTop < targetBottomAbs && dlgBottom > target.getY(); - assertTrue(!overlaps, - "#5028: popup dialog [" + dlgTop + ".." + dlgBottom - + ") overlaps target [" + target.getY() + ".." - + targetBottomAbs + ") -- popup should be placed" - + " either entirely above or entirely below the" - + " target, never covering it"); - - return true; - } finally { - dlg.dispose(); - } - } -} diff --git a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java b/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java deleted file mode 100644 index c85d7bead0..0000000000 --- a/tests/core/test/com/codename1/ui/InteractionDialogPopupTests5029.java +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. Codename One designates this - * particular file as subject to the "Classpath" exception as provided - * by Oracle in the LICENSE file that accompanied this code. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Codename One through http://www.codenameone.com/ if you - * need additional information or have any questions. - */ -package com.codename1.ui; - -import com.codename1.components.InteractionDialog; -import com.codename1.testing.AbstractTest; -import com.codename1.ui.layouts.BoxLayout; - -/// Regression coverage for https://github.com/codenameone/CodenameOne/issues/5029 -/// -/// In the reporter's scenario a popup `InteractionDialog` is anchored to a -/// `Button` halfway down a tall A..Z column. On the first show the popup's -/// arrow points UPWARDS -- away from the Close button it is supposed to -/// indicate -- and only flips to the correct side once the form is scrolled. -/// -/// The arrow direction in `CSSBorder.Arrow` is selected purely from the -/// popup component's `getAbsoluteY()` versus the stored track rectangle. -/// If the two end up pointing in inconsistent directions (because the popup -/// covers / overlaps the target, or because position info is stale on the -/// first paint), the arrow tip is rendered on the wrong edge. This test -/// drives the reporter's exact reproducer and asserts the spatial -/// invariant the arrow logic needs: after the popup settles, it must be -/// **entirely above or entirely below** the target -- never overlapping -- -/// and the side it ends up on must be consistent with an arrow pointing -/// AT the target (the mirror image of the -/// `cabsY >= trackY + trackHeight` / `cabsY + height <= trackY` checks -/// inside `CSSBorder.Arrow`). -/// -/// Related: [InteractionDialogPopupTests5028] -- same area, the -/// popup-overlap-target symptom of the same placement logic. -public class InteractionDialogPopupTests5029 extends AbstractTest { - - @Override - public boolean shouldExecuteOnEDT() { - return true; - } - - @Override - public boolean runTest() throws Exception { - Form form = new Form("Check dispose 5029", new BoxLayout(BoxLayout.Y_AXIS)); - form.setName("PopupArrow5029"); - - Container items = new Container(BoxLayout.y()); - Button target = new Button(""); - target.setName("popupTarget5029"); - // Mirror the reporter's reproducer: A..Z labels with a target - // Button slotted in between J and K. The exact y the target lands - // at depends on display size, but the bug surfaces as long as the - // target lives somewhere within the visible portion of the form - // (the case where `showPopupDialog` has to decide which side of - // the rect to place the popup on the first paint). - for (char c = 'A'; c <= 'Z'; c++) { - items.add(new Label(c + " entry ")); - if (c == 'J') { - items.add(target); - } - } - form.add(items); - form.show(); - waitForFormName("PopupArrow5029"); - - InteractionDialog dlg = new InteractionDialog("InteractionDialog"); - dlg.add(new Label("popup body")); - target.setCommand(Command.create("Close", null, e -> dlg.dispose())); - // Disable the show/hide animation so the popup lands at its final - // resting position synchronously. The animation matters in the - // real app, but for the spatial assertion below we just need the - // final placement. - dlg.setAnimateShow(false); - try { - dlg.showPopupDialog(target); - - // Let the layered pane finish revalidating. - waitFor(100); - - int targetTop = target.getAbsoluteY(); - int targetBottom = targetTop + target.getHeight(); - int dlgTop = dlg.getAbsoluteY(); - int dlgBottom = dlgTop + dlg.getHeight(); - - assertTrue(dlg.getHeight() > 0, - "#5029 setup: popup laid out with zero height (top=" - + dlgTop + ")"); - - // Invariant 1: the popup must not cover the target. If it does, - // the arrow direction inside CSSBorder.Arrow has no consistent - // answer (cabsY straddles trackY..trackY+h) and the arrow ends - // up on the wrong edge -- the user-visible symptom in #5029. - boolean overlaps = dlgTop < targetBottom && dlgBottom > targetTop; - assertTrue(!overlaps, - "#5029: popup dialog [" + dlgTop + ".." + dlgBottom - + ") overlaps target [" + targetTop + ".." - + targetBottom + ") -- arrow direction cannot" - + " be chosen consistently while the popup" - + " covers the target"); - - // Invariant 2: whichever side the popup landed on, the arrow - // direction `CSSBorder.Arrow` would compute from that position - // must point AT the target (i.e. the opposite edge from the - // popup). The two checks below mirror the conditions in - // `CSSBorder.Arrow` (cabsY >= trackY + trackHeight => arrow - // direction TOP, cabsY + height <= trackY => arrow direction - // BOTTOM). One of them must hold; otherwise the popup is - // somewhere the arrow logic cannot point from, which is the - // exact failure mode in the bug report. - boolean popupBelowTarget = dlgTop >= targetBottom; - boolean popupAboveTarget = dlgBottom <= targetTop; - assertTrue(popupBelowTarget || popupAboveTarget, - "#5029: popup at y=[" + dlgTop + ".." + dlgBottom - + ") is neither fully above nor fully below" - + " target [" + targetTop + ".." + targetBottom - + ") -- CSSBorder.Arrow has no consistent" - + " direction to render the arrow on"); - - return true; - } finally { - dlg.dispose(); - } - } -}