Harden CustomSelect against a stale open-time scroll dismissing the menu - #1490
Conversation
The footer CustomSelect dismisses its menu on any page scroll so the fixed listbox never detaches from its trigger. On small touch viewports, tapping the footer control first scrolls it into view; that scroll event can be delivered a frame after open() runs, even though the page already settled, so the freshly opened menu was immediately dismissed. This made the "language selector stays open while its listbox is scrolled" E2E flake on the mobile-chromium project (desktop/tablet scroll far less and never hit it). Capture the window scroll offset when the menu opens and ignore scroll events that report that same already-settled offset; only dismiss once the page actually moves underneath the menu. Existing behavior is preserved (scrolling inside the listbox keeps it open, a real page scroll and resize still close it), and a deterministic regression assertion covers the stale same-offset scroll. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the footer CustomSelect behavior so the language selector menu isn’t immediately dismissed by a stale, open-time scroll event (a race that was causing flaky mobile Playwright E2E failures). The fix distinguishes between “real” page scrolling and same-offset scroll events delivered after opening.
Changes:
- Capture
window.scrollX/scrollYat menu open time and ignore subsequent scroll events that report the same offset (within a small tolerance). - Split the previous combined viewport-change handler into dedicated
resize(always close) and guardedscroll(conditionally close) handlers. - Add an E2E regression assertion to ensure a same-offset
scrollevent does not close the open language selector.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frontend/src/components/CustomSelect.astro | Captures open-time scroll offsets and guards the window scroll handler to avoid immediate stale-scroll dismissal while preserving close-on-real-scroll behavior. |
| src/frontend/tests/e2e/ui-regressions.spec.ts | Adds a deterministic regression assertion to confirm the menu stays open when a same-offset window scroll event fires after opening. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Frontend HTML artifact readyThe latest frontend build uploaded the This comment updates automatically when a new frontend build artifact is uploaded. |
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
The stale window-scroll path works in real Chromium across desktop, mobile, and tablet projects, with 21 focused scenarios passing. I left one must-fix inline: ancestor element scrolls now keep the fixed listbox open and detached because window.scrollX/Y do not move. That looks fixable in place, so approving with the comment.
Address PR review feedback: - The open-time same-offset guard now only applies to page/document/window scrolls, whose position window.scrollX/Y tracks. An ancestor element scroll (e.g. a modal body) leaves window.scrollX/Y unchanged but still slides the trigger out from under the fixed listbox, so those now always close the menu instead of leaving it detached. - Extract the 2px offset tolerance into a named scrollSettleTolerancePx constant. - Add a deterministic E2E regression asserting an ancestor-element scroll dismisses the open language selector. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Hardens the footer language
CustomSelectagainst a stale, open-timescrollevent that could dismiss the menu the same frame it opened. This was surfacing as a flaky E2E test on the mobile-chromium project and intermittently red-lighting otherwise-unrelated chore PRs (e.g. #1487, run 31721890999).Root cause
CustomSelect.astroregisters a capture-phasewindowscrolllistener that dismisses the menu on any page scroll, so theposition: fixedlistbox never visually detaches from its trigger. That is the desired behavior for a real scroll — but on small touch viewports, Playwright's.click()first has to scroll the far-below-the-fold footer control into view. On the Pixel 7 viewport that scroll event can be delivered a frame afteropen()runs, even though the page has already settled, so the freshly opened menu was immediately closed. Thelanguage selector stays open while its listbox is scrolledtest then failed waiting for the listbox to be visible. Desktop (1440×900) and tablet (iPad Pro 11) scroll far less on the way to the footer and never hit the race.Fix
window.scrollX/scrollYwhen the menu opens.resizestill closes unconditionally whilescrolltakes the guarded path. The existing "scroll originates inside the listbox" guard is preserved.The change is deterministic rather than timing-based (no added waits/retries), so it hardens the behavior for real users too, not just the test.
Testing
astro syncclean (no type errors in the client script).pnpm test:unit:components— 62/62 pass.ui-regressions.spec.tsthat dispatches a same-offsetwindowscrollafter opening and asserts the menu stays open. This fails on the old handler and passes with the fix, on every project. Full E2E runs in CI.