From ca0f5b3fc58502fdfd3331e0b9033572e6252ab2 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 5 Jun 2026 10:40:18 +0200 Subject: [PATCH] fix(ui): ensure usermenu always in viewport - ensure also use of only one method to position different contextmenus --- static/js/app/ui.js | 56 ++++------- static/js/components/mySharesList.js | 13 ++- static/js/utils/menuPosition.js | 137 +++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 47 deletions(-) create mode 100644 static/js/utils/menuPosition.js diff --git a/static/js/app/ui.js b/static/js/app/ui.js index d16854e5..68828898 100644 --- a/static/js/app/ui.js +++ b/static/js/app/ui.js @@ -14,6 +14,7 @@ import { fileOps } from '../features/files/fileOperations.js'; import { inlineViewer } from '../features/files/inlineViewer.js'; import { wopiEditor } from '../features/files/wopiEditor.js'; import { recent } from '../features/library/recent.js'; +import { positionMenu } from '../utils/menuPosition.js'; import { loadFiles } from './filesView.js'; import { updateHistory } from './main.js'; import { activateFilesUI, switchToFilesSection, syncViewContainers } from './navigation.js'; @@ -915,33 +916,24 @@ const ui = { */ showContextMenuForItem(item, e) { const trigger = /** @type {HTMLElement | null} */ (/** @type {HTMLElement} */ (e.target).closest('.file-actions')); + const menuId = 'mime_type' in item ? 'file-context-menu' : 'folder-context-menu'; if ('mime_type' in item) { app.contextMenuTargetFile = /** @type {FileItem} */ (item); - if (trigger) { - showContextMenuAtElement(trigger, 'file-context-menu'); - } else { - const menu = document.getElementById('file-context-menu'); - if (menu) { - menu.style.left = `${e.pageX}px`; - menu.style.top = `${e.pageY}px`; - contextMenus.sync(); - menu.classList.remove('hidden'); - } - } } else { app.contextMenuTargetFolder = /** @type {FolderItem} */ (item); - if (trigger) { - showContextMenuAtElement(trigger, 'folder-context-menu'); - } else { - const menu = document.getElementById('folder-context-menu'); - if (menu) { - menu.style.left = `${e.pageX}px`; - menu.style.top = `${e.pageY}px`; - contextMenus.sync(); - menu.classList.remove('hidden'); - } - } } + + if (trigger) { + showContextMenuAtElement(trigger, menuId); + return; + } + // Right-click on the row body with no kebab in scope — open at + // the cursor. positionMenu() clamps into the viewport, so menus + // near the bottom of the screen no longer overflow off-screen. + const menu = /** @type {HTMLElement | null} */ (document.getElementById(menuId)); + if (!menu) return; + contextMenus.sync(); + positionMenu(menu, { x: e.pageX, y: e.pageY }); }, /** @@ -1083,27 +1075,11 @@ function showContextMenuAtElement(triggerElement, menuId) { m.classList.add('hidden'); }); - const menu = document.getElementById(menuId); + const menu = /** @type {HTMLElement | null} */ (document.getElementById(menuId)); if (!menu) return; - const rect = triggerElement.getBoundingClientRect(); - const menuWidth = 200; // approximate - - // Position below the trigger, aligned to the right edge - let left = rect.right - menuWidth + window.scrollX; - let top = rect.bottom + 4 + window.scrollY; - - // Keep inside viewport - if (left < 8) left = 8; - if (top + 300 > window.innerHeight + window.scrollY) { - top = rect.top - 4 + window.scrollY; // flip above if no room - } - contextMenus.sync(); - - menu.style.left = `${left}px`; - menu.style.top = `${top}px`; - menu.classList.remove('hidden'); + positionMenu(menu, { anchor: triggerElement }); } /** diff --git a/static/js/components/mySharesList.js b/static/js/components/mySharesList.js index d2a2707b..e079837f 100644 --- a/static/js/components/mySharesList.js +++ b/static/js/components/mySharesList.js @@ -15,6 +15,7 @@ import { i18n } from '../core/i18n.js'; import { fileSharing } from '../features/sharing/fileSharing.js'; import { grants } from '../model/grants.js'; import { buildExpiryChip } from '../utils/expiryChip.js'; +import { positionMenu } from '../utils/menuPosition.js'; import { buildPasswordChip } from '../utils/passwordChip.js'; import { groupDisplayName, groupIconClass } from './groupDisplay.js'; import { createGroupVignette } from './groupVignette.js'; @@ -502,13 +503,11 @@ class MySharesList { document.body.appendChild(menu); - // Position below the trigger, right-aligned to it, clamped to viewport - const rect = btn.getBoundingClientRect(); - const mw = menu.offsetWidth || 200; - const left = Math.min(rect.right - mw, window.innerWidth - mw - 8); - menu.style.position = 'absolute'; - menu.style.top = `${rect.bottom + window.scrollY + 4}px`; - menu.style.left = `${Math.max(8, left)}px`; + // Position below the trigger, flipping above (or clamping up) + // when the trigger is too close to the bottom of the viewport. + // Single source of truth for menu positioning — see + // `static/js/utils/menuPosition.js`. + positionMenu(menu, { anchor: btn }); const close = (/** @type {Event} */ e) => { if (e.type === 'keydown' && /** @type {KeyboardEvent} */ (e).key !== 'Escape') return; diff --git a/static/js/utils/menuPosition.js b/static/js/utils/menuPosition.js new file mode 100644 index 00000000..245dfc07 --- /dev/null +++ b/static/js/utils/menuPosition.js @@ -0,0 +1,137 @@ +// @ts-check + +/** + * Single positioning engine for every floating menu in the app — + * file/folder context menus, the My Shares per-row action menu, the + * batch-toolbar "more" menu, and any future overlay that needs to sit + * near a trigger button or a click point. + * + * Replaces a sprawl of ad-hoc `style.top`/`style.left` formulas, none of + * which agreed on viewport clamping. The recurring bug fixed here: + * triggers near the bottom of the screen produced menus that overflowed + * off-screen because callers set `top = rect.bottom + 4` without + * checking whether the menu actually fit below. + * + * Resolution policy (anchor target): + * 1. Try below the anchor, right-aligned by default. + * 2. If the menu would overflow the viewport bottom AND there is more + * room above than below, flip to above the anchor. + * 3. Otherwise stay below and clamp the top so the menu fits. + * 4. Horizontally: clamp into `[margin, viewport - margin]`; the + * menu may shift left so the right-aligned default isn't a strict + * invariant when the trigger is near the right edge. + * + * Point target (right-click): open below-right of the cursor, then + * apply the same clamping. No flip — the user expects the menu near + * the click. + * + * Measurement note: callers may invoke this on a menu that is still + * `display:none` (via `.hidden`). We temporarily render it + * `visibility:hidden` so `offsetWidth`/`offsetHeight` reflect the + * actual rendered size, then leave the menu visible. The caller does + * NOT need to toggle `.hidden` before or after. + */ + +/** + * @typedef {Object} AnchorTarget + * @property {HTMLElement} anchor Trigger element (e.g. the ⋯ button). + * Menu opens below it by default, + * flipping above if it doesn't fit. + */ + +/** + * @typedef {Object} PointTarget + * @property {number} x Page-space X (e.g. from `MouseEvent.pageX`). + * @property {number} y Page-space Y (e.g. from `MouseEvent.pageY`). + */ + +/** + * @typedef {Object} PositionOpts + * @property {number} [margin=8] Min gap between the menu and any viewport edge. + * @property {'right'|'left'} [align='right'] + * Anchor mode only — which edge of the menu lines up with the anchor. + * `'right'` is the typical kebab/dropdown convention. + * @property {number} [gap=4] Vertical gap between menu and anchor edge. + */ + +/** + * Position a menu so it stays inside the viewport, anchored to a + * trigger element or a click point. The menu is left visible (its + * `.hidden` class, if present, is removed) and the caller can attach + * dismiss handlers as usual. + * + * @param {HTMLElement} menu + * @param {AnchorTarget | PointTarget} target + * @param {PositionOpts} [opts] + */ +export function positionMenu(menu, target, opts = {}) { + const margin = opts.margin ?? 8; + const gap = opts.gap ?? 4; + const align = opts.align ?? 'right'; + + // Ensure layout so we can measure. The caller may have passed a + // .hidden menu; render it invisibly first. + const wasHidden = menu.classList.contains('hidden'); + let restoreVisibility = null; + if (wasHidden) { + restoreVisibility = menu.style.visibility; + menu.style.visibility = 'hidden'; + menu.classList.remove('hidden'); + } + + // offsetWidth/Height fall back to a sane minimum if the menu has + // no content yet (shouldn't happen in practice; defensive only). + const mw = menu.offsetWidth || 200; + const mh = menu.offsetHeight || 200; + const vw = window.innerWidth; + const vh = window.innerHeight; + const sx = window.scrollX; + const sy = window.scrollY; + + let left; + let top; + + if ('anchor' in target) { + const rect = target.anchor.getBoundingClientRect(); + // Horizontal: right- or left-edge alignment with the trigger, + // converted to page space. + left = align === 'right' ? rect.right - mw + sx : rect.left + sx; + + // Vertical: prefer below; flip above when it doesn't fit and + // there's more room above. Both edges are still clamped below + // — the flip is a preference, not an absolute. + const spaceBelow = vh - rect.bottom; + const spaceAbove = rect.top; + const shouldFlip = mh + gap > spaceBelow && spaceAbove > spaceBelow; + top = shouldFlip ? rect.top - mh - gap + sy : rect.bottom + gap + sy; + } else { + left = target.x; + top = target.y; + } + + // Horizontal clamp. + const minLeft = sx + margin; + const maxLeft = sx + vw - mw - margin; + if (left > maxLeft) left = maxLeft; + if (left < minLeft) left = minLeft; + + // Vertical clamp. Fixes the off-screen bug: even after the + // anchor-flip heuristic, a very tall menu can still overflow the + // viewport. Push it up so its bottom edge sits at `viewport - + // margin`; if that pushes the top off the viewport, surrender and + // clamp at the top edge (the menu is taller than the viewport). + const minTop = sy + margin; + const maxTop = sy + vh - mh - margin; + if (top > maxTop) top = maxTop; + if (top < minTop) top = minTop; + + menu.style.position = 'absolute'; + menu.style.left = `${left}px`; + menu.style.top = `${top}px`; + + // Restore visibility (we want the menu shown, since the caller is + // about to wire its dismiss handlers). + if (wasHidden) { + menu.style.visibility = restoreVisibility ?? ''; + } +}