diff --git a/CHANGELOG.md b/CHANGELOG.md index 5609b843d..7d16fc822 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ of current runtime behavior, see [`docs/CURRENT-STATE.md`](docs/CURRENT-STATE.md ## [Unreleased] +- Extract Location controls and cancellable search presentation; preserve navigation handoff and prevent delayed POI expansion after closing the row. + - Separate Layers panel presentation and clear-control bindings from layer lifecycle operations; revoke listeners and subscriptions on replacement or teardown. - Extract Map Source controls with listener cleanup and protection against obsolete selection feedback. diff --git a/docs/CODE-BOUNDARIES.md b/docs/CODE-BOUNDARIES.md index 9eec38dc5..78f04247d 100644 --- a/docs/CODE-BOUNDARIES.md +++ b/docs/CODE-BOUNDARIES.md @@ -279,3 +279,11 @@ the caller. Hidden-page refresh scheduling remains an explicit callback. `ui/layers/feedback` exposes the existing pure loading/notice reducers separately from DOM controls. Scheduling and presentation stay with their callers. + +## Location controls + +`ui/location` exports Location controls, the cancellable lookup controller and +the existing location-status formatter. Callers supply city data, search and +navigation operations. The component owns DOM listeners and pending expansion; +it imports no geocoder, camera engine, layer or application bootstrap. Existing +camera authority and search providers remain supplied by the application. diff --git a/docs/CURRENT-STATE.md b/docs/CURRENT-STATE.md index 7c5b2ec96..85d03ca5c 100644 --- a/docs/CURRENT-STATE.md +++ b/docs/CURRENT-STATE.md @@ -1,5 +1,15 @@ # God's Eye View Current State +## Location control ownership + +City/POI rows, search/reset bindings, location readouts and the orbit indicator +have a dedicated component with explicit navigation actions. A separate lookup +controller cancels superseded searches and checks camera authority before flight +and result presentation. Existing search providers and camera handoff policy are +preserved. Replacing or closing a POI row cancels its pending expansion frame; +destruction releases listeners, pending searches and the orbit indicator. + + ## Layer panel ownership Layer rows, feed feedback, counts, focus-preserving chips and toggle listeners diff --git a/package.json b/package.json index e78bcbf8b..57d264802 100644 --- a/package.json +++ b/package.json @@ -128,6 +128,7 @@ "./ui/effects/bloom": "./src/bloom.js", "./ui/maps": "./src/ui/mapSource.js", "./ui/layers": "./src/ui/layers.js", - "./ui/layers/feedback": "./src/loadingFeedback.js" + "./ui/layers/feedback": "./src/loadingFeedback.js", + "./ui/location": "./src/ui/location.js" } } diff --git a/scripts/format-scope.json b/scripts/format-scope.json index 2dfafec41..8aa998b56 100644 --- a/scripts/format-scope.json +++ b/scripts/format-scope.json @@ -159,5 +159,12 @@ "src/ui/clearLayersControl.js", "scripts/qa-layer-panel.mjs", "src/loadingFeedback.js", - "src/data/installationFeedback.js" + "src/data/installationFeedback.js", + "src/ui/location.js", + "src/ui/locationControls.js", + "src/ui/locationSearch.js", + "src/locationStatus.js", + "src/ui/locationControls.test.mjs", + "src/ui/locationSearch.test.mjs", + "scripts/qa-location-controls.mjs" ] diff --git a/scripts/package-boundaries.json b/scripts/package-boundaries.json index 971bed61b..9804b619e 100644 --- a/scripts/package-boundaries.json +++ b/scripts/package-boundaries.json @@ -336,5 +336,15 @@ "src/data/installationFeedback.js" ], "external": [] + }, + "location-controls": { + "exports": ["./ui/location"], + "modules": [ + "src/ui/location.js", + "src/ui/locationControls.js", + "src/ui/locationSearch.js", + "src/locationStatus.js" + ], + "external": [] } } diff --git a/scripts/qa-location-controls.mjs b/scripts/qa-location-controls.mjs new file mode 100644 index 000000000..4c3248ce8 --- /dev/null +++ b/scripts/qa-location-controls.mjs @@ -0,0 +1,242 @@ +#!/usr/bin/env node +/** Location UI acceptance; controlled search results isolate UI races from geocoder availability. */ +import fs from 'node:fs'; +import puppeteer from 'puppeteer'; +const browser = await puppeteer.launch({ + headless: true, + args: [ + '--no-sandbox', + ...(process.platform === 'darwin' + ? ['--use-angle=metal', '--enable-gpu'] + : ['--use-gl=angle', '--use-angle=swiftshader']), + ], +}); +const page = await browser.newPage(); +const errors = []; +page.on('pageerror', (e) => errors.push(e.message)); +let failures = 0; +const check = (name, ok) => { + console.log(`[${ok ? 'PASS' : 'FAIL'}] ${name}`); + if (!ok) failures++; +}; +try { + await page.setViewport({ width: 1440, height: 900 }); + await page.goto( + `${process.env.QA_BASE_URL || 'http://localhost:4173'}/?welcome=0`, + { waitUntil: 'domcontentloaded' }, + ); + await page.waitForFunction( + () => + window.__godsEyeView?.styleManager?._locationControls && + document.getElementById('loading-screen')?.classList.contains('hidden'), + { timeout: 60000 }, + ); + await page.click('#location-bar-toggle'); + await page.click('[data-pin-target="location-bar"]'); + const cityId = await page.$eval( + '.location-pill', + (node) => node.dataset.locationId, + ); + await page.click('.location-pill'); + await page.waitForFunction(() => + document.getElementById('poi-row').classList.contains('expanded'), + ); + check( + 'native city selection highlights and expands its POIs', + await page.evaluate( + (id) => + window.__godsEyeView.styleManager._activeLocationId === id && + document.querySelector('.location-pill.active')?.dataset.locationId === + id, + cityId, + ), + ); + await page.keyboard.press('w'); + check( + 'POI keyboard action selects the second landmark', + await page.evaluate( + () => + window.__godsEyeView.styleManager._activePoiIndex === 1 && + document.querySelector('.poi-pill.active')?.dataset.poiIndex === '1', + ), + ); + check( + 'orbit controls and indicator agree', + await page.evaluate(() => { + const ui = window.__godsEyeView.styleManager; + ui.setOrbit(true); + const active = + ui.orbitController.active && + document.getElementById('orbit-indicator').classList.contains('active'); + ui.setOrbit(false); + return ( + active && + !ui.orbitController.active && + !document.getElementById('orbit-indicator').classList.contains('active') + ); + }), + ); + await page.click('#search-toggle'); + await page.type('#location-search', 'qwerty'); + check( + 'typing in search leaves POI selection unchanged', + await page.evaluate( + () => window.__godsEyeView.styleManager._activePoiIndex === 1, + ), + ); + await page.evaluate(() => { + const lookup = window.__godsEyeView.styleManager._locationLookup; + window.__qaSearchRequests = []; + lookup.search = (query, options) => + new Promise((resolve) => + window.__qaSearchRequests.push({ query, options, resolve }), + ); + document.getElementById('location-search').value = 'First'; + }); + await page.keyboard.press('Enter'); + await page.waitForFunction(() => window.__qaSearchRequests.length === 1); + await page.evaluate(() => { + document.getElementById('location-search').value = 'Second'; + document.getElementById('location-search').focus(); + }); + await page.keyboard.press('Enter'); + await page.waitForFunction(() => window.__qaSearchRequests.length === 2); + check( + 'second native submit aborts the first request and keeps current busy state', + await page.evaluate( + () => + window.__qaSearchRequests[0].options.signal.aborted && + document + .getElementById('location-search') + .classList.contains('searching'), + ), + ); + await page.evaluate(() => + window.__qaSearchRequests[1].resolve({ + label: 'Second landmark, Test city', + }), + ); + await page.waitForFunction( + () => + window.__godsEyeView.styleManager._searchedLocationLabel === + 'Second landmark, Test city', + ); + await page.evaluate(() => + window.__qaSearchRequests[0].resolve({ + label: 'First landmark, Test city', + }), + ); + check( + 'late result cannot overwrite the current location readout or style', + await page.evaluate(() => { + const ui = window.__godsEyeView.styleManager; + return ( + ui._searchedLocationLabel === 'Second landmark, Test city' && + ui._locationMiniCity.textContent.includes('Second landmark') && + ui._locationMiniPoi.textContent === 'Test city' && + ui.activeStyle === 'normal' && + !ui._locationSearch.classList.contains('searching') + ); + }), + ); + check( + 'cancelled POI expansion stays closed after a frame', + await page.evaluate(async (id) => { + const ui = window.__godsEyeView.styleManager; + ui._expandPOIRow(id); + ui._collapsePOIRow(); + await new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(resolve)), + ); + return ( + !ui._poiRow.classList.contains('expanded') && + !ui._locationBarDivider.classList.contains('visible') + ); + }, cityId), + ); + await page.evaluate( + (id) => window.__godsEyeView.styleManager._onCityPillClick(id), + cityId, + ); + await page.waitForFunction(() => + document.getElementById('poi-row').classList.contains('expanded'), + ); + // Finish the camera's existing city flight before inspecting both layouts. + await page.waitForFunction( + () => !window.__godsEyeView.viewer.camera._currentFlight, + { timeout: 10000 }, + ); + await page.evaluate( + () => + new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(resolve)), + ), + ); + const settled = await page + .waitForFunction( + () => { + const scene = window.__godsEyeView.viewer.scene; + for (let index = 0; index < scene.primitives.length; index++) { + const primitive = scene.primitives.get(index); + if ( + typeof primitive.tilesLoaded === 'boolean' && + !primitive.tilesLoaded + ) + return false; + } + return true; + }, + { timeout: 60000 }, + ) + .then( + () => true, + () => false, + ); + check('visible city tiles settle before screenshots', settled); + fs.mkdirSync('qa-shots/location-controls', { recursive: true }); + await page.screenshot({ path: 'qa-shots/location-controls/desktop.png' }); + await page.setViewport({ width: 620, height: 900 }); + await page.waitForFunction( + () => + document.getElementById('left-panel-stack')?.dataset.layoutMode === + 'mobile', + ); + await page.screenshot({ path: 'qa-shots/location-controls/narrow.png' }); + check( + 'native reset returns through the existing camera handoff', + await page.evaluate(async () => { + const ui = window.__godsEyeView.styleManager; + ui._resetGlobeBtn.click(); + const pending = ui._globeResetPromise; + if (!pending) return false; + await pending; + return ( + !ui.orbitController.active && !window.__godsEyeView.viewer.trackedEntity + ); + }), + ); + check( + 'destroyed controls cannot navigate or submit another lookup', + await page.evaluate(() => { + const ui = window.__godsEyeView.styleManager; + const count = window.__qaSearchRequests.length; + ui._locationControls.destroy(); + ui._locationLookup.destroy(); + const before = ui._navigationGeneration; + ui._locationPills.querySelector('button').click(); + ui._resetGlobeBtn.click(); + ui._locationSearch.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }), + ); + return ( + before === ui._navigationGeneration && + count === window.__qaSearchRequests.length + ); + }), + ); + check('no uncaught browser errors', errors.length === 0); +} finally { + await browser.close(); +} +console.log(`RESULT: ${failures} failures`); +process.exitCode = failures ? 1 : 0; diff --git a/src/cameraHandoff.test.mjs b/src/cameraHandoff.test.mjs index e7d5b0268..39b1b27d0 100644 --- a/src/cameraHandoff.test.mjs +++ b/src/cameraHandoff.test.mjs @@ -243,22 +243,22 @@ test('validated voice camera destinations share the UI navigation authority faca }); test('deferred search releases only after its final authority check', () => { - const handler = body( - ui, - /this\._locationSearch\.addEventListener\('keydown', async \(e\) => \{([\s\S]*?)\n \}\);/, - 'search handler', - ); - ordered(handler, [ - "this._beginDeferredNavigation('location')", - 'this._activeLocationSearchGeneration = generation;', - 'searchAndFlyTo(this.viewer, query', - 'beforeFly: () => this._reassertNavigationHandoff(generation)', - 'generation !== this._navigationGeneration', + const search = fs.readFileSync(path.join(ROOT, 'src', 'ui', 'locationSearch.js'), 'utf8'); + ordered(search, [ + 'const authority = this.begin();', + 'this.onStart(authority);', + 'await this.search(query', + 'beforeFly: () => current() && this.beforeFly(authority)', + 'if (!current() || controller.signal.aborted) return;', 'destination?.cancelled', 'finally', - 'this._settleLocationSearchUi(generation)', - ], 'deferred search'); - assert.doesNotMatch(handler.slice(0, handler.indexOf('searchAndFlyTo')), /_releaseFollowCamera/); + 'this.onSettled(authority)', + ], 'deferred search component'); + assert.match(ui, /begin: \(\) => this\._beginDeferredNavigation\('location'\)/); + assert.match(ui, /beforeFly: \(generation\) => this\._reassertNavigationHandoff\(generation\)/); + assert.match(ui, /isCurrent: \(generation\) => !this\._disposed && generation === this\._navigationGeneration/); + assert.match(ui, /onSettled: \(generation\) => this\._settleLocationSearchUi\(generation\)/); + assert.doesNotMatch(search.slice(0, search.indexOf('await this.search')), /_releaseFollowCamera/); }); test('a direct globe gesture retires delayed camera and selection restore only', () => { @@ -337,25 +337,18 @@ test('teardown synchronously closes immediate camera entry points', () => { }); test('teardown refuses deferred location work before geocoding begins', () => { - const deferred = body( - ui, - /_beginDeferredNavigation\(noun = 'location', \{ cancelPendingSelection = true \} = \{\}\) \{([\s\S]*?)\n \}/, - 'deferred navigation', - ); + const deferred = body(ui, /_beginDeferredNavigation\(noun = 'location', \{ cancelPendingSelection = true \} = \{\}\) \{([\s\S]*?)\n \}/, 'deferred navigation'); assert.match(deferred, /disposed: this\._disposed/); - - const handler = body( - ui, - /this\._locationSearch\.addEventListener\('keydown', async \(e\) => \{([\s\S]*?)\n \}\);/, - 'search handler', - ); - ordered(handler, [ - "const generation = this._beginDeferredNavigation('location');", - 'if (generation === false)', - 'this._locationSearch.blur();', - 'searchAndFlyTo(this.viewer, query', + const search = fs.readFileSync(path.join(ROOT, 'src', 'ui', 'locationSearch.js'), 'utf8'); + ordered(search, [ + 'if (!query || this.destroyed) return;', + 'const authority = this.begin();', + 'if (authority === false)', + 'this.input.blur();', + 'this.onStart(authority);', + 'await this.search(query', ], 'disposed search refusal'); - assert.match(handler, /if \(generation === false\) \{[\s\S]*?return;[\s\S]*?\}\s*this\._activeLocationSearchGeneration/); + assert.match(search, /if \(authority === false\) \{[\s\S]*?return;[\s\S]*?\}\s*this\.controller/); }); test('refused canned destinations commit no location or POI state', () => { diff --git a/src/cockpitMarkup.test.mjs b/src/cockpitMarkup.test.mjs index 7edd4aea2..515d7da64 100644 --- a/src/cockpitMarkup.test.mjs +++ b/src/cockpitMarkup.test.mjs @@ -468,11 +468,11 @@ test('Reset releases Contact camera ownership through its selection-preserving r assert.ok(contextRelease > resetStart); assert.ok(satelliteRelease > contextRelease); assert.match(ui, /this\._cockpitResetGlobeBtn = document\.getElementById\('cockpit-reset-globe'\)/); - assert.match( - ui, - /for \(const button of \[this\._resetGlobeBtn, this\._cockpitResetGlobeBtn\]\) \{[\s\S]*?addEventListener\('click', this\._globeResetHandler\)/, - 'both reset controls must delegate to the one shared reset route', - ); + const controls = fs.readFileSync(path.join(ROOT, 'src', 'ui', 'locationControls.js'), 'utf8'); + assert.match(ui, /resetButtons: \[this\._resetGlobeBtn, this\._cockpitResetGlobeBtn\]/); + assert.match(ui, /onReset: \(\) => this\.resetToGlobeView\(\)/); + assert.match(controls, /for \(const button of elements\.resetButtons\)[\s\S]*?this\.bind\(button, 'click', onReset\)/, + 'both reset controls delegate to the same supplied reset action'); assert.match(ui, /if \(this\.resetGlobeButton\) this\.resetGlobeButton\.hidden = false;/); assert.match(ui, /if \(this\.resetGlobeButton\) this\.resetGlobeButton\.hidden = true;/); }); @@ -496,7 +496,10 @@ test('Location navigation releases immediate routes before flight and deferred r const poiHandler = ui.slice(ui.indexOf('_onPoiClick(cityId, poiIndex) {'), ui.indexOf('_expandPOIRow(cityId) {')); assert.match(cityHandler, /if \(result === false\) return;[\s\S]*?_setActiveLocation/); assert.match(poiHandler, /if \(result === false\) return;[\s\S]*?_setActiveLocation/); - assert.match(ui.slice(search, search + 320), /beforeFly: \(\) => this\._reassertNavigationHandoff\(generation\)/); + assert.match(ui, /beforeFly: \(generation\) => this\._reassertNavigationHandoff\(generation\)/); + assert.match(ui.slice(search, search + 180), /\.\.\.options/); + const lookup = fs.readFileSync(path.join(ROOT, 'src', 'ui', 'locationSearch.js'), 'utf8'); + assert.match(lookup, /beforeFly: \(\) => current\(\) && this\.beforeFly\(authority\)/); assert.ok(voiceStart >= 0, 'voice Location must use the same handoff'); assert.match(voiceActions, /styleManager\.reassertDeferredLocationNavigation\(generation\)/); }); diff --git a/src/locationReadouts.test.mjs b/src/locationReadouts.test.mjs index 8bcbf6955..62c11e36f 100644 --- a/src/locationReadouts.test.mjs +++ b/src/locationReadouts.test.mjs @@ -44,11 +44,13 @@ test('a free-text search records its destination for the LOCATION mini-status', }); test('the mini-status reads its copy from the shared formatter', () => { - assert.match(ui, /import \{ locationMiniStatus \} from '\.\/locationStatus\.js';/); + const controls = fs.readFileSync(path.join(ROOT, 'src', 'ui', 'locationControls.js'), 'utf8'); + assert.match(controls, /import \{ locationMiniStatus \} from '\.\.\/locationStatus\.js';/); + assert.match(controls, /const lines = locationMiniStatus\(state\)/); const start = ui.indexOf(' _updateLocationMiniStatus() {'); assert.ok(start > 0, '_updateLocationMiniStatus is missing'); const body = ui.slice(start, ui.indexOf('\n }', start)); - assert.match(body, /locationMiniStatus\(\{[\s\S]*?searchedLabel: this\._searchedLocationLabel,[\s\S]*?\}\)/); + assert.match(body, /_locationControls\?\.renderStatus\(\{[\s\S]*?searchedLabel: this\._searchedLocationLabel,[\s\S]*?\}\)/); // No second copy of the placeholder strings to drift out of sync. assert.doesNotMatch(body, /Location: --/); }); diff --git a/src/locationStatus.js b/src/locationStatus.js index 2dcc80fcc..1d90582c9 100644 --- a/src/locationStatus.js +++ b/src/locationStatus.js @@ -55,7 +55,10 @@ export function locationMiniStatus({ // The remaining address is the place's context ("Japan", "Minato City, // Tokyo, Japan"); the readout is ellipsised in CSS, so a long tail is // safe. A one-segment geocode ("Japan") has no context to show. - poi: segments.length > 1 ? segments.slice(1).join(', ') : 'Searched location', + poi: + segments.length > 1 + ? segments.slice(1).join(', ') + : 'Searched location', }; } diff --git a/src/sharelink.celestial.test.mjs b/src/sharelink.celestial.test.mjs index 5c1df1fc9..fd076cbcf 100644 --- a/src/sharelink.celestial.test.mjs +++ b/src/sharelink.celestial.test.mjs @@ -693,6 +693,8 @@ test('visual input listeners are revoked before asynchronous UI teardown', () => assert.match(synchronous, /this\._visualEffects\.stop\(\)/); assert.match(synchronous, /this\._mapSourceControls\?\.destroy\(\)/); assert.match(synchronous, /this\._clearLayersControl\?\.destroy\(\)/); + assert.match(synchronous, /this\._locationControls\?\.destroy\(\)/); + assert.match(synchronous, /this\._locationLookup\?\.destroy\(\)/); assert.match(synchronous, /this\._displayControls\?\.destroy\(\)/); assert.match(synchronous, /this\._styleParameters\?\.destroy\(\)/); }); diff --git a/src/ui.js b/src/ui.js index 9cee03d13..a7abdff95 100644 --- a/src/ui.js +++ b/src/ui.js @@ -1,3 +1,4 @@ +import { LocationControls, LocationSearch } from './ui/location.js'; import { bindClearLayersControl } from './ui/layers.js'; import { createMapSourceControls } from './ui/mapSource.js'; import { VisualEffects, STYLES, GLOBAL_POST_DEFAULTS, STYLE_PRESET_DEFAULTS, MILITARY_DETECTION_PRESET } from './ui/effects.js'; @@ -13,7 +14,6 @@ import { decodeBloomIntensity, } from './bloom.js'; import { LOCATIONS, CITY_POIS, GLOBE_VIEW, flyToGlobeView, flyToPresetLocation, flyToPOI, searchAndFlyTo } from './locations.js'; -import { locationMiniStatus } from './locationStatus.js'; import { interruptCameraMotion } from './cameraVerbs.js'; import { aircraftTrackingTarget, @@ -2085,7 +2085,6 @@ export class StyleManager { this._shareTrackingAcquiringKey = null; this._shareTrackingNoticeGeneration = 0; this._globeResetPromise = null; - this._globeResetHandler = null; this._clearSelectedLayersPromise = null; this._clearSelectedLayersManagerPromise = null; this._dataManager = null; @@ -2565,7 +2564,6 @@ export class StyleManager { this._initLocationBar(); this._initShareButton(); this._initClearSelectedLayersButton(); - this._initResetGlobeButton(); this._initHUDToggle(); this._initModels3dToggle(); this._applyGlobalPostDefaults(); @@ -8528,97 +8526,45 @@ export class StyleManager { * @returns {void} */ _initLocationBar() { - const QWERTY_KEYS = ['Q', 'W', 'E', 'R', 'T']; - - // Render city pills (no submenu wrappers — POI row is separate) - for (const [cityId, city] of Object.entries(CITY_POIS)) { - const pill = document.createElement('button'); - pill.className = 'location-pill'; - pill.dataset.locationId = cityId; - pill.textContent = city.name; - pill.addEventListener('click', () => this._onCityPillClick(cityId)); - this._locationPills.appendChild(pill); - } - - // QWERTY keyboard navigation for POIs - this._poiKeydownHandler = (e) => { - if (!this._expandedCityId) return; - // Bail while a form control is focused so POI hotkeys don't fire from a - //