From 581e99daf32a1e84688f9ce68753785629abb16e Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Thu, 16 Oct 2025 16:29:33 -0400 Subject: [PATCH 1/3] Preserve scroll position during popover focus --- js/ui/overlay/popoverEngine.js | 46 ++++++++++++++++++--------------- tests/ui/popoverEngine.test.mjs | 41 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 21 deletions(-) diff --git a/js/ui/overlay/popoverEngine.js b/js/ui/overlay/popoverEngine.js index f3764129..220f10bb 100644 --- a/js/ui/overlay/popoverEngine.js +++ b/js/ui/overlay/popoverEngine.js @@ -243,6 +243,26 @@ export function createPopover(trigger, render, options = {}) { return documentRef?.defaultView || globalThis; } + function focusWithoutScroll(node) { + if (!node || typeof node.focus !== "function") { + return; + } + + const view = documentRef?.defaultView || null; + const scrollX = view?.scrollX ?? view?.pageXOffset ?? 0; + const scrollY = view?.scrollY ?? view?.pageYOffset ?? 0; + + try { + node.focus({ preventScroll: true }); + } catch (error) { + node.focus(); + } finally { + if (view && typeof view.scrollTo === "function") { + view.scrollTo(scrollX, scrollY); + } + } + } + function resetTypeaheadBuffer() { const view = getView(); if (menuState.typeaheadTimeout) { @@ -337,11 +357,7 @@ export function createPopover(trigger, render, options = {}) { updateAriaActiveDescendant(panelElement, target); if (focus) { - try { - target.focus({ preventScroll: true }); - } catch (error) { - target.focus(); - } + focusWithoutScroll(target); } } @@ -363,11 +379,7 @@ export function createPopover(trigger, render, options = {}) { if (focusPanelFallback) { panelElement.setAttribute("tabindex", panelElement.getAttribute("tabindex") || "-1"); - try { - panelElement.focus({ preventScroll: true }); - } catch (error) { - panelElement.focus(); - } + focusWithoutScroll(panelElement); } } @@ -837,20 +849,12 @@ export function createPopover(trigger, render, options = {}) { function restoreTriggerFocus() { if (anchor && typeof anchor.focus === "function") { - try { - anchor.focus({ preventScroll: true }); - return; - } catch (error) { - anchor.focus(); - } + focusWithoutScroll(anchor); + return; } if (previousActiveElement && typeof previousActiveElement.focus === "function") { - try { - previousActiveElement.focus({ preventScroll: true }); - } catch (error) { - previousActiveElement.focus(); - } + focusWithoutScroll(previousActiveElement); } } diff --git a/tests/ui/popoverEngine.test.mjs b/tests/ui/popoverEngine.test.mjs index f6303f50..84e771c0 100644 --- a/tests/ui/popoverEngine.test.mjs +++ b/tests/ui/popoverEngine.test.mjs @@ -53,6 +53,9 @@ beforeEach(() => { global.Node = windowRef.Node; global.getComputedStyle = windowRef.getComputedStyle.bind(windowRef); global.ResizeObserver = StubObserver; + global.IntersectionObserver = StubObserver; + windowRef.IntersectionObserver = StubObserver; + windowRef.scrollTo = () => {}; if (!global.PointerEvent) { global.PointerEvent = windowRef.PointerEvent || windowRef.Event; } @@ -66,6 +69,7 @@ afterEach(() => { delete global.Node; delete global.getComputedStyle; delete global.ResizeObserver; + delete global.IntersectionObserver; if (dom) { dom.window.close(); } @@ -209,6 +213,31 @@ test("supports roving focus, home/end navigation, and typeahead", async () => { height: 40, }); + const initialScroll = { x: 64, y: 128 }; + windowRef.scrollX = initialScroll.x; + windowRef.scrollY = initialScroll.y; + + const scrollCalls = []; + const originalScrollTo = windowRef.scrollTo; + windowRef.scrollTo = (x, y) => { + scrollCalls.push({ x, y }); + windowRef.scrollX = x; + windowRef.scrollY = y; + }; + + const focusMutations = []; + const originalFocus = windowRef.HTMLElement.prototype.focus; + const registerFocusMutation = (element, mutatedX, mutatedY) => { + element.focus = function focus(options = {}) { + focusMutations.push({ id: element.id || element.textContent, options }); + windowRef.scrollX = mutatedX; + windowRef.scrollY = mutatedY; + if (typeof originalFocus === "function") { + originalFocus.call(this, options); + } + }; + }; + const popover = createPopover( trigger, ({ container }) => { @@ -239,18 +268,21 @@ test("supports roving focus, home/end navigation, and typeahead", async () => { copy.className = "menu__item"; copy.type = "button"; copy.textContent = "Copy link"; + registerFocusMutation(copy, 400, 500); list.appendChild(copy); const deleteBtn = documentRef.createElement("button"); deleteBtn.className = "menu__item"; deleteBtn.type = "button"; deleteBtn.textContent = "Delete video"; + registerFocusMutation(deleteBtn, 420, 520); list.appendChild(deleteBtn); const share = documentRef.createElement("button"); share.className = "menu__item"; share.type = "button"; share.textContent = "Share"; + registerFocusMutation(share, 440, 540); list.appendChild(share); container.appendChild(panel); @@ -307,7 +339,16 @@ test("supports roving focus, home/end navigation, and typeahead", async () => { assert.equal(documentRef.activeElement, deleteBtn); assert.equal(panel.getAttribute("aria-activedescendant"), deleteBtn.id); + assert.ok(focusMutations.length >= 1, "focus mutations should occur"); + assert.ok(scrollCalls.length >= focusMutations.length); + scrollCalls.forEach((call) => { + assert.deepEqual(call, initialScroll); + }); + assert.equal(windowRef.scrollX, initialScroll.x); + assert.equal(windowRef.scrollY, initialScroll.y); + popover.destroy(); + windowRef.scrollTo = originalScrollTo; }); test("escape closes the popover and restores trigger focus", async () => { From 2b65a85335e058a0a0980ffeaab25c3a5ab57f3f Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Thu, 16 Oct 2025 16:35:06 -0400 Subject: [PATCH 2/3] Improve popover focus scroll restoration --- js/ui/overlay/popoverEngine.js | 32 +++++++++++++++++++++++++++----- tests/ui/popoverEngine.test.mjs | 25 +++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/js/ui/overlay/popoverEngine.js b/js/ui/overlay/popoverEngine.js index 220f10bb..138b7d9c 100644 --- a/js/ui/overlay/popoverEngine.js +++ b/js/ui/overlay/popoverEngine.js @@ -248,18 +248,40 @@ export function createPopover(trigger, render, options = {}) { return; } - const view = documentRef?.defaultView || null; + const view = getView(); const scrollX = view?.scrollX ?? view?.pageXOffset ?? 0; const scrollY = view?.scrollY ?? view?.pageYOffset ?? 0; + let preventScrollSupported = true; + try { node.focus({ preventScroll: true }); } catch (error) { + preventScrollSupported = false; node.focus(); - } finally { - if (view && typeof view.scrollTo === "function") { - view.scrollTo(scrollX, scrollY); - } + } + + if (!view || typeof view.scrollTo !== "function") { + return; + } + + const restoreScroll = () => { + view.scrollTo(scrollX, scrollY); + }; + + restoreScroll(); + + if (preventScrollSupported) { + return; + } + + if (typeof view.requestAnimationFrame === "function") { + view.requestAnimationFrame(restoreScroll); + return; + } + + if (typeof view.setTimeout === "function") { + view.setTimeout(restoreScroll, 0); } } diff --git a/tests/ui/popoverEngine.test.mjs b/tests/ui/popoverEngine.test.mjs index 84e771c0..cc653ad1 100644 --- a/tests/ui/popoverEngine.test.mjs +++ b/tests/ui/popoverEngine.test.mjs @@ -219,17 +219,36 @@ test("supports roving focus, home/end navigation, and typeahead", async () => { const scrollCalls = []; const originalScrollTo = windowRef.scrollTo; + const originalRequestAnimationFrame = windowRef.requestAnimationFrame; + const rafCalls = []; windowRef.scrollTo = (x, y) => { scrollCalls.push({ x, y }); windowRef.scrollX = x; windowRef.scrollY = y; }; + windowRef.requestAnimationFrame = (callback) => { + rafCalls.push(true); + if (typeof callback === "function") { + callback(); + } + return rafCalls.length; + }; const focusMutations = []; const originalFocus = windowRef.HTMLElement.prototype.focus; - const registerFocusMutation = (element, mutatedX, mutatedY) => { + const registerFocusMutation = ( + element, + mutatedX, + mutatedY, + { supportsPreventScroll = true } = {}, + ) => { element.focus = function focus(options = {}) { focusMutations.push({ id: element.id || element.textContent, options }); + + if (!supportsPreventScroll && options && typeof options === "object" && "preventScroll" in options) { + throw new TypeError("preventScroll not supported"); + } + windowRef.scrollX = mutatedX; windowRef.scrollY = mutatedY; if (typeof originalFocus === "function") { @@ -268,7 +287,7 @@ test("supports roving focus, home/end navigation, and typeahead", async () => { copy.className = "menu__item"; copy.type = "button"; copy.textContent = "Copy link"; - registerFocusMutation(copy, 400, 500); + registerFocusMutation(copy, 400, 500, { supportsPreventScroll: false }); list.appendChild(copy); const deleteBtn = documentRef.createElement("button"); @@ -344,11 +363,13 @@ test("supports roving focus, home/end navigation, and typeahead", async () => { scrollCalls.forEach((call) => { assert.deepEqual(call, initialScroll); }); + assert.ok(rafCalls.length >= 1, "should schedule scroll restoration when preventScroll unsupported"); assert.equal(windowRef.scrollX, initialScroll.x); assert.equal(windowRef.scrollY, initialScroll.y); popover.destroy(); windowRef.scrollTo = originalScrollTo; + windowRef.requestAnimationFrame = originalRequestAnimationFrame; }); test("escape closes the popover and restores trigger focus", async () => { From f9455db2a7a33ba840ccbbf6247765a53be2dbca Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Thu, 16 Oct 2025 16:43:58 -0400 Subject: [PATCH 3/3] Stop closing menus from refocusing unrelated cards --- js/ui/components/VideoCard.js | 32 ++++-- tests/video-card-source-visibility.test.mjs | 111 ++++++++++++++++++++ 2 files changed, 137 insertions(+), 6 deletions(-) diff --git a/js/ui/components/VideoCard.js b/js/ui/components/VideoCard.js index a5c7ee9f..657221d3 100644 --- a/js/ui/components/VideoCard.js +++ b/js/ui/components/VideoCard.js @@ -226,8 +226,13 @@ export class VideoCard { closeMoreMenu(options = {}) { const restoreFocus = options?.restoreFocus !== false; + const trigger = this.moreMenuButton; + const wasExpanded = + typeof trigger?.getAttribute === "function" && + trigger.getAttribute("aria-expanded") === "true"; + const detail = { - trigger: this.moreMenuButton, + trigger, video: this.video, card: this, restoreFocus, @@ -247,9 +252,14 @@ export class VideoCard { } } - if (!handled && restoreFocus && this.moreMenuButton?.focus) { + if ( + !handled && + restoreFocus && + wasExpanded && + typeof trigger?.focus === "function" + ) { try { - this.moreMenuButton.focus(); + trigger.focus(); } catch (error) { /* noop */ } @@ -258,8 +268,13 @@ export class VideoCard { closeSettingsMenu(options = {}) { const restoreFocus = options?.restoreFocus !== false; + const trigger = this.settingsButton; + const wasExpanded = + typeof trigger?.getAttribute === "function" && + trigger.getAttribute("aria-expanded") === "true"; + const detail = { - trigger: this.settingsButton, + trigger, video: this.video, card: this, restoreFocus, @@ -279,9 +294,14 @@ export class VideoCard { } } - if (!handled && restoreFocus && this.settingsButton?.focus) { + if ( + !handled && + restoreFocus && + wasExpanded && + typeof trigger?.focus === "function" + ) { try { - this.settingsButton.focus(); + trigger.focus(); } catch (error) { /* noop */ } diff --git a/tests/video-card-source-visibility.test.mjs b/tests/video-card-source-visibility.test.mjs index 93e48610..e56020f4 100644 --- a/tests/video-card-source-visibility.test.mjs +++ b/tests/video-card-source-visibility.test.mjs @@ -139,3 +139,114 @@ test("VideoCard hides cards without playable sources until a healthy CDN update assert.equal(root.hidden, false, "a healthy CDN update should unhide the card"); assert.equal(root.dataset.sourceVisibility, "visible", "visibility dataset should reflect the restored state"); }); + +test("VideoCard.closeMoreMenu only restores focus when the trigger was expanded", (t) => { + const { document } = setupDom(t); + + const card = new VideoCard({ + document, + video: { + id: "event-focus", + title: "Focusable clip", + }, + formatters: { + formatTimeAgo: () => "just now", + }, + helpers: { + isMagnetSupported: () => true, + }, + }); + + const root = card.getRoot(); + document.body.appendChild(root); + + const trigger = card.moreMenuButton; + assert.ok(trigger, "the more menu trigger should exist"); + + let focusCalls = 0; + trigger.focus = () => { + focusCalls += 1; + }; + + card.onCloseMoreMenu = () => false; + + card.closeMoreMenu(); + assert.equal( + focusCalls, + 0, + "collapsed menus should not restore focus when closing", + ); + + trigger.setAttribute("aria-expanded", "true"); + card.closeMoreMenu(); + assert.equal( + focusCalls, + 1, + "expanded menus should restore focus back to the trigger", + ); + + trigger.setAttribute("aria-expanded", "true"); + card.closeMoreMenu({ restoreFocus: false }); + assert.equal( + focusCalls, + 1, + "restoreFocus=false should suppress focus restoration", + ); +}); + +test("VideoCard.closeSettingsMenu only restores focus when the trigger was expanded", (t) => { + const { document } = setupDom(t); + + const card = new VideoCard({ + document, + video: { + id: "event-settings", + title: "Editable clip", + }, + capabilities: { + canEdit: true, + }, + formatters: { + formatTimeAgo: () => "just now", + }, + helpers: { + isMagnetSupported: () => true, + }, + }); + + const root = card.getRoot(); + document.body.appendChild(root); + + const trigger = card.settingsButton; + assert.ok(trigger, "the settings trigger should exist when canEdit is true"); + + let focusCalls = 0; + trigger.focus = () => { + focusCalls += 1; + }; + + card.onCloseSettingsMenu = () => false; + + card.closeSettingsMenu(); + assert.equal( + focusCalls, + 0, + "collapsed settings menus should not restore focus when closing", + ); + + trigger.setAttribute("aria-expanded", "true"); + card.closeSettingsMenu(); + assert.equal( + focusCalls, + 1, + "expanded settings menus should restore focus back to the trigger", + ); + + trigger.setAttribute("aria-expanded", "true"); + card.closeSettingsMenu({ restoreFocus: false }); + assert.equal( + focusCalls, + 1, + "restoreFocus=false should suppress settings focus restoration", + ); +});