diff --git a/imports.js b/imports.js index 94c5a960..0f975c7c 100644 --- a/imports.js +++ b/imports.js @@ -17,3 +17,4 @@ export * as Topbar from './topbar.js'; export * as Utils from './utils.js'; export * as Workspace from './workspace.js'; export * as OverviewLayout from './overviewlayout.js'; +export * as PopupUtil from './popuputil.js'; diff --git a/popuputil.js b/popuputil.js new file mode 100644 index 00000000..6f6a569d --- /dev/null +++ b/popuputil.js @@ -0,0 +1,50 @@ +/** + * Pure (shell-free) helpers for positioning popup/transient windows. + * + * Deliberately free of `gi://` and gnome-shell imports so it can be unit-tested + * with standalone GJS (tests/test-popup-visibility.js). `tiling.js`'s + * `ensureVisibleInWorkArea` is a thin adapter that feeds live MetaWindow / + * Space objects into these functions. + */ + +/** + * Reconcile a Space's monitor-relative workArea into screen-absolute bounds. + * + * `Space.workArea()` returns monitor-relative coordinates (it subtracts + * `monitor.x` / `monitor.y`), while `MetaWindow.get_frame_rect()` and + * `move_frame()` are screen-absolute. Folding the monitor origin back in here + * avoids relocating windows to the wrong monitor on multi-head. + * + * @param {{x:number,y:number}} monitor the monitor's screen-absolute origin + * @param {{x:number,y:number,width:number,height:number}} workArea monitor-relative + * @returns {{x:number,y:number,width:number,height:number}} + */ +export function workAreaToBounds(monitor, workArea) { + return { + x: monitor.x + workArea.x, + y: monitor.y + workArea.y, + width: workArea.width, + height: workArea.height, + }; +} + +/** + * Clamp a frame rectangle into bounds, returning the target {x, y}. + * + * A window that fits is shifted minimally so it is fully inside bounds. One + * that is larger than bounds on an axis is pinned to the leading edge + * (minX / minY) rather than allowed to go negative. + * + * @param {{x:number,y:number,width:number,height:number}} frame + * @param {{x:number,y:number,width:number,height:number}} bounds + * @returns {{x:number,y:number}} + */ +export function computeClampedPosition(frame, bounds) { + const minX = bounds.x; + const maxX = bounds.x + bounds.width - frame.width; + const minY = bounds.y; + const maxY = bounds.y + bounds.height - frame.height; + const x = Math.max(minX, Math.min(frame.x, maxX)); + const y = Math.max(minY, Math.min(frame.y, maxY)); + return { x, y }; +} diff --git a/tests/test-popup-visibility.js b/tests/test-popup-visibility.js new file mode 100644 index 00000000..c243a8ec --- /dev/null +++ b/tests/test-popup-visibility.js @@ -0,0 +1,150 @@ +#!/usr/bin/env gjs +/* + * Unit tests for the pure popup-positioning helpers (popuputil.js). + * + * Run: gjs -m tests/test-popup-visibility.js + * + * These cover the shell-free pieces only — the coordinate reconciliation + * (workArea is monitor-relative, frame/move_frame are screen-absolute) and the + * clamp arithmetic (incl. the oversize edge case). The shell-facing wrappers in + * tiling.js (ensureVisibleInWorkArea, the focus / demands-attention entry + * points, the isPopupClass predicate) need a live gnome-shell session and are + * exercised manually. + */ + +import { workAreaToBounds, computeClampedPosition } from '../popuputil.js'; +import system from 'system'; + +let _passed = 0, _failed = 0; +const _failures = []; + +function check(name, cond, detail) { + if (cond) { + _passed++; + } else { + _failed++; + _failures.push(detail ? `${name} — ${detail}` : name); + print(` FAIL: ${name}${detail ? ` — ${detail}` : ''}`); + } +} + +function rectEq(a, b) { + return Math.round(a.x) === Math.round(b.x) && Math.round(a.y) === Math.round(b.y); +} + +// ─── workAreaToBounds (the coordinate bug: monitor-relative vs screen-absolute) ─ + +print('\n== workAreaToBounds (coordinate reconciliation) =='); + +{ + const mon = { x: 0, y: 0 }; + const wa = { x: 0, y: 12, width: 1920, height: 1068 }; + const b = workAreaToBounds(mon, wa); + check('primary monitor keeps origin', b.x === 0 && b.y === 12, + `got x=${b.x} y=${b.y}`); + check('primary width/height pass through', b.width === 1920 && b.height === 1068); +} + +{ + // THE BUG SCENARIO: secondary monitor at x=1920. A naive clamp comparing + // screen-absolute frame.x against monitor-relative workArea.x (0) would + // relocate the window onto the primary monitor. Reconciliation adds 1920. + const mon = { x: 1920, y: 0 }; + const wa = { x: 0, y: 0, width: 1920, height: 1080 }; + const b = workAreaToBounds(mon, wa); + check('secondary monitor bounds.x == 1920', b.x === 1920, + `got ${b.x} (wrong monitor if 0)`); + check('secondary monitor right edge == 3840', b.x + b.width === 3840, + `got ${b.x + b.width}`); +} + +// ─── computeClampedPosition ──────────────────────────────────────────────────── + +print('\n== computeClampedPosition (clamp math) =='); + +{ + const bounds = { x: 0, y: 0, width: 1920, height: 1080 }; + + let r = computeClampedPosition({ x: 100, y: 100, width: 800, height: 600 }, bounds); + check('already visible: unchanged', rectEq(r, { x: 100, y: 100 }), + `got x=${r.x} y=${r.y}`); + + r = computeClampedPosition({ x: -200, y: 100, width: 800, height: 600 }, bounds); + check('off left: clamped to minX', r.x === 0, `got ${r.x}`); + + r = computeClampedPosition({ x: 1500, y: 100, width: 800, height: 600 }, bounds); + check('off right: clamped to maxX - width', r.x === 1920 - 800, `got ${r.x}`); + + r = computeClampedPosition({ x: 100, y: -50, width: 800, height: 600 }, bounds); + check('off top: clamped to minY', r.y === 0, `got ${r.y}`); + + r = computeClampedPosition({ x: 100, y: 700, width: 800, height: 600 }, bounds); + check('off bottom: clamped to maxY - height', r.y === 1080 - 600, `got ${r.y}`); + + // oversize: wider AND taller than the workarea must pin to the leading + // edge, not go negative (this case is why computeClampedPosition uses + // Math.max(minX, …) rather than a bare right-edge rule). + r = computeClampedPosition({ x: 500, y: 500, width: 3000, height: 2000 }, bounds); + check('oversize: clamped to minX', r.x === 0, `got ${r.x}`); + check('oversize: clamped to minY', r.y === 0, `got ${r.y}`); +} + +{ + // Multi-monitor: a popup half-off the RIGHT edge of the secondary monitor + // must stay on the secondary monitor, not jump to the primary. + const bounds = { x: 1920, y: 0, width: 1920, height: 1080 }; + const frame = { x: 1920 + 1500, y: 100, width: 800, height: 600 }; + const r = computeClampedPosition(frame, bounds); + check('multi-monitor: stays on secondary', + r.x === 1920 + (1920 - 800), `got ${r.x}, expected ${1920 + (1920 - 800)}`); + check('multi-monitor: not relocated to primary', r.x >= 1920, + `got ${r.x} (< 1920 would be the primary-monitor bug)`); +} + +// ─── end-to-end (workArea + frame + clamp, multi-monitor) ────────────────────── + +print('\n== end-to-end (workArea + frame + clamp, multi-monitor) =='); + +{ + // Reproduces the reported symptom: popup of an off-screen parent, partially + // visible (clamped to the monitor edge by mutter). Should land fully + // on-screen AND on the correct monitor. + const monitor = { x: 1920, y: 0 }; + const workArea = { x: 0, y: 0, width: 1920, height: 1080 }; + const bounds = workAreaToBounds(monitor, workArea); + const frame = { x: 3500, y: 100, width: 800, height: 600 }; + const target = computeClampedPosition(frame, bounds); + const fullyOnScreen = + target.x >= bounds.x && + target.y >= bounds.y && + target.x + frame.width <= bounds.x + bounds.width && + target.y + frame.height <= bounds.y + bounds.height; + check('end-to-end: popup fully on-screen', fullyOnScreen, + `target x=${target.x} y=${target.y}`); + check('end-to-end: on correct (secondary) monitor', target.x >= 1920, + `got ${target.x}`); + check('end-to-end: actually moved (not a no-op)', + !(target.x === frame.x && target.y === frame.y)); +} + +{ + // No-op case: popup already visible should not move (idempotency for + // repeated demands-attention signals). + const monitor = { x: 0, y: 0 }; + const workArea = { x: 0, y: 0, width: 1920, height: 1080 }; + const bounds = workAreaToBounds(monitor, workArea); + const frame = { x: 500, y: 400, width: 800, height: 600 }; + const target = computeClampedPosition(frame, bounds); + check('no-op when already visible', + target.x === frame.x && target.y === frame.y, + `expected unchanged, got x=${target.x} y=${target.y}`); +} + +// ─── summary ─────────────────────────────────────────────────────────────────── + +print(`\n━━━ ${_passed} passed, ${_failed} failed ━━━`); +if (_failed > 0) { + print('Failures:'); + _failures.forEach(f => print(` • ${f}`)); +} +system.exit(_failed > 0 ? 1 : 0); diff --git a/tests/trigger-popup.js b/tests/trigger-popup.js new file mode 100644 index 00000000..7a3d23d4 --- /dev/null +++ b/tests/trigger-popup.js @@ -0,0 +1,82 @@ +#!/usr/bin/env gjs +/* + * Manual-verification trigger for the popup-visibility fix — the live-window + * companion to tests/test-popup-visibility.js (which covers only the pure, + * shell-free geometry). Verified to reproduce the bug pre-fix and confirm it + * post-fix. + * + * Run: gjs -m tests/trigger-popup.js [delay_secs] + * + * 1. Opens a parent window (PaperWM tiles it). + * 2. Waits seconds (default 10) — scroll the parent OUT OF VIEW. + * 3. Pops up a transient (modal) dialog for the parent. + * + * fix works ⇒ the dialog lands fully on-screen (Case A if it got focus, + * Case B / demands-attention if focus was denied — no focus steal + * either way, it's just repositioned). + * bug present ⇒ dialog clamped at the monitor edge / partially off-screen, + * or (Case B + skip_taskbar) not visible at all. + * + * To exercise Case A (popup gets focus), click the parent window right before + * the timer fires. To exercise Case B (focus denied — the common background + * case), just don't touch the parent after it opens; the popup arrives "stale" + * and mutter's focus-stealing prevention denies focus. + */ + +import Gtk from 'gi://Gtk?version=4.0'; +import GLib from 'gi://GLib'; + +const delay = parseInt(ARGV[0] ?? '10', 10); + +const app = new Gtk.Application({ application_id: 'org.paperwm.trigger' }); + +let parent; + +app.connect('activate', () => { + parent = new Gtk.ApplicationWindow({ + application: app, + title: 'PaperWM popup trigger — parent', + default_width: 520, + default_height: 350, + }); + parent.set_child(new Gtk.Label({ + label: `Parent window.\n\nIn ${delay}s a transient dialog will pop up.\n` + + '→ scroll me OUT OF VIEW in PaperWM now.', + })); + parent.connect('close-request', () => app.quit()); + parent.present(); + + print(`parent up — popup in ${delay}s (scroll the parent off-screen now)`); + + GLib.timeout_add_seconds(GLib.PRIORITY_DEFAULT, delay, () => { + const dlg = new Gtk.Window({ + transient_for: parent, + modal: true, + title: 'Transient popup', + default_width: 360, + default_height: 160, + destroy_with_parent: true, + }); + const box = new Gtk.Box({ + orientation: Gtk.Orientation.VERTICAL, + spacing: 12, + margin_top: 14, margin_bottom: 14, margin_start: 14, margin_end: 14, + }); + box.append(new Gtk.Label({ + label: 'Readable + fully on-screen ⇒ the fix works.\n' + + 'Clamped at the edge / invisible ⇒ bug.', + })); + const btn = new Gtk.Button({ label: 'Close' }); + btn.connect('clicked', () => { + dlg.destroy(); + app.quit(); + }); + box.append(btn); + dlg.set_child(box); + dlg.present(); + print('popup shown'); + return GLib.SOURCE_REMOVE; + }); +}); + +app.run([]); diff --git a/tiling.js b/tiling.js index a56ce3c8..b7a488cf 100644 --- a/tiling.js +++ b/tiling.js @@ -13,6 +13,7 @@ import { import { Easer, DispatcherMode } from './utils.js'; import { ClickOverlay } from './stackoverlay.js'; import { WorkspaceSettings } from './workspace.js'; +import { workAreaToBounds, computeClampedPosition } from './popuputil.js'; const { signals: Signals } = imports; const workspaceManager = global.workspace_manager; @@ -2304,6 +2305,11 @@ export const Spaces = class Spaces extends Map { this.signals.connect(display, 'window-created', (display, metaWindow, _user_data) => this.window_created(metaWindow)); + this.signals.connect(display, 'window-demands-attention', + (_display, metaWindow) => positionPopupOnDemand(metaWindow)); + this.signals.connect(display, 'window-marked-urgent', + (_display, metaWindow) => positionPopupOnDemand(metaWindow)); + this.signals.connect(display, 'grab-op-begin', (display, mw, type) => grabBegin(mw, type)); this.signals.connect(display, 'grab-op-end', (display, mw, type) => grabEnd(mw, type)); @@ -4683,6 +4689,58 @@ export function getDefaultFocusMode() { } // `MetaWindow::focus` handling +/** + * Whether `metaWindow` is popup-class: a real (non-tiled) surface mutter + * positions itself — transients and non-NORMAL dialogs/modals, but not sticky + * or scratch windows (which have their own positioning). Derived from the + * canonical `add_filter` (single source of truth for tiling eligibility) minus + * sticky/scratch, so it follows automatically if `add_filter` widens. + */ +export function isPopupClass(metaWindow) { + if (metaWindow.is_on_all_workspaces() || Scratch.isScratchWindow(metaWindow)) + return false; + return !add_filter(metaWindow); +} + +/** + * Reposition a popup-class window fully inside its space's workArea. + * + * Unlike tiled windows, popups are real MetaWindows positioned by mutter (not + * in the clone container), so they can't be scrolled via `ensureViewport`. We + * move them directly with `move_frame`. No-op if already fully on-screen. + * + * Precondition: `metaWindow`'s space agrees with its physical monitor — + * cross-workspace popups are redirected by `insertWindow` before this runs. + */ +export function ensureVisibleInWorkArea(metaWindow) { + const space = spaces.spaceOfWindow(metaWindow); + // Bail if the window's physical monitor doesn't match its workspace's space + // (can desync on cross-workspace moves) — repositioning from the wrong + // monitor's workArea would yank the popup across heads. + if (metaWindow.get_monitor() !== space.monitor.index) + return; + const bounds = workAreaToBounds(space.monitor, space.workArea()); + const frame = metaWindow.get_frame_rect(); + const { x, y } = computeClampedPosition(frame, bounds); + if (x !== frame.x || y !== frame.y) + metaWindow.move_frame(true, x, y); +} + +/** + * Make a popup that demanded attention fully visible WITHOUT stealing focus + * (focus was denied by mutter's focus-stealing prevention, or never requested). + * By design the user's current window keeps focus. Non-popup demands-attention + * (a tiled window wanting attention) is left to gnome-shell's default handler. + */ +export function positionPopupOnDemand(metaWindow) { + // Demands-attention / urgent can fire mid-teardown on a window whose actor + // is gone or frame not yet computed — bail before touching it. + if (!metaWindow || metaWindow.unmapped || !metaWindow.get_compositor_private()) + return; + if (isPopupClass(metaWindow)) + ensureVisibleInWorkArea(metaWindow); +} + export function focus_handler(metaWindow) { console.debug("focus:", metaWindow?.title); if (Scratch.isScratchWindow(metaWindow)) { @@ -4692,9 +4750,13 @@ export function focus_handler(metaWindow) { return; } - // If metaWindow is a transient window, return (after deselecting tiled focus indicators) + // Transient window: it's a real MetaWindow mutter positions itself, not in + // the clone container, so ensureViewport can't scroll it. Move it fully + // on-screen instead, then bail out of the tiled focus logic (after + // deselecting tiled focus indicators). if (isTransient(metaWindow)) { setAllWorkspacesInactive(); + ensureVisibleInWorkArea(metaWindow); return; } @@ -5026,8 +5088,7 @@ export function cycleWindowWidthBackwards(metawindow) { export function cycleWindowWidthDirection(metaWindow, direction) { let frame = metaWindow.get_frame_rect(); let space = spaces.spaceOfWindow(metaWindow); - let workArea = space.workArea(); - workArea.x += space.monitor.x; + let workArea = workAreaToBounds(space.monitor, space.workArea()); let findFn = direction === CycleWindowSizesDirection.FORWARD ? Lib.findNext : Lib.findPrev;