From e2fedc92a789db4c1fe5d6575108cc43265a10a4 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 16 Sep 2026 16:42:00 +0200 Subject: menu: give the panel the room that is actually below it `max-height: calc(100vh - 16px)` says how tall the menu may be and nothing about where its bottom lands, so one opened partway down the window ran past it and its last rows scrolled out of reach. Measure from where the panel was placed, and re-measure when a submenu opens. Co-Authored-By: Claude Opus 5 --- .../meshbay-hub/tests/harness/menu_scroll_probe.py | 76 +++++++++++++++++++++- packages/meshbay-hub/tests/test_menu_scroll.py | 68 ++++++++++++++++++- 2 files changed, 142 insertions(+), 2 deletions(-) (limited to 'packages/meshbay-hub/tests') diff --git a/packages/meshbay-hub/tests/harness/menu_scroll_probe.py b/packages/meshbay-hub/tests/harness/menu_scroll_probe.py index 809d1b1..5fb9579 100755 --- a/packages/meshbay-hub/tests/harness/menu_scroll_probe.py +++ b/packages/meshbay-hub/tests/harness/menu_scroll_probe.py @@ -119,7 +119,81 @@ let closed = false; } cases.push({ case: 'pressed outside the menu', closed }); - // 4. And so does a scroll of the page. Not optional: the menu is fixed, so + // 4. Opened partway down the window, the panel must end *inside* it. + // Scrolling a panel whose own bottom is off-screen moves its last rows + // into a region of itself that no scroll can bring back, which is what + // "the last track cannot be reached" was. + closed = false; + render(html`<${Menu} x=${300} y=${300} items=${ITEMS} onClose=${onClose} />`, + document.getElementById('root')); + await frame(); + const low = document.querySelector('.ctx-menu'); + low.scrollTop = low.scrollHeight; + await frame(); + const lowRows = low.querySelectorAll('.ctx-menu-item'); + const lowLast = lowRows[lowRows.length - 1]; + cases.push({ case: 'a long menu opened partway down the window', + openedAt: 300, viewport: window.innerHeight, + panelTop: Math.round(low.getBoundingClientRect().top), + panelBottom: Math.round(low.getBoundingClientRect().bottom), + lastRowBottom: Math.round(lowLast.getBoundingClientRect().bottom), + lastRowLabel: lowLast.textContent.trim(), + overflows: low.scrollHeight > low.clientHeight + 1 }); + + // 5. And it must still end inside it after a submenu has expanded, which + // is a size change happening in `MenuItems`' own state — the placement + // effect's dependencies cannot see it. + // + // Opened low on purpose. A two-row menu 600px down has 192px of room + // beneath it, and that is the height the panel is given; expanding a + // sixty-track list into it must re-place the panel rather than thread + // the list through the porthole the collapsed one was measured for. + const NESTED = [ + { key: 'load', label: 'Load a playlist', items: ITEMS.slice(0, 3) }, + { key: 'remove', label: 'Remove a track', items: ITEMS }, + ]; + render(html`<${Menu} x=${300} y=${600} items=${NESTED} onClose=${onClose} />`, + document.getElementById('root')); + await frame(); + const nested = document.querySelector('.ctx-menu'); + const collapsedBottom = Math.round(nested.getBoundingClientRect().bottom); + const toggle = Array.from(nested.querySelectorAll('.ctx-menu-item')) + .find((b) => b.textContent.indexOf('Remove a track') >= 0); + if (!toggle) return fail('no submenu row to expand'); + toggle.click(); + await frame(); + await sleep(60); + await frame(); + const open = document.querySelector('.ctx-menu'); + open.scrollTop = open.scrollHeight; + await frame(); + const openRows = open.querySelectorAll('.ctx-menu-item'); + const openLast = openRows[openRows.length - 1]; + cases.push({ case: 'a submenu expanded inside it', + openedAt: 600, viewport: window.innerHeight, + collapsedBottom, + panelHeight: Math.round(open.getBoundingClientRect().height), + panelTop: Math.round(open.getBoundingClientRect().top), + panelBottom: Math.round(open.getBoundingClientRect().bottom), + lastRowBottom: Math.round(openLast.getBoundingClientRect().bottom), + lastRowLabel: openLast.textContent.trim(), + overflows: open.scrollHeight > open.clientHeight + 1 }); + + // 6. A short menu opened at the very bottom still flips above the pointer + // rather than being pinned there and scrolled — the condition deciding + // that was rewritten, so it is measured rather than assumed. + const SHORT = ITEMS.slice(0, 4); + render(html`<${Menu} x=${300} y=${780} items=${SHORT} onClose=${onClose} />`, + document.getElementById('root')); + await frame(); + const short = document.querySelector('.ctx-menu'); + cases.push({ case: 'a short menu opened at the bottom edge', + openedAt: 780, viewport: window.innerHeight, + panelTop: Math.round(short.getBoundingClientRect().top), + panelBottom: Math.round(short.getBoundingClientRect().bottom), + overflows: short.scrollHeight > short.clientHeight + 1 }); + + // 7. And so does a scroll of the page. Not optional: the menu is fixed, so // a grid scrolling under it leaves it pointing at the wrong album. render(html`<${Menu} x=${300} y=${40} items=${ITEMS} onClose=${onClose} />`, document.getElementById('root')); diff --git a/packages/meshbay-hub/tests/test_menu_scroll.py b/packages/meshbay-hub/tests/test_menu_scroll.py index 5b680a4..b56a053 100644 --- a/packages/meshbay-hub/tests/test_menu_scroll.py +++ b/packages/meshbay-hub/tests/test_menu_scroll.py @@ -1,5 +1,8 @@ """ -A pop-up menu taller than the window can be scrolled without dismissing itself. +A pop-up menu taller than the window can be scrolled, and scrolled all the way. + +Two defects, one symptom — "the last track cannot be reached" — and they had to +be fixed in that order, because the first hides the second. `.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, @@ -16,6 +19,14 @@ 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. + +The second defect is where the panel ends. `max-height: calc(100vh - 16px)` +says how tall it may be and nothing about where its bottom lands, so a panel +opened 300px down the window ran 300px past the bottom of it. It scrolled — but +its last rows scrolled into a part of itself that is off the screen, which no +further scrolling brings back. `menu.js` measures the room left below where it +put the panel. The flip case is measured too, because the condition deciding it +was rewritten in the same breath. """ import json @@ -74,3 +85,58 @@ def test_scrolling_the_page_still_closes_it(cases): # 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"] + + +def test_a_long_menu_ends_inside_the_window(cases): + c = cases["a long menu opened partway down the window"] + assert c["overflows"], f"the fixture measures nothing: {c}" + assert c["panelBottom"] <= c["viewport"], ( + f"the panel runs {c['panelBottom'] - c['viewport']}px past the bottom " + f"of the window: {c}") + + +def test_the_last_row_of_a_long_menu_can_be_reached(cases): + # Scrolled to the end, the last row has to be *visible*. This is the + # user-facing sentence, and the one that fails while the panel overruns the + # window: the rows are there, scrolled into a region nothing can show. + c = cases["a long menu opened partway down the window"] + assert c["lastRowLabel"].startswith("Track 60"), c + assert c["lastRowBottom"] <= c["viewport"], ( + f"the last row sits {c['lastRowBottom'] - c['viewport']}px below the " + f"window after scrolling to the end: {c}") + + +def test_it_still_opens_at_the_pointer_when_the_room_is_below(cases): + # The cure must not be a menu that jumps somewhere else: a list too tall + # for any placement stays where it was asked for and scrolls. + c = cases["a long menu opened partway down the window"] + assert c["panelTop"] == c["openedAt"], c + + +def test_an_expanded_submenu_stays_inside_the_window(cases): + c = cases["a submenu expanded inside it"] + assert c["panelBottom"] > c["collapsedBottom"], ( + f"the submenu did not actually expand: {c}") + assert c["panelBottom"] <= c["viewport"], c + assert c["lastRowLabel"].startswith("Track 60"), c + assert c["lastRowBottom"] <= c["viewport"], c + + +def test_an_expanded_submenu_is_re_placed_rather_than_left_in_its_slot(cases): + # The size change happens in `MenuItems`' own state, which the placement + # effect cannot see on its own — this is what the report upward is for. + # Without it the panel keeps the height measured for its collapsed self: + # the menu stays inside the window, and a sixty-track list is threaded + # through the 192px that were free below where it was opened. + c = cases["a submenu expanded inside it"] + assert c["panelHeight"] > c["viewport"] - c["openedAt"], ( + f"the panel is still the size the collapsed menu was given: {c}") + + +def test_a_short_menu_at_the_bottom_edge_still_flips(cases): + # No regression on the placement that already worked: a menu that fits + # above the pointer is drawn there whole, not pinned to the edge and + # squeezed into the few pixels left below it. + c = cases["a short menu opened at the bottom edge"] + assert c["panelBottom"] <= c["openedAt"], c + assert not c["overflows"], f"a four-item menu should not need scrolling: {c}" -- cgit v1.2.3