From 8b952ff1ecf32361820b26f453cf98d9daaaee21 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 12 May 2026 04:51:11 +0300 Subject: [PATCH 1/3] fix(picker): stage setX during lightweight popup and roll back on Cancel Follow-up to #4897: custom popup buttons that mutate the picker mid-edit (e.g. "+7 days" calling setDate(getDate() + 7d)) wrote the staged value straight into the committed `value` field. If the user then dismissed the popup with Cancel, getDate() returned the staged value instead of the date the picker held before editing began, leaking the unwanted change into the picker. Snapshot `value` into a new preEditValue field at the moment the lightweight popup is shown, then roll back to it in endEditing when the user presses Cancel. Done / Next / Prev still commit the wheel value as before, and clear the snapshot on the way out. Native pickers are unchanged - they are read-only while shown, which the existing setter Javadocs already call out; the new staging semantics only apply to the lightweight popup. Adds PickerCancelRestoreTest: drives a Picker with a "+7 Days" custom button programmatically, asserts that Cancel restores the pre-edit date and that Done commits the staged date. Registered in Cn1ssDeviceRunner alongside the existing lightweight-picker tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/com/codename1/ui/spinner/Picker.java | 51 ++++- .../tests/Cn1ssDeviceRunner.java | 1 + .../tests/PickerCancelRestoreTest.java | 176 ++++++++++++++++++ 3 files changed, 219 insertions(+), 9 deletions(-) create mode 100644 scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/PickerCancelRestoreTest.java diff --git a/CodenameOne/src/com/codename1/ui/spinner/Picker.java b/CodenameOne/src/com/codename1/ui/spinner/Picker.java index b5be0ac298..237c88c973 100644 --- a/CodenameOne/src/com/codename1/ui/spinner/Picker.java +++ b/CodenameOne/src/com/codename1/ui/spinner/Picker.java @@ -124,6 +124,13 @@ public class Picker extends Button { /// so that setters propagate into the visible wheels and getters read the wheel /// position. Null whenever the popup is not showing. private InternalPickerWidget currentSpinner; + /// Snapshot of `value` taken right before the lightweight popup is shown. + /// Setter calls from custom popup buttons (e.g. a "+7 days" action that does + /// `setDate(getDate() + 7d)`) stage their result into `value`, but if the user + /// dismisses the popup with Cancel we roll `value` back to this snapshot so + /// `getDate()` returns what the picker held before editing began. Non-null only + /// while the popup is on screen. + private Object preEditValue; /// Placement options for custom lightweight popup buttons. public static final class LightweightPopupButtonPlacement { @@ -493,7 +500,15 @@ private void endEditing(int command, InteractionDialog dlg, InternalPickerWidget } restoreContentPane(); dlg.disposeToTheBottom(); - if (command != COMMAND_CANCEL) { + if (command == COMMAND_CANCEL) { + // Roll back any setX calls made while the popup was showing + // (e.g. a custom "+7 days" button) so getDate() returns the + // value the picker held before editing began. + value = preEditValue; + preEditValue = null; + updateValue(); + } else { + preEditValue = null; value = spinner.getValue(); updateValue(); // (x, y) = (-99, -99) signals the built-in action listner @@ -545,6 +560,13 @@ private void showInteractionDialog() { throw new IllegalArgumentException("Unsupported picker type " + type); } currentSpinner = spinner; + // Snapshot the committed value so a Cancel press can restore it. + // Custom popup buttons stage their result through setDate / setTime / + // setDuration / setSelectedString etc., which now mutate `value` + // directly so that getDate() during the edit returns the staged + // value; without this snapshot a Cancel after such a button would + // leak the staged value into the picker (#4897 follow-up). + preEditValue = value; final InteractionDialog dlg = new InteractionDialog() { ActionListener keyListener; @@ -1238,6 +1260,9 @@ public Date getDate() { /// If the lightweight popup is currently on screen the visible scroll wheels are also /// moved to the new value, so a custom popup button can do `setDate(getDate() + n)` and /// have the wheels reflect it (see `#addLightweightPopupButton(String, Runnable)`). + /// Such changes are *staged*: if the user dismisses the popup with Cancel the picker + /// rolls back to the date it held before the popup was shown; if the user presses Done + /// the staged value is committed. /// The native picker is read-only while shown - calling `setDate` against a Picker whose /// native popup is open updates the committed `value` but leaves the on-screen wheels /// unchanged until the user dismisses and re-opens the picker. @@ -1515,8 +1540,10 @@ public String getSelectedString() { } /// Sets the current value in a string array picker. If the lightweight popup is on screen - /// the visible scroll wheel is also moved to the new value; the native picker is read-only - /// while shown so against a native popup this updates only the committed value. + /// the visible scroll wheel is also moved to the new value, and the change is *staged*: + /// a Cancel press rolls back to the value the picker held before the popup was shown, + /// while a Done press commits it. The native picker is read-only while shown so against + /// a native popup this updates only the committed value. /// /// #### Parameters /// @@ -1558,8 +1585,10 @@ public int getSelectedStringIndex() { } /// Sets the index of the selected string. If the lightweight popup is on screen the - /// visible wheel is also moved to the new value; the native picker is read-only while - /// shown so against a native popup this updates only the committed value. + /// visible wheel is also moved to the new value, and the change is *staged*: a Cancel + /// press rolls back to the value the picker held before the popup was shown, while a + /// Done press commits it. The native picker is read-only while shown so against a + /// native popup this updates only the committed value. /// /// #### Parameters /// @@ -1676,8 +1705,10 @@ public int getTime() { /// This value is only used for time type and is ignored in the case of date and time where /// both are embedded within the date. If the lightweight popup is on screen the visible - /// scroll wheels are also moved to the new value; the native picker is read-only while - /// shown so against a native popup this updates only the committed value. + /// scroll wheels are also moved to the new value, and the change is *staged*: a Cancel + /// press rolls back to the value the picker held before the popup was shown, while a + /// Done press commits it. The native picker is read-only while shown so against a native + /// popup this updates only the committed value. /// /// #### Parameters /// @@ -1738,8 +1769,10 @@ public long getDuration() { } /// This value is only used for duration type. If the lightweight popup is on screen the - /// visible scroll wheels are also moved to the new value; the native picker is read-only - /// while shown so against a native popup this updates only the committed value. + /// visible scroll wheels are also moved to the new value, and the change is *staged*: + /// a Cancel press rolls back to the value the picker held before the popup was shown, + /// while a Done press commits it. The native picker is read-only while shown so against + /// a native popup this updates only the committed value. /// /// #### Parameters /// diff --git a/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/Cn1ssDeviceRunner.java b/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/Cn1ssDeviceRunner.java index a62561c108..5071520dcd 100644 --- a/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/Cn1ssDeviceRunner.java +++ b/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/Cn1ssDeviceRunner.java @@ -176,6 +176,7 @@ private static int testTimeoutMs() { new TextAreaAlignmentScreenshotTest(), new ValidatorLightweightPickerScreenshotTest(), new LightweightPickerButtonsScreenshotTest(), + new PickerCancelRestoreTest(), new ToastBarTopPositionScreenshotTest(), // Native-theme fidelity tests (Phase 7): each emits a light+dark PNG pair // so the iOS Modern and Android Material themes get exercised per UIID. diff --git a/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/PickerCancelRestoreTest.java b/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/PickerCancelRestoreTest.java new file mode 100644 index 0000000000..ac6150d1ac --- /dev/null +++ b/scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/PickerCancelRestoreTest.java @@ -0,0 +1,176 @@ +package com.codenameone.examples.hellocodenameone.tests; + +import com.codename1.ui.Button; +import com.codename1.ui.Component; +import com.codename1.ui.Container; +import com.codename1.ui.Display; +import com.codename1.ui.Form; +import com.codename1.ui.layouts.BoxLayout; +import com.codename1.ui.spinner.Picker; +import com.codename1.ui.util.UITimer; + +import java.time.LocalDate; +import java.time.ZoneId; +import java.util.Date; + +/** + * Regression test for the staging behavior layered on top of #4897: + * `setX` calls made via custom lightweight popup buttons while the + * picker is showing must be staged. If the user dismisses with Cancel + * the staged value rolls back to whatever the picker held before the + * popup was shown; if the user presses Done the staged value is + * committed. Drives the picker programmatically and asserts + * {@code getDate()} after each dismiss path. + */ +public class PickerCancelRestoreTest extends BaseTest { + private Form form; + private Picker picker; + private Date initialDate; + private Date stagedDate; + + @Override + public boolean shouldTakeScreenshot() { + return false; + } + + @Override + public boolean runTest() { + initialDate = toDate(LocalDate.of(2026, 4, 11)); + stagedDate = toDate(LocalDate.of(2026, 4, 18)); + + form = new Form("Picker Cancel Restore", BoxLayout.y()) { + @Override + protected void onShowCompleted() { + runCancelScenario(); + } + }; + picker = new Picker(); + picker.setType(Display.PICKER_TYPE_DATE); + picker.setUseLightweightPopup(true); + picker.setDate(initialDate); + picker.addLightweightPopupButton("+7 Days", new Runnable() { + @Override + public void run() { + picker.setDate(stagedDate); + } + }); + form.add(picker); + form.show(); + return true; + } + + private void runCancelScenario() { + picker.startEditingAsync(); + UITimer.timer(600, false, form, new Runnable() { + @Override + public void run() { + if (!clickPopupButton("+7 Days")) { + return; + } + if (!datesEqual(stagedDate, picker.getDate())) { + fail("Staged setDate did not take effect during edit. Expected " + + stagedDate + " but got " + picker.getDate()); + return; + } + if (!clickPopupButton("Cancel")) { + return; + } + // Cancel dismisses the popup; give the dialog a frame to + // settle so the picker has run its rollback before we read. + UITimer.timer(600, false, form, new Runnable() { + @Override + public void run() { + if (!datesEqual(initialDate, picker.getDate())) { + fail("Cancel did not restore initial date. Expected " + + initialDate + " but got " + picker.getDate()); + return; + } + runDoneScenario(); + } + }); + } + }); + } + + private void runDoneScenario() { + picker.startEditingAsync(); + UITimer.timer(600, false, form, new Runnable() { + @Override + public void run() { + if (!clickPopupButton("+7 Days")) { + return; + } + if (!clickPopupButton("Done")) { + return; + } + UITimer.timer(600, false, form, new Runnable() { + @Override + public void run() { + if (!datesEqual(stagedDate, picker.getDate())) { + fail("Done did not commit staged date. Expected " + + stagedDate + " but got " + picker.getDate()); + return; + } + done(); + } + }); + } + }); + } + + private boolean clickPopupButton(String text) { + Button btn = findButtonByText(form, text); + if (btn == null) { + fail("Could not find button with text '" + text + "' in the picker popup"); + return false; + } + btn.pressed(); + btn.released(); + return true; + } + + private static Button findButtonByText(Container c, String text) { + for (Component child : c) { + if (child instanceof Button && matchesButtonText((Button) child, text)) { + return (Button) child; + } + if (child instanceof Container) { + Button result = findButtonByText((Container) child, text); + if (result != null) { + return result; + } + } + } + return null; + } + + private static boolean matchesButtonText(Button btn, String text) { + // Button.setText() uppercases when capsTextDefault is on (Material + // theme default), so a case-insensitive compare matches both + // "Cancel" and "CANCEL"; the pre-caps original is also stashed in + // a client property which we check as a second chance. + if (text.equalsIgnoreCase(btn.getText())) { + return true; + } + Object orig = btn.getClientProperty("cn1$origText"); + return orig instanceof String && text.equalsIgnoreCase((String) orig); + } + + private static boolean datesEqual(Date expected, Date actual) { + if (expected == actual) { + return true; + } + if (expected == null || actual == null) { + return false; + } + // The picker's DateSpinner3D commits midnight-of-day, so a tolerant + // day-granularity compare keeps the test stable against the wheel + // wrapping the time component when Done is pressed. + return expected.getTime() / (24L * 60 * 60 * 1000) + == actual.getTime() / (24L * 60 * 60 * 1000); + } + + private static Date toDate(LocalDate date) { + return new Date(date.atStartOfDay(ZoneId.systemDefault()).toInstant().toEpochMilli()); + } +} From 000e15d5cc748976e6230cedc492343b250ae60f Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 12 May 2026 06:05:23 +0300 Subject: [PATCH 2/3] fix(picker): scope close()'s currentSpinner/currentInput nulling to its own popup The follow-up cancel/done test caught a pre-existing bug exposed once the popup was reopened after a Cancel. endEditing nulls Picker's local currentInput field but leaves Form.currentInputDevice pointing at the just-disposed VirtualInputDevice. When the next showInteractionDialog reassigns Picker.currentSpinner to the freshly created spinner and calls Form.setCurrentInputDevice(newInput), the form synchronously calls close() on the OLD VirtualInputDevice -- whose close() was unconditionally setting currentSpinner = null. That clobbered the new popup's live wheel reference, so setX calls from custom buttons no longer propagated and Done committed the spinner's initial value. Capture the spinner in registerAsInputDevice and only null currentSpinner / currentInput in close() when they still point at THIS popup, mirroring the same compare-then-null pattern endEditing already uses. Single-popup flows (initial open, Done, Cancel, stopEditing) are unaffected because the fields still match THIS device when close() fires. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/com/codename1/ui/spinner/Picker.java | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/CodenameOne/src/com/codename1/ui/spinner/Picker.java b/CodenameOne/src/com/codename1/ui/spinner/Picker.java index 237c88c973..ef9337a6c1 100644 --- a/CodenameOne/src/com/codename1/ui/spinner/Picker.java +++ b/CodenameOne/src/com/codename1/ui/spinner/Picker.java @@ -759,7 +759,7 @@ public void actionPerformed(ActionEvent evt) { dlg.setY(Display.getInstance().getDisplayHeight()); dlg.setX(0); dlg.setRepositionAnimation(false); - registerAsInputDevice(dlg); + registerAsInputDevice(dlg, spinner); if (Display.getInstance().isTablet()) { getComponentForm().getAnimationManager().flushAnimation(new Runnable() { @@ -1354,7 +1354,7 @@ public void run() { } } - private void registerAsInputDevice(final InteractionDialog dlg) { + private void registerAsInputDevice(final InteractionDialog dlg, final InternalPickerWidget spinner) { final Form f = this.getComponentForm(); if (f != null) { @@ -1399,8 +1399,19 @@ public void run() { @Override public void close() throws Exception { - currentInput = null; - currentSpinner = null; + // Only null the picker-wide fields if they still point at THIS popup. + // When a second popup opens before the form has finished switching + // input devices, Form#setCurrentInputDevice calls close() on the + // previous device AFTER showInteractionDialog has already assigned + // currentSpinner / currentInput to the new popup; unconditionally + // nulling them here would clobber the new popup's live spinner and + // break setX propagation. + if (currentInput == this) { //NOPMD CompareObjectsWithEquals + currentInput = null; + } + if (currentSpinner == spinner) { //NOPMD CompareObjectsWithEquals + currentSpinner = null; + } if (sizeChanged != null) { f.removeSizeChangedListener(sizeChanged); } From a5491b19f9e158eba3c2512c7354cfe350249d6b Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 12 May 2026 11:37:42 +0300 Subject: [PATCH 3/3] ci(javascript-screenshots): re-install Playwright chromium even on cache hit The cache key was fixed at v1 while npm install playwright resolves the floating latest. When Playwright shipped a new chromium build between runs, the cached browser binary stayed at the older revision but the freshly installed Playwright pointed at a path that didn't exist (chromium_headless_shell-1223/...), so every CI run since the drift failed with "browserType.launch: Executable doesn't exist" before any test could start. Always run `npx playwright install chromium` after npm install. It's a no-op when the cached binary matches and re-downloads when versions drift, so the workflow self-heals on the next Playwright bump. OS deps still install unconditionally (they were never cached). Bumps the cache key to v2 so the bad entry gets rebuilt instead of restored. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/scripts-javascript.yml | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/workflows/scripts-javascript.yml b/.github/workflows/scripts-javascript.yml index e6aa710c32..1b6c4b95c8 100644 --- a/.github/workflows/scripts-javascript.yml +++ b/.github/workflows/scripts-javascript.yml @@ -107,7 +107,7 @@ jobs: uses: actions/cache@v4 with: path: ~/.cache/ms-playwright - key: ${{ runner.os }}-playwright-chromium-v1 + key: ${{ runner.os }}-playwright-chromium-v2 restore-keys: | ${{ runner.os }}-playwright- @@ -116,11 +116,16 @@ jobs: cd scripts npm init -y 2>/dev/null || true npm install playwright - if [ "${{ steps.playwright-cache.outputs.cache-hit }}" = "true" ]; then - npx playwright install-deps chromium - else - npx playwright install --with-deps chromium - fi + # OS-level deps (apt packages) aren't cached so always install them. + npx playwright install-deps chromium + # `npm install playwright` resolves the floating version, so the + # cached browser binary can drift away from what the freshly + # installed Playwright expects (the cache only stores the binary). + # `install chromium` is a no-op when the versions match and + # re-downloads the right build when they don't, so it makes the + # job resilient to Playwright revving its bundled chromium build + # in between cache writes. + npx playwright install chromium - name: Install Xvfb for headless Java AWT run: sudo apt-get update && sudo apt-get install -y xvfb