From 17ddd4de9087a98c87bec28a6b773572b1c58b50 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 16 Sep 2026 16:26:16 +0200 Subject: menu: do not close on the panel's own scrolling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dismiss-on-scroll listener is on the capture phase, because `scroll` does not bubble — so it also heard the menu scrolling itself, and a long tracklist closed the moment it was wheeled. Filter on the event's origin. Co-Authored-By: Claude Opus 5 --- docs/playlists.md | 15 +- .../meshbay-hub/src/meshbay_hub/static/menu.js | 17 +- .../meshbay-hub/tests/harness/menu_scroll_probe.py | 216 +++++++++++++++++++++ packages/meshbay-hub/tests/test_menu_scroll.py | 76 ++++++++ 4 files changed, 318 insertions(+), 6 deletions(-) create mode 100755 packages/meshbay-hub/tests/harness/menu_scroll_probe.py create mode 100644 packages/meshbay-hub/tests/test_menu_scroll.py diff --git a/docs/playlists.md b/docs/playlists.md index 49b1585..c450815 100644 --- a/docs/playlists.md +++ b/docs/playlists.md @@ -808,10 +808,17 @@ The codebase has no context menu and no dropdown outside the account menu in button** — `icon.js` already has `dots` (`icon.js:54`). Both affordances, both platforms: the button is visible on hover on a fine pointer and always visible under `@media (pointer: coarse)`. -- closes on Escape, on an outside click, and **on scroll**. The last one is not - optional here: Music's toolbar is a sticky band and the grid scrolls beneath - it, so a menu that survives a scroll is a menu now anchored to a different - album than the one it was opened on. +- closes on Escape, on an outside click, and **on a scroll of the page**. The + last one is not optional here: Music's toolbar is a sticky band and the grid + scrolls beneath it, so a menu that survives a scroll is a menu now anchored to + a different album than the one it was opened on. **Never on a scroll of its + own panel** — the panel is `overflow-y: auto` and a tracklist is routinely + taller than the window, so the two must be told apart by where the event came + from. `scroll` does not bubble, so that listener is on the capture phase, + which is also what made it hear the panel's own scrolling: the menu closed the + instant it was wheeled or its scrollbar dragged, and no track below the fold + could be reached. Measured by `menu_scroll_probe.py`; `playlist_ui_probe.py` + cannot see it, because `.click()` scrolls nothing. - flips its anchor when it would open past the viewport edge, which on a phone is most of the time. - submenus **expand in place**, downward, at every width. This is the diff --git a/packages/meshbay-hub/src/meshbay_hub/static/menu.js b/packages/meshbay-hub/src/meshbay_hub/static/menu.js index 1c6a96a..16aecd8 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/menu.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/menu.js @@ -155,14 +155,27 @@ function Menu({ x, y, items, onClose }) { // sticky bands and the grid scrolls underneath them, so a menu that // survives a scroll is a menu still pointing at the album it was opened // on while sitting over a completely different one. + // + // But the panel also scrolls *itself*: a playlist's tracklist expands + // inside it and is routinely taller than the window. `scroll` does not + // bubble, which is why this listener is on the capture phase — and capture + // is equally what makes it hear the panel's own scrolling, on the way + // down. So the menu closed the instant it was scrolled, by wheel or by + // dragging its own scrollbar, and the track being reached for could not be + // reached at all. Ask where the scroll came from, not merely that one + // happened. + const onScroll = (e) => { + if (ref.current && e.target instanceof Node && ref.current.contains(e.target)) return; + onClose(); + }; document.addEventListener('keydown', onKey); document.addEventListener('mousedown', onDown); - window.addEventListener('scroll', onClose, true); + window.addEventListener('scroll', onScroll, true); window.addEventListener('resize', onClose); return () => { document.removeEventListener('keydown', onKey); document.removeEventListener('mousedown', onDown); - window.removeEventListener('scroll', onClose, true); + window.removeEventListener('scroll', onScroll, true); window.removeEventListener('resize', onClose); }; }, [onClose]); diff --git a/packages/meshbay-hub/tests/harness/menu_scroll_probe.py b/packages/meshbay-hub/tests/harness/menu_scroll_probe.py new file mode 100755 index 0000000..809d1b1 --- /dev/null +++ b/packages/meshbay-hub/tests/harness/menu_scroll_probe.py @@ -0,0 +1,216 @@ +#!/usr/bin/env python3 +""" +The pop-up menu, scrolled in a real browser. + +A playlist's tracklist expands inside `.ctx-menu`, which is `overflow-y: auto` +with a `max-height` — so a menu taller than the window is the ordinary case, +not an edge one, and scrolling it is the only way to reach the track being +removed. + +`playlist_ui_probe.py` cannot see this: it reaches every row with `.click()`, +which scrolls nothing. Neither can a source-reading test, because the question +is which listener a scroll event reaches. So this mounts the shipped `Menu`, +scrolls it the two ways a person can, and reads back whether it survived. + +The last case is the one that must keep failing to close: a scroll of the +*page* has to dismiss the menu, or it ends up pointing at an album that has +moved out from under it. + + menu_scroll_probe.py + +Prints JSON: one entry per case. +""" + +import http.server +import json +import socketserver +import subprocess +import sys +import tempfile +import threading +import time +from pathlib import Path + +STATIC = Path(__file__).resolve().parents[2] / "src" / "meshbay_hub" / "static" +PORT = 8757 +RECORDS = [] +socketserver.TCPServer.allow_reuse_address = True + +FRAME = r""" + +
+ +
+""" + +PAGE = r""" +
""" + + +class H(http.server.BaseHTTPRequestHandler): + def log_message(self, *a): + pass + + def do_POST(self): + length = int(self.headers.get("Content-Length") or 0) + if self.path == "/log": + RECORDS.append(json.loads(self.rfile.read(length).decode())) + else: + self.rfile.read(length) + self.send_response(204) + self.end_headers() + + def _send(self, body: bytes, ctype: str) -> None: + self.send_response(200) + self.send_header("Content-Type", ctype) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def do_GET(self): + path = self.path.split("?")[0] + if path == "/": + self._send(PAGE.encode(), "text/html; charset=utf-8") + elif path == "/case": + self._send(FRAME.encode(), "text/html; charset=utf-8") + else: + asset = (STATIC / path.lstrip("/")).resolve() + if not str(asset).startswith(str(STATIC)) or not asset.is_file(): + self.send_response(404) + self.end_headers() + return + self._send(asset.read_bytes(), + "text/css" if asset.suffix == ".css" + else "text/javascript" if asset.suffix == ".js" + else "application/octet-stream") + + +def main() -> int: + with socketserver.TCPServer(("127.0.0.1", PORT), H) as srv: + threading.Thread(target=srv.serve_forever, daemon=True).start() + with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as profile: + proc = subprocess.Popen( + ["google-chrome", "--headless=new", "--disable-gpu", "--no-sandbox", + f"--user-data-dir={profile}", "--window-size=1100,900", + f"http://127.0.0.1:{PORT}/"], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + for _ in range(300): + if RECORDS: + break + time.sleep(0.1) + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + if not RECORDS: + print(json.dumps({"error": "no measurement"}), file=sys.stderr) + return 1 + print(json.dumps(RECORDS[0], indent=1)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/packages/meshbay-hub/tests/test_menu_scroll.py b/packages/meshbay-hub/tests/test_menu_scroll.py new file mode 100644 index 0000000..5b680a4 --- /dev/null +++ b/packages/meshbay-hub/tests/test_menu_scroll.py @@ -0,0 +1,76 @@ +""" +A pop-up menu taller than the window can be scrolled without dismissing itself. + +`.ctx-menu` is `overflow-y: auto` under a `max-height`, and a playlist's +tracklist expands *inside* it — sixty tracks is three times the panel's height, +so scrolling is the only way to reach most of them. Both ways of scrolling it +closed it instead: the wheel, and dragging its own scrollbar. Reported against +"remove a track", where the menu vanished before a track could be clicked. + +`scroll` does not bubble, so the listener that dismisses the menu on a page +scroll is on the capture phase — and capture is equally what made it hear the +panel's own scrolling on the way down. + +Measured, not read. `playlist_ui_probe.py` drives the same menu and cannot see +this: it reaches every row with `.click()`, which scrolls nothing at all. + +The last two cases are the ones that must keep closing the menu, and they are +why the fix is a filter on the event's origin rather than a removed listener. +""" + +import json +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +HARNESS = Path(__file__).parent / "harness" / "menu_scroll_probe.py" + + +@pytest.fixture(scope="module") +def cases(): + if shutil.which("google-chrome") is None: + pytest.skip("Chrome is not available") + proc = subprocess.run([sys.executable, str(HARNESS)], + capture_output=True, text=True, timeout=90) + data = json.loads(proc.stdout) + assert "error" not in data, f"probe failed: {proc.stdout}{proc.stderr}" + return {c["case"]: c for c in data["cases"]} + + +def test_the_fixture_actually_overflows(cases): + # A menu that fits measures nothing, and would let every assertion below + # pass with the fix removed. + c = cases["the panel is scrollable at all"] + assert c["overflows"], f"60 rows did not overflow the panel: {c}" + + +def test_the_wheel_scrolls_the_menu_instead_of_closing_it(cases): + c = cases["scrolled inside the menu"] + assert c["stillOpen"], "the menu closed when it was scrolled" + assert c["scrollTop"] == 200, "the menu did not scroll" + + +def test_dragging_the_menus_own_scrollbar_does_not_close_it(cases): + c = cases["pressed the menu scrollbar and dragged"] + assert c["stillOpen"], "pressing the scrollbar closed the menu" + assert c["scrollTop"] == 400, "the drag did not scroll" + + +def test_reaching_the_end_does_not_scroll_the_page_behind(cases): + # Chained to the page, that overscroll *is* a page scroll, and a page + # scroll closes the menu on purpose — the same symptom by another door. + assert cases["the panel is scrollable at all"]["overscrollBehaviorY"] == "contain" + + +def test_a_press_outside_still_closes_it(cases): + assert cases["pressed outside the menu"]["closed"] + + +def test_scrolling_the_page_still_closes_it(cases): + # Not optional: the menu is `position: fixed` and the media grids scroll + # under a sticky toolbar, so a menu that survives leaves itself pointing at + # an album that has moved. + assert cases["scrolled the page underneath"]["closed"] -- cgit v1.2.3