diff options
Diffstat (limited to 'packages/meshbay-hub')
4 files changed, 198 insertions, 21 deletions
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/menu.js b/packages/meshbay-hub/src/meshbay_hub/static/menu.js index 16aecd8..12ca584 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/menu.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/menu.js @@ -58,11 +58,20 @@ function useMenu() { return { menu, openAt, close }; } -function MenuItems({ items, depth, onClose }) { +function MenuItems({ items, depth, onClose, onResize }) { const [expanded, setExpanded] = useState(null); // index -> the items that came back, or 'loading'. const [loaded, setLoaded] = useState({}); + // A submenu opening, and a lazily-read one arriving, are the only two things + // that change how tall the panel is — and `Menu` can see neither, because + // both are state in here. So they are reported rather than watched for: an + // observer on the panel would be an observer on the very box the callback + // resizes, which is a loop the browser breaks by raising an error. + useEffect(() => { + if (onResize) onResize(); + }, [expanded, loaded, onResize]); + const toggle = useCallback((i, it) => { if (expanded === i) { setExpanded(null); return; } setExpanded(i); @@ -95,7 +104,8 @@ function MenuItems({ items, depth, onClose }) { ? html`<div class="ctx-menu-empty" style=${`padding-left: ${28 + depth * 14}px`}><span class="spinner"></span></div>` : (rows && rows.length - ? html`<${MenuItems} items=${rows} depth=${depth + 1} onClose=${onClose} />` + ? html`<${MenuItems} items=${rows} depth=${depth + 1} + onClose=${onClose} onResize=${onResize} />` : html`<div class="ctx-menu-empty" style=${`padding-left: ${28 + depth * 14}px`}>${it.empty || ''}</div>`))} `; @@ -123,26 +133,46 @@ function MenuItems({ items, depth, onClose }) { function Menu({ x, y, items, onClose }) { const ref = useRef(null); const [place, setPlace] = useState(null); + // Bumped by the tree below when it changes height; a dependency of the + // measurement, and nothing else reads it. + const [grew, setGrew] = useState(0); + const remeasure = useCallback(() => setGrew((n) => n + 1), []); - // Measure, then place. A menu opened near the right or bottom edge has to - // flip rather than be clipped, and how tall it is depends on its own items — - // so the number cannot be written down, it has to be read. + // Measure, then place. A menu opened near an edge has to flip rather than be + // clipped, and how tall it is depends on its own items — so the numbers + // cannot be written down, they have to be read. // - // This is not the mutate-then-measure loop CLAUDE.md records: the effect - // writes `place`, which moves the panel, and `place` is not in its - // dependencies. Position does not change the size being measured, so it - // cannot wake itself. + // This is not the mutate-then-measure loop CLAUDE.md records. The height read + // is `scrollHeight`, the content's, which the `max-height` written back does + // not move; and `place` is not one of the dependencies. So the effect cannot + // wake itself. useEffect(() => { const el = ref.current; if (!el) return; - const r = el.getBoundingClientRect(); - const left = Math.max(MARGIN, Math.min(x, window.innerWidth - r.width - MARGIN)); - const top = (y + r.height > window.innerHeight - MARGIN) - ? Math.max(MARGIN, y - r.height) - : y; - setPlace({ left, top }); + // Content plus the panel's own borders — `box-sizing` is `border-box`, so + // those count against the `max-height` about to be set. + const h = el.scrollHeight + (el.offsetHeight - el.clientHeight); + const left = Math.max(MARGIN, + Math.min(x, window.innerWidth - el.offsetWidth - MARGIN)); + const below = window.innerHeight - MARGIN - y; + const above = y - MARGIN; + // Fits below, or below is simply the roomier side: stay at the pointer and + // scroll. Otherwise flip above it. + const top = (h <= below || below >= above) ? y : Math.max(MARGIN, y - h); + // And the panel may take exactly the room left below where it was just + // put. The stylesheet's `max-height` is a floor: it says how tall the panel + // 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. That is "the last track cannot be reached". + const maxHeight = Math.max(0, window.innerHeight - MARGIN - top); + setPlace((prev) => ((prev && prev.left === left && prev.top === top + && prev.maxHeight === maxHeight) ? prev : { left, top, maxHeight })); + // `grew` is a submenu reporting that it opened. Without it the panel keeps + // the placement measured for its collapsed self, and a two-row menu opened + // low threads sixty tracks through the room that was free beneath it. // eslint-disable-next-line - }, [x, y, items]); + }, [x, y, items, grew]); useEffect(() => { const onKey = (e) => { if (e.key === 'Escape') onClose(); }; @@ -183,11 +213,12 @@ function Menu({ x, y, items, onClose }) { return html` <div class="ctx-menu" ref=${ref} role="menu" style=${place - ? `left: ${place.left}px; top: ${place.top}px` + ? `left: ${place.left}px; top: ${place.top}px; max-height: ${place.maxHeight}px` // Drawn where asked but not yet shown, for the one frame it takes to // find out how big it is — otherwise it visibly jumps into place. : `left: ${x}px; top: ${y}px; visibility: hidden`}> - <${MenuItems} items=${items} depth=${0} onClose=${onClose} /> + <${MenuItems} items=${items} depth=${0} onClose=${onClose} + onResize=${remeasure} /> </div> `; } diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 5ef04b0..2d07fd1 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -5040,7 +5040,13 @@ h2 .gn-owner, h3 .gn-owner { font-size: 0.55em; } max-width: calc(100vw - 16px); /* A playlist's tracklist expands inside this panel rather than flying out sideways, so it can be far taller than the window. Same answer the - account menu's language list already has. */ + account menu's language list already has. + + A floor, not the real value: this says how tall the panel may be and + nothing about where its bottom lands, so a panel opened partway down the + window overruns it and its last rows end up in a region of itself that is + off-screen. `menu.js` measures where it put the panel and overrides this + with the room actually left below that point. */ max-height: calc(100vh - 16px); overflow-x: hidden; overflow-y: auto; 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}" |