From 539d514adbbae93718a595f33de32fe5a860253e Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Mon, 5 Oct 2026 15:55:18 +0100 Subject: [PATCH] fix(street-level): viewer lifecycle, hover cost, keyboard focus, panel lift From the IC8 review of #768 (P2s), each with a test that fails before: - Switching the layer off and on during the first photo download no longer leaves the viewer stuck on "not mounted": an outdated mount only unmounts an adapter nobody else holds and subscribes poses only once current, and the MapillaryJS adapter never builds a viewer after it was unmounted. - Hover picks at most every 120 ms (the CCTV convention), not while the camera moves or a button is held, and not after the layer went off. - Esc shrinks the expanded viewer wherever focus is (document capture), so the map keeps its selection, and Tab from outside returns into it. Closing a photo moves focus to the panel instead of dropping it on body. - Portable panels lift only after a mouse or pen moves past the drag threshold: a click on a docked panel's edge or a touch swipe on its header no longer turns it into a floating window, and pointercancel saves nothing and puts the panel back. Co-Authored-By: Claude Opus 5.5 --- scripts/format-scope.json | 1 + .../streetLevel/providers/mapillary/viewer.js | 9 + src/layers/streetLevel/selection.js | 121 +++++++++-- src/layers/streetLevel/selection.test.mjs | 140 +++++++++++++ src/layers/streetLevel/viewerHost.js | 20 +- src/layers/streetLevel/viewerHost.test.mjs | 139 +++++++++++- src/ui/panelDock.test.mjs | 114 ++++++++++ src/ui/panelPositionControls.js | 110 +++++++--- src/ui/streetLevelControls.js | 43 +++- src/ui/streetLevelControls.test.mjs | 197 +++++++++++++++--- 10 files changed, 793 insertions(+), 101 deletions(-) create mode 100644 src/layers/streetLevel/selection.test.mjs diff --git a/scripts/format-scope.json b/scripts/format-scope.json index 8bc44303d..58ae396c6 100644 --- a/scripts/format-scope.json +++ b/scripts/format-scope.json @@ -915,6 +915,7 @@ "src/ui/streetLevelControls.test.mjs", "src/ui/panelDock.test.mjs", "src/layers/streetLevel/viewerHost.test.mjs", + "src/layers/streetLevel/selection.test.mjs", "src/layers/streetLevel/index.test.mjs", "scripts/qa-overpass-offload.mjs", "src/overpassOffload.test.mjs", diff --git a/src/layers/streetLevel/providers/mapillary/viewer.js b/src/layers/streetLevel/providers/mapillary/viewer.js index 91128262a..41a8f0c1b 100644 --- a/src/layers/streetLevel/providers/mapillary/viewer.js +++ b/src/layers/streetLevel/providers/mapillary/viewer.js @@ -14,6 +14,8 @@ export function createMapillaryViewer({ source, render } = {}) { let prewarming = false; /** In-flight viewer construction, so pre-warm and open never build two. */ let creating = null; + /** Bumped by `unmount`, so a construction it overtook builds nothing. */ + let generation = 0; let renderMode = 'letterbox'; /** Metadata of the image on screen; pov/position events reuse it. */ let current = null; @@ -106,8 +108,12 @@ export function createMapillaryViewer({ source, render } = {}) { } async function createViewer(host) { + const built = generation; destroyViewer(); const { Viewer } = await ensureLibrary(); + // Unmounted (layer off) while the library loaded: a viewer built now + // would hold a WebGL context nobody releases. + if (built !== generation) throw new Error('Mapillary viewer was unmounted'); viewer = new Viewer({ accessToken: source.token, container: host, @@ -165,6 +171,9 @@ export function createMapillaryViewer({ source, render } = {}) { unmount() { pendingOpen = null; + // The next mount builds afresh rather than joining a stale build. + generation++; + creating = null; destroyViewer(); }, diff --git a/src/layers/streetLevel/selection.js b/src/layers/streetLevel/selection.js index 6818f40ae..6273a4c4c 100644 --- a/src/layers/streetLevel/selection.js +++ b/src/layers/streetLevel/selection.js @@ -1,6 +1,12 @@ import * as Cesium from 'cesium'; import { STREET_LEVEL_LAYER_ID } from './policy.js'; +/** + * Minimum time between hover picks: ~8 `scene.pick` calls a second while the + * pointer moves, as the CCTV layer's hover does, never one per frame. + */ +const HOVER_PICK_INTERVAL_MS = 120; + /** One click/hover handler for every provider's coverage and image cones. */ export function createSelection({ state, parts }) { const { picking, input } = state.services; @@ -43,33 +49,100 @@ export function createSelection({ state, parts }) { let hoverQueued = false; let hoverCursor = false; - /** Pointer cursor over anything this layer owns, so lines read as clickable. */ + let hoverPosition = null; + let hoverLastPickAt = -Infinity; + let hoverTimer = null; + let cameraMoving = false; + let pointerButtons = 0; + let removeHoverWatchers = null; + + function setHoverCursor(canvas, hit) { + if (hit && !hoverCursor) { + canvas.style.cursor = 'pointer'; + hoverCursor = true; + } else if (!hit && hoverCursor) { + canvas.style.cursor = ''; + hoverCursor = false; + } + } + + function pickHover() { + hoverQueued = false; + const viewer = state.viewer; + // Switched off (or torn down) since the frame was queued. + if (!viewer || !state.enabled || !state.clickHandler) return; + // Orbiting, or a drag in progress: the pointer is the camera's. + if (cameraMoving || pointerButtons !== 0) return; + const canvas = viewer.scene?.canvas; + if (!canvas || viewer.isDestroyed?.()) return; + hoverLastPickAt = Date.now(); + let hit = false; + try { + const picked = viewer.scene.pick(hoverPosition); + const id = picking?.resolvePickId + ? picking.resolvePickId(picked) + : picked?.id; + hit = ownsPick(id); + } catch { + hit = false; + } + setHoverCursor(canvas, hit); + } + + /** + * Pointer cursor over anything this layer owns, so lines read as clickable. + * Picks the latest position at most every HOVER_PICK_INTERVAL_MS; a move + * inside the interval is picked when it ends, so the cursor never sticks. + */ function onMove(movement) { const viewer = state.viewer; - if (!viewer || !state.enabled || hoverQueued) return; + if (!viewer || !state.enabled) return; + hoverPosition = movement.endPosition; + if (hoverQueued) return; hoverQueued = true; - requestAnimationFrame(() => { - hoverQueued = false; - const canvas = viewer.scene?.canvas; - if (!canvas || viewer.isDestroyed?.()) return; - let hit = false; - try { - const picked = viewer.scene.pick(movement.endPosition); - const id = picking?.resolvePickId - ? picking.resolvePickId(picked) - : picked?.id; - hit = ownsPick(id); - } catch { - hit = false; - } - if (hit && !hoverCursor) { - canvas.style.cursor = 'pointer'; - hoverCursor = true; - } else if (!hit && hoverCursor) { - canvas.style.cursor = ''; - hoverCursor = false; - } + const wait = hoverLastPickAt + HOVER_PICK_INTERVAL_MS - Date.now(); + if (wait <= 0) { + requestAnimationFrame(pickHover); + return; + } + hoverTimer = setTimeout(() => { + hoverTimer = null; + requestAnimationFrame(pickHover); + }, wait); + } + + /** Track what makes a hover pick pointless: camera motion, held buttons. */ + function watchHover(viewer) { + const canvas = viewer.scene.canvas; + const camera = viewer.camera; + const onButtons = (event) => { + pointerButtons = event.buttons ?? 0; + }; + for (const type of ['pointerdown', 'pointermove', 'pointerup']) + canvas.addEventListener?.(type, onButtons); + const removeStart = camera?.moveStart?.addEventListener(() => { + cameraMoving = true; }); + const removeEnd = camera?.moveEnd?.addEventListener(() => { + cameraMoving = false; + }); + removeHoverWatchers = () => { + for (const type of ['pointerdown', 'pointermove', 'pointerup']) + canvas.removeEventListener?.(type, onButtons); + removeStart?.(); + removeEnd?.(); + }; + } + + function unwatchHover() { + removeHoverWatchers?.(); + removeHoverWatchers = null; + clearTimeout(hoverTimer); + hoverTimer = null; + hoverQueued = false; + hoverLastPickAt = -Infinity; + cameraMoving = false; + pointerButtons = 0; } function install(viewer) { @@ -85,6 +158,7 @@ export function createSelection({ state, parts }) { onMove, Cesium.ScreenSpaceEventType.MOUSE_MOVE, ); + watchHover(viewer); document.addEventListener('keydown', onKeyDown); picking?.registerPickOwner?.(STREET_LEVEL_LAYER_ID, ownsPick); } @@ -92,6 +166,7 @@ export function createSelection({ state, parts }) { function uninstall() { // Nothing to undo for a layer that was never enabled. if (!state.clickHandler) return; + unwatchHover(); if (hoverCursor && state.viewer?.scene?.canvas) { state.viewer.scene.canvas.style.cursor = ''; hoverCursor = false; diff --git a/src/layers/streetLevel/selection.test.mjs b/src/layers/streetLevel/selection.test.mjs new file mode 100644 index 000000000..eb680cd43 --- /dev/null +++ b/src/layers/streetLevel/selection.test.mjs @@ -0,0 +1,140 @@ +import assert from 'node:assert/strict'; +import test, { mock } from 'node:test'; +import * as Cesium from 'cesium'; +import { createSelection } from './selection.js'; + +/** + * The selection installed on a stand-in viewer. Animation frames queue until + * `frame()` runs them; timers and Date.now() are mocked so a burst of moves + * has a known timeline. + */ +function harness() { + const saved = { + document: globalThis.document, + requestAnimationFrame: globalThis.requestAnimationFrame, + }; + mock.timers.enable({ apis: ['setTimeout', 'Date'] }); + const frames = []; + globalThis.requestAnimationFrame = (task) => frames.push(task); + globalThis.document = new EventTarget(); + const picks = []; + const canvas = Object.assign(new EventTarget(), { + style: {}, + // Keep Cesium's handler on the canvas; there is no real document here. + disableRootEvents: true, + onwheel: null, + }); + const camera = { moveStart: new Cesium.Event(), moveEnd: new Cesium.Event() }; + const viewer = { + scene: { + canvas, + pick(position) { + picks.push(position); + return null; + }, + }, + camera, + isDestroyed: () => false, + }; + const state = { + viewer, + enabled: true, + clickHandler: null, + services: { picking: null, input: null }, + }; + const selection = createSelection({ + state, + parts: { + router: { ownsPick: () => false, resolve: () => null }, + hasSelectedSequence: () => false, + clearSequences() {}, + }, + }); + selection.install(viewer); + const onMove = state.clickHandler.getInputAction( + Cesium.ScreenSpaceEventType.MOUSE_MOVE, + ); + return { + state, + canvas, + camera, + picks, + move: (x) => onMove({ endPosition: { x, y: 10 } }), + frame() { + for (const task of frames.splice(0)) task(); + }, + /** Let `ms` pass, running the frames that come due on the way. */ + wait(ms) { + this.frame(); + mock.timers.tick(ms); + this.frame(); + }, + buttons(type, buttons) { + const event = new Event(type); + Object.defineProperty(event, 'buttons', { value: buttons }); + canvas.dispatchEvent(event); + }, + restore() { + selection.uninstall(); + mock.timers.reset(); + Object.assign(globalThis, saved); + }, + }; +} + +test('hover picks at most every 120 ms through a burst of moves, ending on the last one', () => { + const h = harness(); + try { + // One second of a pointer moving at 60 Hz. + for (let i = 0; i < 60; i++) { + h.move(i); + h.wait(16); + } + assert.ok(h.picks.length <= 9, `${h.picks.length} picks in ~1 s`); + assert.ok(h.picks.length >= 7, `${h.picks.length} picks in ~1 s`); + h.wait(200); + assert.deepEqual(h.picks.at(-1), { x: 59, y: 10 }, 'the resting position'); + const settled = h.picks.length; + h.wait(1000); + assert.equal(h.picks.length, settled, 'a still pointer costs nothing'); + } finally { + h.restore(); + } +}); + +test('hover does not pick while the camera moves or a mouse button is held', () => { + const h = harness(); + try { + h.camera.moveStart.raiseEvent(); + h.move(1); + h.wait(200); + assert.equal(h.picks.length, 0, 'camera in motion'); + h.camera.moveEnd.raiseEvent(); + h.move(2); + h.wait(200); + assert.equal(h.picks.length, 1, 'picks again once the camera rests'); + + h.buttons('pointerdown', 1); + h.move(3); + h.wait(200); + assert.equal(h.picks.length, 1, 'a drag in progress'); + h.buttons('pointerup', 0); + h.move(4); + h.wait(200); + assert.equal(h.picks.length, 2); + } finally { + h.restore(); + } +}); + +test('a hover frame queued before the layer went off does not pick', () => { + const h = harness(); + try { + h.move(1); + h.state.enabled = false; + h.wait(200); + assert.equal(h.picks.length, 0); + } finally { + h.restore(); + } +}); diff --git a/src/layers/streetLevel/viewerHost.js b/src/layers/streetLevel/viewerHost.js index b142951a0..fabe67468 100644 --- a/src/layers/streetLevel/viewerHost.js +++ b/src/layers/streetLevel/viewerHost.js @@ -76,19 +76,19 @@ export function createViewerHost({ state, parts }) { } const adapter = entry.instance.viewer; const promise = (async () => { - const unsubscribe = adapter.onPose(applyPose); - try { - await adapter.mount(state.street.host); - } catch (error) { - unsubscribe(); - throw error; - } + await adapter.mount(state.street.host); if (mounting?.promise !== promise) { - // Unmounted (layer off, provider switched) while loading. - unsubscribe(); - adapter.unmount(); + // Unmounted (layer off, provider switched) while loading. A newer + // mount of this provider shares the adapter and its viewer: tearing + // it down here would leave that mount active with no viewer. + const reused = + active?.id === entry.def.id || mounting?.id === entry.def.id; + if (!reused) adapter.unmount(); throw new Error('Street-level viewer was closed'); } + // Listen only once current: an outdated mount releasing the same + // `applyPose` would otherwise unhook the mount that replaced it. + const unsubscribe = adapter.onPose(applyPose); active = { id: entry.def.id, adapter, unsubscribe }; warmed.delete(entry.def.id); adapter.setRenderMode?.(state.street.renderMode); diff --git a/src/layers/streetLevel/viewerHost.test.mjs b/src/layers/streetLevel/viewerHost.test.mjs index 4ef069e90..61717aca9 100644 --- a/src/layers/streetLevel/viewerHost.test.mjs +++ b/src/layers/streetLevel/viewerHost.test.mjs @@ -1,7 +1,35 @@ import assert from 'node:assert/strict'; +import { registerHooks } from 'node:module'; import test from 'node:test'; import { createViewerHost } from './viewerHost.js'; +// MapillaryJS needs a browser; the adapter tests swap in a stand-in library. +// Every import gets a fresh copy that waits for the current test's gate, so +// each test controls when its "download" finishes. +let libraryImports = 0; +registerHooks({ + resolve(specifier, context, next) { + if (!specifier.startsWith('mapillary-js')) return next(specifier, context); + const kind = specifier === 'mapillary-js' ? 'library' : 'asset'; + return { + url: `gev-test-stub:mapillary-js/${kind}?${++libraryImports}`, + shortCircuit: true, + }; + }, + load(url, context, next) { + if (!url.startsWith('gev-test-stub:mapillary-js/')) + return next(url, context); + const source = url.includes('/library?') + ? 'const lib = await globalThis.__gevMapillaryLibrary();\n' + + 'export const Viewer = lib.Viewer;\n' + + 'export const RenderMode = lib.RenderMode;\n' + : 'export default {};\n'; + return { format: 'module', shortCircuit: true, source }; + }, +}); +const { createMapillaryViewer } = + await import('./providers/mapillary/viewer.js'); + function harness(adapter) { const state = { enabled: true, @@ -18,14 +46,23 @@ function harness(adapter) { return { state, host: createViewerHost({ state, parts }) }; } -function fakeAdapter({ failMounts = 0 } = {}) { +/** + * A stand-in adapter. Like the real one, concurrent mounts share one + * construction (`gate` holds it open) and `unmount` destroys the viewer that + * every later `open` needs. + */ +function fakeAdapter({ failMounts = 0, gate = null } = {}) { const calls = { mount: 0, open: [], unmount: 0, listeners: 0 }; let emit = null; + let mounted = false; return { calls, async mount() { calls.mount++; - if (calls.mount <= failMounts) throw new Error('library failed to load'); + const attempt = calls.mount; + await gate?.promise; + if (attempt <= failMounts) throw new Error('library failed to load'); + mounted = true; }, /** A pose event from the library, as MapillaryJS fires them. */ emitPose(id) { @@ -40,12 +77,14 @@ function fakeAdapter({ failMounts = 0 } = {}) { }); }, async open(id) { + if (!mounted) throw new Error('viewer is not mounted'); calls.open.push(id); this.emitPose(id); }, close() {}, unmount() { calls.unmount++; + mounted = false; }, resize() {}, onPose(listener) { @@ -146,3 +185,99 @@ test('a pose that arrives after the photo was closed does not bring it back', as assert.equal(state.street.sequenceId, null); assert.deepEqual(state.street, before); }); + +function deferred() { + let resolve; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + +test('switching the layer off and on during a cold first open keeps the shared viewer', async () => { + const gate = deferred(); + const adapter = fakeAdapter({ gate }); + const { state, host } = harness(adapter); + const first = host.open('mapillary', 'a'); // the library is still loading + host.unmount(); // layer off … + const second = host.open('mapillary', 'b'); // … on again, and a new click + gate.resolve(); + await Promise.all([first, second]); + assert.equal(adapter.calls.unmount, 0, 'the outdated mount left it alone'); + assert.equal(adapter.calls.listeners, 1); + assert.equal(state.street.error, null); + assert.equal(state.street.imageId, 'b'); + await host.open('mapillary', 'c'); + assert.equal(state.street.error, null, 'later opens are not stuck'); + assert.equal(state.street.imageId, 'c'); +}); + +/** MapillaryJS, counting live viewers (each holds a WebGL context). */ +function fakeLibrary() { + const gate = deferred(); + const viewers = { created: 0, live: 0 }; + class Viewer { + constructor() { + viewers.created++; + viewers.live++; + this.removed = false; + } + on() {} + async getPosition() { + return { lng: 1, lat: 2 }; + } + async getPointOfView() { + return { bearing: 10, tilt: 0 }; + } + async moveTo(id) { + return { id, cameraType: 'perspective', sequenceId: 's' }; + } + remove() { + if (this.removed) return; + this.removed = true; + viewers.live--; + } + setRenderMode() {} + resize() {} + } + globalThis.__gevMapillaryLibrary = async () => { + await gate.promise; + return { Viewer, RenderMode: {} }; + }; + return { gate, viewers }; +} + +test('the Mapillary viewer survives the layer going off and on during its first download', async () => { + const { gate, viewers } = fakeLibrary(); + const adapter = createMapillaryViewer({ source: { token: 't' } }); + const { state, host } = harness(adapter); + const first = host.open('mapillary', 'img1'); + await Promise.resolve(); + host.unmount(); + const second = host.open('mapillary', 'img2'); + await Promise.resolve(); + gate.resolve(); + await Promise.all([first, second]); + assert.equal(state.street.error, null); + assert.equal(state.street.imageId, 'img2'); + await host.open('mapillary', 'img3'); + assert.equal(state.street.error, null, 'later opens are not stuck'); + assert.equal(state.street.imageId, 'img3'); + assert.equal(viewers.live, 1); +}); + +test('a Mapillary viewer unmounted mid-download is never built; the next mount builds afresh', async () => { + const { gate, viewers } = fakeLibrary(); + const adapter = createMapillaryViewer({ source: { token: 't' } }); + const element = {}; + const stale = adapter.mount(element); + adapter.unmount(); + const fresh = adapter.mount(element); + gate.resolve(); + await assert.rejects(stale, /unmounted/); + await fresh; + assert.equal(viewers.created, 1, 'only the fresh mount built one'); + await adapter.open('x'); + adapter.unmount(); + assert.equal(viewers.live, 0, 'nothing left holding a WebGL context'); +}); diff --git a/src/ui/panelDock.test.mjs b/src/ui/panelDock.test.mjs index ee9858a95..0b4291d55 100644 --- a/src/ui/panelDock.test.mjs +++ b/src/ui/panelDock.test.mjs @@ -34,8 +34,10 @@ function fixture() { querySelector: () => null, querySelectorAll: () => [], closest: () => null, + setAttribute() {}, removeAttribute() {}, appendChild() {}, + remove() {}, getBoundingClientRect: () => ({ left: 100, top: 80, @@ -49,6 +51,7 @@ function fixture() { globalThis.document = Object.assign(new EventTarget(), { getElementById: () => null, querySelectorAll: () => [], + createElement: () => element(), }); globalThis.window = Object.assign(new EventTarget(), { innerWidth: 1440, @@ -245,3 +248,114 @@ test('renumbering the z ladder never drops a window below the docked panels', () f.restore(); } }); + +/** A portable panel still in its rail, with its header and resize handles. */ +function dockedPanel(f, id) { + const panel = f.element(); + const handle = f.element(); + panel.id = id; + const portable = { panel, min: { width: 320, height: 280 } }; + f.owner._portablePanels.set(id, portable); + f.owner._makePanelDraggable(id, panel, handle); + f.owner._makePanelResizable(id, panel, portable.min); + const edges = Object.fromEntries( + f.owner._resizeHandles.map((node) => [node.dataset.dir, node]), + ); + return { panel, handle, edges }; +} + +/** A pointer event; `pointerType` defaults to what a mouse reports. */ +function pointer(target, type, { x = 200, y = 90, pointerType = 'mouse' }) { + const event = new Event(type, { cancelable: true }); + for (const [key, value] of Object.entries({ + button: 0, + clientX: x, + clientY: y, + pointerType, + })) + Object.defineProperty(event, key, { value }); + target.dispatchEvent(event); + return event; +} + +const POSITION_KEY = 'godsEyeView.v8.panelPos.street-level-panel'; + +test('a click on a docked panel’s resize edge leaves it docked and stores nothing', () => { + const f = fixture(); + try { + const { panel, edges } = dockedPanel(f, 'street-level-panel'); + pointer(edges.w, 'pointerdown', { x: 101, y: 200 }); + pointer(window, 'pointermove', { x: 103, y: 201 }); // a jittery click + pointer(window, 'pointerup', { x: 103, y: 201 }); + assert.equal(panel.classList.contains('panel-floating'), false); + assert.equal(panel.style.width, undefined, 'no size was pinned'); + assert.equal(f.store.has(POSITION_KEY), false); + assert.deepEqual(f.calls.resized, []); + + // A real resize still lifts it out and remembers the window. + pointer(edges.w, 'pointerdown', { x: 101, y: 200 }); + pointer(window, 'pointermove', { x: 61, y: 200 }); + pointer(window, 'pointerup', { x: 61, y: 200 }); + assert.equal(panel.classList.contains('panel-floating'), true); + assert.equal(JSON.parse(f.store.get(POSITION_KEY)).floating, true); + } finally { + f.restore(); + } +}); + +test('a touch on the header or an edge does not lift the panel, and the rail can still scroll', () => { + const f = fixture(); + try { + const { panel, handle, edges } = dockedPanel(f, 'street-level-panel'); + const press = pointer(handle, 'pointerdown', { pointerType: 'touch' }); + assert.equal(press.defaultPrevented, false, 'the swipe can scroll'); + pointer(window, 'pointermove', { y: 190, pointerType: 'touch' }); + pointer(window, 'pointerup', { y: 190, pointerType: 'touch' }); + pointer(edges.s, 'pointerdown', { y: 500, pointerType: 'touch' }); + pointer(window, 'pointermove', { y: 600, pointerType: 'touch' }); + pointer(window, 'pointerup', { y: 600, pointerType: 'touch' }); + assert.equal(panel.classList.contains('panel-floating'), false); + assert.equal(f.store.has(POSITION_KEY), false); + } finally { + f.restore(); + } +}); + +test('a pointercancel mid-drag saves nothing and puts the panel back', () => { + const f = fixture(); + try { + const { panel, handle, edges } = dockedPanel(f, 'street-level-panel'); + pointer(handle, 'pointerdown', {}); + pointer(window, 'pointermove', { x: 260, y: 140 }); + assert.equal(panel.classList.contains('panel-floating'), true, 'lifted'); + pointer(window, 'pointercancel', { x: 260, y: 140 }); + assert.equal(panel.classList.contains('panel-floating'), false, 'docked'); + assert.equal(f.store.has(POSITION_KEY), false); + + pointer(edges.e, 'pointerdown', { x: 459 }); + pointer(window, 'pointermove', { x: 520 }); + pointer(window, 'pointercancel', { x: 520 }); + assert.equal(panel.classList.contains('panel-floating'), false); + assert.equal(f.store.has(POSITION_KEY), false); + } finally { + f.restore(); + } +}); + +test('a pointercancel while dragging a floating window returns it to where it was', () => { + const f = fixture(); + try { + const { panel, handle } = floatingPanel(f, 'cctv-panel'); + const stored = f.store.get('godsEyeView.v8.panelPos.cctv-panel'); + pointer(handle, 'pointerdown', {}); + pointer(window, 'pointermove', { x: 400, y: 300 }); + assert.notEqual(panel.style.left, '100px', 'it moved'); + pointer(window, 'pointercancel', { x: 400, y: 300 }); + assert.equal(panel.style.left, '100px'); + assert.equal(panel.style.top, '80px'); + assert.equal(panel.classList.contains('panel-floating'), true); + assert.equal(f.store.get('godsEyeView.v8.panelPos.cctv-panel'), stored); + } finally { + f.restore(); + } +}); diff --git a/src/ui/panelPositionControls.js b/src/ui/panelPositionControls.js index 5ec2476a5..5ab57465a 100644 --- a/src/ui/panelPositionControls.js +++ b/src/ui/panelPositionControls.js @@ -52,6 +52,21 @@ function clamp(value, min, max) { return Math.max(min, Math.min(max, value)); } +/** + * Only a mouse or pen lifts and resizes a portable panel. On a phone the + * rail is a scroll container, so a finger on a header is a swipe. + */ +function isPrecisePointer(event) { + const pointerType = event.pointerType || 'mouse'; + return pointerType === 'mouse' || pointerType === 'pen'; +} + +/** Inline geometry a cancelled gesture puts back. */ +function geometryOf(panelEl) { + const { left, top, width, height } = panelEl.style; + return { left, top, width, height }; +} + export class PanelPositionControls { /** * @param {object} options @@ -548,7 +563,7 @@ export class PanelPositionControls { }); let lastPress = null; this.listen(handleEl, 'pointerdown', (event) => { - if (event.button !== 0) return; + if (event.button !== 0 || !isPrecisePointer(event)) return; const interactive = event.target.closest?.(INTERACTIVE_SELECTOR); if (interactive && !interactive.matches('.panel-collapse-btn')) return; @@ -574,6 +589,8 @@ export class PanelPositionControls { const startX = event.clientX; const startY = event.clientY; let dragging = false; + let lifted = false; + let before = null; let offsetX = 0; let offsetY = 0; let width = 0; @@ -594,7 +611,8 @@ export class PanelPositionControls { offsetY = startY - rect.top; width = rect.width; height = rect.height; - this._liftPanelOut(panelId, panelEl, rect); + before = geometryOf(panelEl); + lifted = this._liftPanelOut(panelId, panelEl, rect); panelEl.classList.add('panel-dragging'); this._promotePanelZ(panelEl); } @@ -615,7 +633,7 @@ export class PanelPositionControls { panelEl.classList.remove('panel-dragging'); window.removeEventListener('pointermove', onMove); window.removeEventListener('pointerup', onUp); - window.removeEventListener('pointercancel', onUp); + window.removeEventListener('pointercancel', onCancel); this._cancelDrag = null; }; const onUp = () => { @@ -628,14 +646,33 @@ export class PanelPositionControls { this._savePanelPosition(panelId, panelEl); this._onPanelResized?.(panelId); }; + const onCancel = () => { + cancel(); + if (dragging) + this._revertPanelGesture(panelId, panelEl, lifted, before); + }; this._cancelDrag = cancel; window.addEventListener('pointermove', onMove); window.addEventListener('pointerup', onUp); - window.addEventListener('pointercancel', onUp); + window.addEventListener('pointercancel', onCancel); }); } + /** + * The browser took a gesture back (pointercancel): leave no trace and save + * nothing. A panel the gesture lifted returns to its rail; a window that + * was already floating returns to where it was. + */ + _revertPanelGesture(panelId, panelEl, lifted, before) { + if (lifted) { + this._resetPanelPosition(panelId); + return; + } + Object.assign(panelEl.style, before); + this._onPanelResized?.(panelId); + } + /** * Append a resize handle for every edge and corner. Seven are invisible * strips; the bottom-right corner shows a grip. Resizing a docked panel @@ -661,63 +698,76 @@ export class PanelPositionControls { } _startPanelResize(event, panelId, panelEl, dir, limits) { - if (event.button !== 0) return; + if (event.button !== 0 || !isPrecisePointer(event)) return; event.preventDefault(); this._cancelDrag?.(); - const rect = panelEl.getBoundingClientRect(); - this._liftPanelOut(panelId, panelEl, rect); - const startBox = { - left: Math.round(rect.left), - top: Math.round(rect.top), - width: Math.round(rect.width), - height: Math.round(rect.height), - }; const startX = event.clientX; const startY = event.clientY; + // Set once the pointer has travelled: a plain click on an edge (or on + // the globe just beside a docked panel) must not lift it out. + let startBox = null; + let lifted = false; + let before = null; const apply = ({ left, top, width, height }) => { panelEl.style.left = `${left}px`; panelEl.style.top = `${top}px`; panelEl.style.width = `${width}px`; panelEl.style.height = `${height}px`; }; - panelEl.classList.add('panel-resizing'); - this._promotePanelZ(panelEl); - apply(startBox); + const begin = () => { + const rect = panelEl.getBoundingClientRect(); + before = geometryOf(panelEl); + lifted = this._liftPanelOut(panelId, panelEl, rect); + startBox = { + left: Math.round(rect.left), + top: Math.round(rect.top), + width: Math.round(rect.width), + height: Math.round(rect.height), + }; + panelEl.classList.add('panel-resizing'); + this._promotePanelZ(panelEl); + apply(startBox); + }; const onMove = (moveEvent) => { + const dx = moveEvent.clientX - startX; + const dy = moveEvent.clientY - startY; + if (!startBox) { + if (Math.hypot(dx, dy) < DRAG_THRESHOLD_PX) return; + begin(); + } moveEvent.preventDefault(); apply( - resizeBox( - startBox, - dir, - moveEvent.clientX - startX, - moveEvent.clientY - startY, - { - minWidth: limits.width, - minHeight: limits.height, - viewportWidth: window.innerWidth, - viewportHeight: window.innerHeight, - }, - ), + resizeBox(startBox, dir, dx, dy, { + minWidth: limits.width, + minHeight: limits.height, + viewportWidth: window.innerWidth, + viewportHeight: window.innerHeight, + }), ); }; const cancel = () => { panelEl.classList.remove('panel-resizing'); window.removeEventListener('pointermove', onMove); window.removeEventListener('pointerup', onUp); - window.removeEventListener('pointercancel', onUp); + window.removeEventListener('pointercancel', onCancel); this._cancelDrag = null; }; const onUp = () => { cancel(); + if (!startBox) return; this._savePanelPosition(panelId, panelEl); this._onPanelResized?.(panelId); }; + const onCancel = () => { + cancel(); + if (startBox) this._revertPanelGesture(panelId, panelEl, lifted, before); + }; this._cancelDrag = cancel; window.addEventListener('pointermove', onMove); window.addEventListener('pointerup', onUp); - window.addEventListener('pointercancel', onUp); + window.addEventListener('pointercancel', onCancel); } destroy() { diff --git a/src/ui/streetLevelControls.js b/src/ui/streetLevelControls.js index 4694518f4..77866f786 100644 --- a/src/ui/streetLevelControls.js +++ b/src/ui/streetLevelControls.js @@ -121,8 +121,13 @@ export class StreetLevelControls { } catch { /* storage unavailable */ } - // The expanded viewer is a dialog: Esc closes it, Tab stays inside. - this.listen(el.viewerWrap, 'keydown', (event) => this._onDialogKey(event)); + // The expanded viewer is a dialog: Esc shrinks it, Tab stays inside. + // Listen on the document, ahead of everything else, so both still hold + // once focus has left it (a click on the globe) and Esc never reaches + // the map's own handler, which would clear the selected sequence. + this.listen(document, 'keydown', (event) => this._onDialogKey(event), { + capture: true, + }); // MapillaryJS only tracks window resizes; the panel resizes on its own. if (typeof ResizeObserver === 'function' && el.viewer) { let queued = false; @@ -258,9 +263,15 @@ export class StreetLevelControls { this._wrapHome = null; const target = this._expandReturnFocus; this._expandReturnFocus = null; - if (target?.isConnected && typeof target.focus === 'function') - target.focus({ preventScroll: true }); - else this._elements.viewerExpand?.focus?.({ preventScroll: true }); + // A closed image hides the wrap: focus inside it would drop to . + const reachable = (node) => + node?.isConnected && + typeof node.focus === 'function' && + !(wrap.hidden && wrap.contains(node)); + if (reachable(target)) target.focus({ preventScroll: true }); + else if (reachable(this._elements.viewerExpand)) + this._elements.viewerExpand.focus({ preventScroll: true }); + else this._focusPanelControl(); } const button = this._elements.viewerExpand; if (button) { @@ -275,6 +286,14 @@ export class StreetLevelControls { requestAnimationFrame(() => this.layer.resizeViewer?.()); } + /** Park focus on a control that stays on screen while the viewer is hidden. */ + _focusPanelControl() { + const el = this._elements; + const target = + el.status || el.providerChips?.querySelector('.data-toggle-chip'); + target?.focus?.({ preventScroll: true }); + } + _onDialogKey(event) { if (!this.isViewerExpanded()) return; if (event.key === 'Escape') { @@ -391,7 +410,19 @@ export class StreetLevelControls { _renderViewer(view, state) { const el = this._elements; const { viewer } = view; - if (el.viewerWrap) el.viewerWrap.hidden = !viewer.open; + const wrap = el.viewerWrap; + if (wrap) { + // Closing the image (its × button) hides the wrap: move focus out + // first, or it drops to . A shrink below returns it itself. + if ( + !viewer.open && + !wrap.hidden && + !this.isViewerExpanded() && + wrap.contains(document.activeElement) + ) + this._focusPanelControl(); + wrap.hidden = !viewer.open; + } if (!viewer.open && this.isViewerExpanded()) this.setViewerExpanded(false); if (el.viewerPlaceholder) el.viewerPlaceholder.hidden = !viewer.loading; if (el.followBtn) { diff --git a/src/ui/streetLevelControls.test.mjs b/src/ui/streetLevelControls.test.mjs index c1341468d..1cbfc390b 100644 --- a/src/ui/streetLevelControls.test.mjs +++ b/src/ui/streetLevelControls.test.mjs @@ -66,6 +66,9 @@ class FakeNode { getAttribute(key) { return this.attributes.get(key) ?? null; } + removeAttribute(key) { + this.attributes.delete(key); + } appendChild(child) { return this.insertBefore(child, null); } @@ -95,6 +98,12 @@ class FakeNode { if (current === this) return true; return false; } + get isConnected() { + return this.ownerDocument?.contains(this) === true; + } + focus() { + this.ownerDocument.activeElement = this; + } closest(selector) { const name = selector.replace(/^\./, ''); for (let node = this; node; node = node.parent) @@ -110,18 +119,33 @@ class FakeNode { querySelector(selector) { return this.querySelectorAll(selector)[0] || null; } - querySelectorAll(selector) { - const byId = /^#(.+)$/.exec(selector); - const byData = /^\[data-([a-z-]+)\]$/.exec(selector); - const key = byData?.[1].replace(/-([a-z])/g, (_, c) => c.toUpperCase()); - return [...this.walk()].filter((node) => - byId ? node.id === byId[1] : key ? key in node.dataset : false, - ); + /** `#id`, `[data-x]`, and the tag / attribute forms of a focusable list. */ + matches(selector) { + return selector.split(',').some((part) => { + const one = part.trim(); + const byId = /^#(.+)$/.exec(one); + if (byId) return this.id === byId[1]; + const byData = /^\[data-([a-z-]+)\]$/.exec(one); + if (byData) + return ( + byData[1].replace(/-([a-z])/g, (_, c) => c.toUpperCase()) in + this.dataset + ); + if (one === '[tabindex]:not([tabindex="-1"])') + return ![null, '-1'].includes(this.getAttribute('tabindex')); + const byTag = /^([a-z]+)(\[href\])?(:not\(\[disabled\]\))?$/.exec(one); + if (!byTag || this.tagName !== byTag[1].toUpperCase()) return false; + if (byTag[2] && this.getAttribute('href') === null) return false; + return !(byTag[3] && this.disabled); + }); } - addEventListener(type, listener, { signal } = {}) { + querySelectorAll(selector) { + return [...this.walk()].filter((node) => node.matches(selector)); + } + addEventListener(type, listener, { signal, capture = false } = {}) { if (signal?.aborted) return; - if (!this.listeners.has(type)) this.listeners.set(type, new Set()); - this.listeners.get(type).add(listener); + if (!this.listeners.has(type)) this.listeners.set(type, new Map()); + this.listeners.get(type).set(listener, capture); signal?.addEventListener('abort', () => this.listeners.get(type)?.delete(listener), ); @@ -129,16 +153,45 @@ class FakeNode { removeEventListener(type, listener) { this.listeners.get(type)?.delete(listener); } - /** Deliver to this node, then bubble to its ancestors with the same target. */ + /** + * Capture down from the document, then deliver to this node and bubble + * back up with the same target; `stopPropagation` ends the trip. + */ dispatchEvent(event) { + let stopped = false; const delivered = { type: event.type, target: this, bubbles: event.bubbles, + key: event.key, + shiftKey: event.shiftKey === true, + defaultPrevented: false, + preventDefault() { + this.defaultPrevented = true; + }, + stopPropagation() { + stopped = true; + }, }; - for (let node = this; node; node = delivered.bubbles ? node.parent : null) - for (const listener of [...(node.listeners.get(event.type) || [])]) - listener(delivered); + const path = []; + for (let node = this; node; node = node.parent) path.push(node); + const run = (node, phase) => { + for (const [listener, capture] of [ + ...(node.listeners.get(event.type) || []), + ]) + if (phase === 'target' || capture === (phase === 'capture')) + listener(delivered); + }; + for (const node of path.slice(1).reverse()) { + run(node, 'capture'); + if (stopped) return true; + } + run(this, 'target'); + if (!delivered.bubbles) return true; + for (const node of path.slice(1)) { + if (stopped) return true; + run(node, 'bubble'); + } return true; } click() { @@ -148,39 +201,48 @@ class FakeNode { /** The Street Level panel's body, as layer-panels.html lays it out. */ function panelDom() { - const document = { + const document = new FakeNode(null, '#document'); + Object.assign(document, { + ownerDocument: document, activeElement: null, - body: null, createElement: (tag) => new FakeNode(document, tag), - }; - document.body = new FakeNode(document, 'body'); - const root = new FakeNode(document, 'section', { id: 'street-level-panel' }); + }); + document.body = document.appendChild(new FakeNode(document, 'body')); + const root = document.body.appendChild( + new FakeNode(document, 'section', { id: 'street-level-panel' }), + ); const add = (parent, tag, options) => parent.appendChild(new FakeNode(document, tag, options)); + add(root, 'button', { id: 'sl-status' }); for (const id of [ - 'sl-status', 'sl-provider-chips', 'sl-error', 'sl-error-text', 'sl-since', 'sl-since-label', 'sl-legend', - 'sl-follow-btn', - 'sl-viewer-expand', - 'sl-viewer-close', - 'sl-viewer-placeholder', - 'sl-image-by', - 'sl-image-when', - 'sl-image-link', 'sl-coverage-meta', ]) add(root, id === 'sl-legend' ? 'ul' : 'div', { id }); const controls = add(root, 'fieldset', { id: 'sl-controls' }); for (const pano of ['all', 'pano', 'flat']) add(controls, 'button', { dataset: { slPano: pano } }); - const wrap = add(root, 'div', { id: 'sl-viewer-wrap' }); - add(wrap, 'div', { id: 'sl-viewer' }); - return { document, root }; + // The viewer's own tool bar lives inside the wrap, as in the markup. + const wrap = add(controls, 'div', { id: 'sl-viewer-wrap' }); + for (const id of ['sl-viewer-expand', 'sl-follow-btn', 'sl-viewer-close']) + add(wrap, 'button', { id }); + for (const id of [ + 'sl-viewer-placeholder', + 'sl-viewer', + 'sl-image-by', + 'sl-image-when', + 'sl-image-link', + ]) + add(wrap, id === 'sl-image-link' ? 'a' : 'div', { id }); + // The globe's canvas is focusable too (tabindex=0). + const globe = add(document.body, 'canvas'); + globe.setAttribute('tabindex', '0'); + return { document, root, globe }; } /** Run `fn` with the fake document and an immediate animation frame. */ @@ -504,3 +566,78 @@ test('the viewer is resized when it opens, not on every render (review #8)', () layer.publish(uiState({ open: true })); assert.equal(layer.calls.resize, 2, 'the next photo resizes again'); })); + +/* ── The expanded viewer as a dialog, and where focus goes ─────────────── */ + +const keydown = (target, key, { shiftKey = false } = {}) => + target.dispatchEvent({ type: 'keydown', bubbles: true, key, shiftKey }); + +/** The map's own Esc handler (selection.js) listens on the document. */ +function mapEscape(dom) { + const reached = []; + dom.document.addEventListener('keydown', (event) => { + if (event.key === 'Escape') reached.push(event); + }); + return reached; +} + +test('Esc shrinks the expanded viewer after focus left it, and the map keeps its selection', () => + withDom(async (dom) => { + const { controls } = stubPanel(dom, uiState({ open: true })); + const reached = mapEscape(dom); + controls.setViewerExpanded(true); + dom.globe.focus(); // a click on the globe + keydown(dom.globe, 'Escape'); + assert.equal(controls.isViewerExpanded(), false); + assert.equal(reached.length, 0, 'the selected sequence was not cleared'); + keydown(dom.globe, 'Escape'); + assert.equal(reached.length, 1, 'once shrunk, Esc is the map’s again'); + controls.destroy(); + })); + +test('Tab from outside the expanded viewer brings focus back into it', () => + withDom(async (dom) => { + const { controls } = stubPanel(dom, uiState({ open: true })); + controls.setViewerExpanded(true); + dom.globe.focus(); + keydown(dom.globe, 'Tab'); + const wrap = dom.document.body.querySelector('#sl-viewer-wrap'); + assert.equal( + dom.document.activeElement, + wrap.querySelector('#sl-viewer-expand'), + ); + keydown(dom.document.activeElement, 'Tab', { shiftKey: true }); + assert.equal( + dom.document.activeElement, + wrap.querySelector('#sl-viewer-close'), + 'Shift+Tab wraps to the last control', + ); + controls.destroy(); + })); + +test('closing the image from its × button leaves focus on the panel, not ', () => + withDom(async (dom) => { + const { layer, controls } = stubPanel(dom, uiState({ open: true })); + dom.root.querySelector('#sl-viewer-close').focus(); + layer.publish(uiState({ open: false })); + assert.equal(dom.root.querySelector('#sl-viewer-wrap').hidden, true); + assert.equal( + dom.document.activeElement, + dom.root.querySelector('#sl-status'), + ); + controls.destroy(); + })); + +test('an image closed while expanded does not return focus into the hidden viewer', () => + withDom(async (dom) => { + const { layer, controls } = stubPanel(dom, uiState({ open: true })); + dom.root.querySelector('#sl-viewer-expand').focus(); + controls.setViewerExpanded(true); // remembers EXPAND to return to + layer.publish(uiState({ open: false })); + assert.equal(controls.isViewerExpanded(), false); + assert.equal( + dom.document.activeElement, + dom.root.querySelector('#sl-status'), + ); + controls.destroy(); + }));