From d9a170fb3718960ebe6b5bfe91ebb8aeec3d3091 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Mon, 27 Apr 2026 15:55:33 +0100 Subject: [PATCH 01/10] Add markers and features to list basics --- demo/js/index.js | 2 +- plugins/interact/src/InteractInit.jsx | 3 + plugins/interact/src/hooks/useMapItemList.js | 125 +++++++ .../interact/src/hooks/useMapItemList.test.js | 308 ++++++++++++++++++ providers/beta/esri/src/esriProvider.js | 4 + providers/maplibre/src/maplibreProvider.js | 8 + .../components/Viewport/Features.module.scss | 4 + src/App/components/Viewport/Viewport.jsx | 2 +- src/App/hooks/useFeatureFocus.js | 28 +- src/App/hooks/useFeatureFocus.test.js | 88 +++++ src/App/hooks/useFeatureItems.js | 4 +- src/App/hooks/useFeatureItems.test.js | 16 +- src/config/events.js | 4 + src/types.js | 4 + 14 files changed, 583 insertions(+), 17 deletions(-) create mode 100644 plugins/interact/src/hooks/useMapItemList.js create mode 100644 plugins/interact/src/hooks/useMapItemList.test.js diff --git a/demo/js/index.js b/demo/js/index.js index afe064b4..805c7121 100755 --- a/demo/js/index.js +++ b/demo/js/index.js @@ -61,7 +61,7 @@ const interactPlugin = createInteractPlugin({ idProperty: 'id' }], debug: true, - interactionModes: ['selectMarker', 'placeMarker', 'selectFeature'], // e.g. ['selectMarker'], ['selectFeature'], ['placeMarker'], or combinations + interactionModes: ['selectMarker'], // e.g. ['selectMarker'], ['selectFeature'], ['placeMarker'], or combinations // multiSelect: true, contiguous: true, deselectOnClickOutside: true diff --git a/plugins/interact/src/InteractInit.jsx b/plugins/interact/src/InteractInit.jsx index 01eebd36..304d5296 100755 --- a/plugins/interact/src/InteractInit.jsx +++ b/plugins/interact/src/InteractInit.jsx @@ -1,6 +1,7 @@ import { useEffect, useRef } from 'react' import { EVENTS } from '../../../src/config/events.js' import { useInteractionHandlers } from './hooks/useInteractionHandlers.js' +import { useMapItemList } from './hooks/useMapItemList.js' import { useHighlightSync } from './hooks/useHighlightSync.js' import { useHoverCursor } from './hooks/useHoverCursor.js' import { attachEvents } from './events.js' @@ -22,6 +23,8 @@ export const InteractInit = ({ const isTouchOrKeyboard = ['touch', 'keyboard'].includes(interfaceType) const selectMarkerOnly = isSelectMarkerOnly(interactionModes) + useMapItemList({ mapState, pluginState, services, mapProvider }) + // Core interaction logic (click > select/marker) const { handleInteraction } = useInteractionHandlers({ appState, diff --git a/plugins/interact/src/hooks/useMapItemList.js b/plugins/interact/src/hooks/useMapItemList.js new file mode 100644 index 00000000..ac4e05e2 --- /dev/null +++ b/plugins/interact/src/hooks/useMapItemList.js @@ -0,0 +1,125 @@ +import { useEffect } from 'react' +import { EVENTS } from '../../../../src/config/events.js' +import { isStandaloneLabel } from '../../../../src/utils/symbolUtils.js' +import { buildLayerConfigMap } from '../utils/featureQueries.js' + +const isInViewport = (el) => { + const container = el.closest('.im-c-viewport__markers') + if (!container) { + return false + } + const containerRect = container.getBoundingClientRect() + const { left, right, top, bottom } = el.getBoundingClientRect() + return right > containerRect.left && left < containerRect.right && + bottom > containerRect.top && top < containerRect.bottom +} + +const collectVisibleMarkers = (markers) => { + const items = [] + for (const marker of markers.items) { + if (isStandaloneLabel(marker)) { + continue + } + const el = markers.markerRefs?.get(marker.id) + if (!el || !isInViewport(el)) { + continue + } + items.push({ id: marker.id, label: marker.label || marker.id }) + } + return items +} + +const collectVisibleFeatures = (mapProvider, layers) => { + const items = [] + const layerIds = layers.map(layer => layer.layerId) + const layerConfigMap = buildLayerConfigMap(layers) + const features = mapProvider.getVisibleFeatures(layerIds) + for (const feature of features) { + const config = layerConfigMap[feature.layer?.id] + if (!config) { + continue + } + const id = feature.properties?.[config.idProperty] ?? feature.id + if (id == null) { + continue + } + const label = config.labelProperty + ? (feature.properties?.[config.labelProperty] ?? String(id)) + : String(id) + items.push({ id: String(id), label }) + } + return items +} + +const findFeatureById = (features, layerConfigMap, targetId) => { + for (const feature of features) { + const config = layerConfigMap[feature.layer?.id] + if (!config) { + continue + } + const id = feature.properties?.[config.idProperty] ?? feature.id + if (id != null && String(id) === String(targetId)) { + return { feature, config } + } + } + return null +} + +export function useMapItemList ({ mapState, pluginState, services, mapProvider }) { + const { markers } = mapState + const { dispatch, interactionModes, layers } = pluginState + const { eventBus } = services + + useEffect(() => { + const handleMoveEnd = () => { + const items = [] + if (interactionModes.includes('selectMarker')) { + items.push(...collectVisibleMarkers(markers)) + } + if (interactionModes.includes('selectFeature') && layers.length > 0) { + items.push(...collectVisibleFeatures(mapProvider, layers)) + } + eventBus.emit(EVENTS.MAP_SET_FEATURES, { items }) + } + + eventBus.on(EVENTS.MAP_MOVE_END, handleMoveEnd) + return () => { eventBus.off(EVENTS.MAP_MOVE_END, handleMoveEnd) } + }, [markers, interactionModes, layers, mapProvider, eventBus]) + + useEffect(() => { + const handle = ({ id }) => { + if (id === null) { + dispatch({ type: 'CLEAR_SELECTED_FEATURES' }) + return + } + const markerMatch = markers.items.find(m => m.id === id) + if (markerMatch) { + dispatch({ type: 'SELECT_MARKER', payload: { markerId: id, multiSelect: false } }) + return + } + if (interactionModes.includes('selectFeature') && layers.length > 0) { + const layerIds = layers.map(layer => layer.layerId) + const layerConfigMap = buildLayerConfigMap(layers) + const features = mapProvider.getVisibleFeatures(layerIds) + const match = findFeatureById(features, layerConfigMap, id) + if (match) { + dispatch({ + type: 'TOGGLE_SELECTED_FEATURES', + payload: { + featureId: id, + multiSelect: false, + replaceAll: true, + layerId: match.config.layerId, + idProperty: match.config.idProperty, + properties: match.feature.properties, + geometry: match.feature.geometry + } + }) + } + } + } + + eventBus.on(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) + return () => { eventBus.off(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) } + }, [markers, interactionModes, layers, mapProvider, eventBus, dispatch]) +} diff --git a/plugins/interact/src/hooks/useMapItemList.test.js b/plugins/interact/src/hooks/useMapItemList.test.js new file mode 100644 index 00000000..04c7f44a --- /dev/null +++ b/plugins/interact/src/hooks/useMapItemList.test.js @@ -0,0 +1,308 @@ +import { renderHook, act } from '@testing-library/react' +import { useMapItemList } from './useMapItemList.js' + +const MOVE_END = 'map:moveend' +const SET_FEATURES = 'map:setfeatures' +const SET_ACTIVE = 'map:setactivefeature' + +const makeEventBus = () => { + const listeners = {} + return { + on: jest.fn((e, fn) => { listeners[e] = fn }), + off: jest.fn(), + emit: jest.fn((e, payload) => listeners[e]?.(payload)) + } +} + +const makeMarkerEl = ({ inViewport = true } = {}) => { + const el = document.createElement('div') + const container = document.createElement('div') + container.className = 'im-c-viewport__markers' + container.appendChild(el) + document.body.appendChild(container) + + const containerRect = { left: 0, top: 0, right: 500, bottom: 500 } + const markerRect = inViewport + ? { left: 100, top: 100, right: 150, bottom: 150 } + : { left: 600, top: 600, right: 650, bottom: 650 } + + jest.spyOn(container, 'getBoundingClientRect').mockReturnValue(containerRect) + jest.spyOn(el, 'getBoundingClientRect').mockReturnValue(markerRect) + + return { el, container } +} + +const makeMarkers = (overrides = []) => { + const markerRefs = new Map() + return { items: overrides, markerRefs } +} + +const makeMapProvider = (features = []) => ({ + getVisibleFeatures: jest.fn(() => features) +}) + +const setup = ({ interactionModes = [], markers, layers = [], mapProvider, eventBus, dispatch } = {}) => { + const eb = eventBus ?? makeEventBus() + const mp = mapProvider ?? makeMapProvider() + const dp = dispatch ?? jest.fn() + const { result, unmount } = renderHook(() => useMapItemList({ + mapState: { markers: markers ?? makeMarkers() }, + pluginState: { interactionModes, layers, dispatch: dp }, + services: { eventBus: eb }, + mapProvider: mp + })) + return { eb, mp, dp, result, unmount } +} + +// ─── useMapItemList — lifecycle ────────────────────────────────────────── + +describe('useMapItemList — lifecycle', () => { + it('subscribes to map:moveend on mount', () => { + const { eb } = setup() + expect(eb.on).toHaveBeenCalledWith(MOVE_END, expect.any(Function)) + }) + + it('unsubscribes from map:moveend on unmount', () => { + const { eb, unmount } = setup() + unmount() + expect(eb.off).toHaveBeenCalledWith(MOVE_END, expect.any(Function)) + }) +}) + +// ─── useMapItemList — selectMarker mode ─────────────────────────────────── + +describe('useMapItemList — selectMarker mode', () => { + afterEach(() => { document.body.innerHTML = '' }) + + it('emits visible markers as items on moveend', () => { + const { el, container } = makeMarkerEl({ inViewport: true }) + const markers = makeMarkers([{ id: 'm1', label: 'Marker One', symbol: 'pin', isVisible: true }]) + markers.markerRefs.set('m1', el) + + const { eb } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: 'm1', label: 'Marker One' }] + }) + container.remove() + }) + + it('excludes markers outside the viewport', () => { + const { el, container } = makeMarkerEl({ inViewport: false }) + const markers = makeMarkers([{ id: 'm1', label: 'Offscreen', symbol: 'pin', isVisible: true }]) + markers.markerRefs.set('m1', el) + + const { eb } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + container.remove() + }) + + it('excludes standalone label markers', () => { + const { el, container } = makeMarkerEl({ inViewport: true }) + const markers = makeMarkers([{ id: 'm1', label: 'Label', symbol: null, isVisible: true }]) + markers.markerRefs.set('m1', el) + + const { eb } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + container.remove() + }) + + it('falls back to marker id when label is absent', () => { + const { el, container } = makeMarkerEl({ inViewport: true }) + const markers = makeMarkers([{ id: 'm1', symbol: 'pin', isVisible: true }]) + markers.markerRefs.set('m1', el) + + const { eb } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: 'm1', label: 'm1' }] + }) + container.remove() + }) +}) + +// ─── useMapItemList — selectFeature mode ────────────────────────────────── + +describe('useMapItemList — selectFeature mode', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] + + it('emits layer features as items on moveend', () => { + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High Street' } } + ] + const { eb, mp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + + act(() => eb.emit(MOVE_END, {})) + + expect(mp.getVisibleFeatures).toHaveBeenCalledWith(['roads']) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: '1', label: 'High Street' }] + }) + }) + + it('falls back to idProperty value when labelProperty is absent on feature', () => { + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '2' } } + ] + const { eb } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: '2', label: '2' }] + }) + }) + + it('uses idProperty as label when layer has no labelProperty configured', () => { + const layersNoLabel = [{ layerId: 'roads', idProperty: 'road_id' }] + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '3' } } + ] + const { eb } = setup({ + interactionModes: ['selectFeature'], + layers: layersNoLabel, + mapProvider: makeMapProvider(features) + }) + + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: '3', label: '3' }] + }) + }) + + it('skips features with no matching layer config', () => { + const features = [ + { layer: { id: 'unknown-layer' }, properties: { road_id: '4' } } + ] + const { eb } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + }) + + it('emits empty items when layers array is empty', () => { + const { eb } = setup({ interactionModes: ['selectFeature'], layers: [] }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + }) +}) + +// ─── useMapItemList — combined modes ───────────────────────────────────── + +describe('useMapItemList — combined modes', () => { + afterEach(() => { document.body.innerHTML = '' }) + + it('emits both markers and features when both modes are active', () => { + const { el, container } = makeMarkerEl({ inViewport: true }) + const markers = makeMarkers([{ id: 'm1', label: 'A Marker', symbol: 'pin', isVisible: true }]) + markers.markerRefs.set('m1', el) + + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] + const features = [{ layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'Main Rd' } }] + + const { eb } = setup({ + interactionModes: ['selectMarker', 'selectFeature'], + markers, + layers, + mapProvider: makeMapProvider(features) + }) + + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: 'm1', label: 'A Marker' }, { id: '1', label: 'Main Rd' }] + }) + container.remove() + }) + + it('emits empty items when no interaction modes are active', () => { + const { eb } = setup({ interactionModes: [] }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + }) +}) + +// ─── useMapItemList — map:setactivefeature listener ────────────────────── + +describe('useMapItemList — map:setactivefeature listener', () => { + afterEach(() => { document.body.innerHTML = '' }) + + it('subscribes to map:setactivefeature on mount and unsubscribes on unmount', () => { + const { eb, unmount } = setup() + expect(eb.on).toHaveBeenCalledWith(SET_ACTIVE, expect.any(Function)) // NOSONAR + unmount() + expect(eb.off).toHaveBeenCalledWith(SET_ACTIVE, expect.any(Function)) // NOSONAR + }) + + it('dispatches CLEAR_SELECTED_FEATURES when id is null', () => { + const { eb, dp } = setup({ interactionModes: ['selectMarker'] }) + act(() => eb.emit(SET_ACTIVE, { id: null })) // NOSONAR + expect(dp).toHaveBeenCalledWith({ type: 'CLEAR_SELECTED_FEATURES' }) + }) + + it('dispatches SELECT_MARKER when id matches a marker', () => { + const markers = makeMarkers([{ id: 'm1', label: 'Marker', symbol: 'pin', isVisible: true }]) + const { eb, dp } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(SET_ACTIVE, { id: 'm1' })) // NOSONAR + expect(dp).toHaveBeenCalledWith({ + type: 'SELECT_MARKER', + payload: { markerId: 'm1', multiSelect: false } + }) + }) + + it('dispatches TOGGLE_SELECTED_FEATURES when id matches a feature', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High St' }, geometry: { type: 'Point' } } + ] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + act(() => eb.emit(SET_ACTIVE, { id: '1' })) // NOSONAR + expect(dp).toHaveBeenCalledWith({ + type: 'TOGGLE_SELECTED_FEATURES', + payload: { + featureId: '1', + multiSelect: false, + replaceAll: true, + layerId: 'roads', + idProperty: 'road_id', + properties: { road_id: '1', road_name: 'High St' }, + geometry: { type: 'Point' } + } + }) + }) + + it('does not dispatch when feature id is not found in visible features', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id' }] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider([]) + }) + act(() => eb.emit(SET_ACTIVE, { id: 'missing' })) // NOSONAR + expect(dp).not.toHaveBeenCalled() + }) +}) diff --git a/providers/beta/esri/src/esriProvider.js b/providers/beta/esri/src/esriProvider.js index f540726a..ddf2fc71 100644 --- a/providers/beta/esri/src/esriProvider.js +++ b/providers/beta/esri/src/esriProvider.js @@ -182,6 +182,10 @@ export default class EsriProvider { return queryVectorTileFeatures(this.view, point) } + getVisibleFeatures (_layerIds) { + return [] + } + // ========================== // Spatial helpers // ========================== diff --git a/providers/maplibre/src/maplibreProvider.js b/providers/maplibre/src/maplibreProvider.js index 0792bd06..aedabb10 100755 --- a/providers/maplibre/src/maplibreProvider.js +++ b/providers/maplibre/src/maplibreProvider.js @@ -302,6 +302,14 @@ export default class MapLibreProvider { return queryFeatures(this.map, point, options) } + getVisibleFeatures (layerIds) { + const presentLayers = layerIds.filter(id => this.map.getLayer(id)) + if (!presentLayers.length) { + return [] + } + return this.map.queryRenderedFeatures(undefined, { layers: presentLayers }) + } + /** * Rasterise and register symbol images for the given pre-resolved symbol configs. * Delegates to the shared symbol image utility so any plugin's MapLibre adapter can diff --git a/src/App/components/Viewport/Features.module.scss b/src/App/components/Viewport/Features.module.scss index 4bb1306a..ff2bfe35 100644 --- a/src/App/components/Viewport/Features.module.scss +++ b/src/App/components/Viewport/Features.module.scss @@ -15,4 +15,8 @@ @include tools.border-focus-base( $is-inset: true ); + + &[data-focus-visible="true"][aria-activedescendant]::after { + opacity: 0; + } } diff --git a/src/App/components/Viewport/Viewport.jsx b/src/App/components/Viewport/Viewport.jsx index ec592db8..3ed20823 100755 --- a/src/App/components/Viewport/Viewport.jsx +++ b/src/App/components/Viewport/Viewport.jsx @@ -33,7 +33,7 @@ export const Viewport = () => { const [keyboardHintVisible, setKeyboardHintVisible] = useState(false) const featureItems = useFeatureItems(eventBus) - const { activeFeatureId, onFocus: onFeaturesFocus } = useFeatureFocus({ viewportRef: layoutRefs.viewportRef, featuresRef, items: featureItems }) + const { activeFeatureId, onFocus: onFeaturesFocus } = useFeatureFocus({ viewportRef: layoutRefs.viewportRef, featuresRef, items: featureItems, eventBus }) // Attach map keyboard controls useKeyboardShortcuts(layoutRefs.viewportRef) diff --git a/src/App/hooks/useFeatureFocus.js b/src/App/hooks/useFeatureFocus.js index dd040be5..396c584c 100644 --- a/src/App/hooks/useFeatureFocus.js +++ b/src/App/hooks/useFeatureFocus.js @@ -1,4 +1,5 @@ -import { useState, useEffect } from 'react' +import { useState, useEffect, useRef } from 'react' +import { EVENTS } from '../../config/events.js' const getNavigatedId = (id, key, items) => { if (!items.length) { @@ -11,8 +12,20 @@ const getNavigatedId = (id, key, items) => { return idx === -1 ? items[items.length - 1].id : items[Math.max(idx - 1, 0)].id } -export function useFeatureFocus ({ viewportRef, featuresRef, items = [] }) { +export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBus }) { const [activeFeatureId, setActiveFeatureId] = useState(null) + const activeFeatureIdRef = useRef(null) + + useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) + + useEffect(() => { + if (!eventBus) { + return undefined + } + const handle = ({ id }) => { setActiveFeatureId(id) } + eventBus.on(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) + return () => { eventBus.off(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) } + }, [eventBus]) useEffect(() => { const listboxEl = featuresRef.current @@ -24,10 +37,13 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [] }) { if (event.key === 'Escape') { event.preventDefault() setActiveFeatureId(null) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: null }) viewportRef.current?.focus() } else if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { event.preventDefault() - setActiveFeatureId(id => getNavigatedId(id, event.key, items)) + const newId = getNavigatedId(activeFeatureIdRef.current, event.key, items) + setActiveFeatureId(newId) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: newId }) } else { // No action } @@ -35,11 +51,13 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [] }) { listboxEl.addEventListener('keydown', handleKeyDown) return () => { listboxEl.removeEventListener('keydown', handleKeyDown) } - }, [viewportRef, featuresRef, items]) + }, [viewportRef, featuresRef, items, eventBus]) const onFocus = () => { if (items.length) { - setActiveFeatureId(items[0].id) + const firstId = items[0].id + setActiveFeatureId(firstId) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: firstId }) } } diff --git a/src/App/hooks/useFeatureFocus.test.js b/src/App/hooks/useFeatureFocus.test.js index 5dd9807b..24f0d319 100644 --- a/src/App/hooks/useFeatureFocus.test.js +++ b/src/App/hooks/useFeatureFocus.test.js @@ -7,6 +7,18 @@ const ITEMS = [ { id: 'c', label: 'Feature C' } ] +const SET_ACTIVE = 'map:setactivefeature' // NOSONAR + +const makeEventBus = () => { + const listeners = {} + return { + on: jest.fn((e, fn) => { listeners[e] = fn }), + off: jest.fn(), + emit: jest.fn(), + trigger: (e, payload) => listeners[e]?.(payload) + } +} + const makeRefs = ({ viewportFocus } = {}) => ({ viewportRef: { current: { focus: viewportFocus ?? jest.fn() } }, featuresRef: { current: document.createElement('ul') } @@ -198,3 +210,79 @@ describe('useFeatureFocus — ArrowUp navigation', () => { unmount(); el.remove() }) }) + +// ─── useFeatureFocus — eventBus integration ─────────────────────────────────── + +describe('useFeatureFocus — eventBus: map:setactivefeature listener', () => { + it('subscribes to map:setactivefeature on mount and unsubscribes on unmount', () => { + const eb = makeEventBus() + const { unmount } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + expect(eb.on).toHaveBeenCalledWith(SET_ACTIVE, expect.any(Function)) + unmount() + expect(eb.off).toHaveBeenCalledWith(SET_ACTIVE, expect.any(Function)) + }) + + it('updates activeFeatureId when map:setactivefeature is received', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + act(() => eb.trigger(SET_ACTIVE, { id: 'a' })) + expect(result.current.activeFeatureId).toBe('a') + }) + + it('clears activeFeatureId when map:setactivefeature is received with null', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS, eventBus: eb })) + act(() => eb.trigger(SET_ACTIVE, { id: 'a' })) + act(() => eb.trigger(SET_ACTIVE, { id: null })) + expect(result.current.activeFeatureId).toBeNull() + }) +}) + +describe('useFeatureFocus — eventBus: map:setactivefeature emit', () => { + const setupWithBus = () => { + const eb = makeEventBus() + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS, eventBus: eb })) + return { eb, el, result, unmount } + } + + afterEach(() => { document.body.innerHTML = '' }) + + it('emits map:setactivefeature with first item id on onFocus', () => { + const { eb, result } = setupWithBus() + act(() => result.current.onFocus()) + expect(eb.emit).toHaveBeenCalledWith(SET_ACTIVE, { id: 'a' }) + }) + + it('emits map:setactivefeature with new id on ArrowDown', () => { + const { eb, result, el, unmount } = setupWithBus() + act(() => result.current.onFocus()) + eb.emit.mockClear() + fireKey(el, 'ArrowDown') + expect(eb.emit).toHaveBeenCalledWith(SET_ACTIVE, { id: 'b' }) + unmount() + }) + + it('emits map:setactivefeature with null on Escape', () => { + const { eb, result, el, unmount } = setupWithBus() + act(() => result.current.onFocus()) + eb.emit.mockClear() + fireKey(el, 'Escape') + expect(eb.emit).toHaveBeenCalledWith(SET_ACTIVE, { id: null }) + unmount() + }) + + it('does not emit when eventBus is not provided', () => { + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) + expect(() => { + act(() => result.current.onFocus()) + fireKey(el, 'ArrowDown') + }).not.toThrow() + unmount() + }) +}) diff --git a/src/App/hooks/useFeatureItems.js b/src/App/hooks/useFeatureItems.js index 66cc85ff..8c475ff3 100644 --- a/src/App/hooks/useFeatureItems.js +++ b/src/App/hooks/useFeatureItems.js @@ -10,9 +10,9 @@ export function useFeatureItems (eventBus) { const handle = ({ items: next = [] }) => { setItems(next) } - eventBus.on('features:setItems', handle) + eventBus.on('map:setfeatures', handle) return () => { - eventBus.off('features:setItems', handle) + eventBus.off('map:setfeatures', handle) } }, [eventBus]) diff --git a/src/App/hooks/useFeatureItems.test.js b/src/App/hooks/useFeatureItems.test.js index ff35b836..18ab0d64 100644 --- a/src/App/hooks/useFeatureItems.test.js +++ b/src/App/hooks/useFeatureItems.test.js @@ -27,17 +27,17 @@ describe('useFeatureItems — initial state', () => { // ─── useFeatureItems — event subscription ──────────────────────────────────── describe('useFeatureItems — event subscription', () => { - it('subscribes to features:setItems on mount', () => { + it('subscribes to map:setfeatures on mount', () => { const eb = makeEventBus() renderHook(() => useFeatureItems(eb)) - expect(eb.on).toHaveBeenCalledWith('features:setItems', expect.any(Function)) + expect(eb.on).toHaveBeenCalledWith('map:setfeatures', expect.any(Function)) // NOSONAR }) it('unsubscribes on unmount', () => { const eb = makeEventBus() const { unmount } = renderHook(() => useFeatureItems(eb)) unmount() - expect(eb.off).toHaveBeenCalledWith('features:setItems', expect.any(Function)) + expect(eb.off).toHaveBeenCalledWith('map:setfeatures', expect.any(Function)) // NOSONAR }) it('does not subscribe when eventBus is undefined', () => { @@ -50,26 +50,26 @@ describe('useFeatureItems — event subscription', () => { // ─── useFeatureItems — updates ──────────────────────────────────────────────── describe('useFeatureItems — updates', () => { - it('updates items when features:setItems is emitted', () => { + it('updates items when map:setfeatures is emitted', () => { const eb = makeEventBus() const { result } = renderHook(() => useFeatureItems(eb)) const items = [{ id: 'a', label: 'Feature A' }, { id: 'b', label: 'Feature B' }] - act(() => eb.emit('features:setItems', { items })) + act(() => eb.emit('map:setfeatures', { items })) // NOSONAR expect(result.current).toEqual(items) }) it('clears items when emitted with an empty array', () => { const eb = makeEventBus() const { result } = renderHook(() => useFeatureItems(eb)) - act(() => eb.emit('features:setItems', { items: [{ id: 'a', label: 'A' }] })) - act(() => eb.emit('features:setItems', { items: [] })) + act(() => eb.emit('map:setfeatures', { items: [{ id: 'a', label: 'A' }] })) // NOSONAR + act(() => eb.emit('map:setfeatures', { items: [] })) // NOSONAR expect(result.current).toEqual([]) }) it('defaults to empty array when items key is missing from payload', () => { const eb = makeEventBus() const { result } = renderHook(() => useFeatureItems(eb)) - act(() => eb.emit('features:setItems', {})) + act(() => eb.emit('map:setfeatures', {})) // NOSONAR expect(result.current).toEqual([]) }) }) diff --git a/src/config/events.js b/src/config/events.js index 5691d35a..b971ecc1 100644 --- a/src/config/events.js +++ b/src/config/events.js @@ -123,6 +123,10 @@ export const EVENTS = { MAP_SET_STYLE: 'map:setstyle', /** @internal Set map size. Payload: { width, height } */ MAP_SET_SIZE: 'map:setsize', + /** @internal Set the accessible features list. Payload: { items: { id: string, label: string }[] } */ + MAP_SET_FEATURES: 'map:setfeatures', + /** @internal Set the active feature in the accessible features list. Payload: { id: string | null } */ + MAP_SET_ACTIVE_FEATURE: 'map:setactivefeature', /** @internal Set pixel ratio. Payload: pixelRatio */ MAP_SET_PIXEL_RATIO: 'map:setpixelratio', /** @internal Fit the map to a bounding box. Payload: [west, south, east, north] */ diff --git a/src/types.js b/src/types.js index de0e6683..e48ec442 100644 --- a/src/types.js +++ b/src/types.js @@ -265,6 +265,10 @@ * @property {(point: { x: number, y: number }) => any[]} getFeaturesAtPoint * Query rendered features at a screen pixel position (x from left edge, y from top edge of viewport). * + * @property {(layerIds: string[]) => any[]} getVisibleFeatures + * Return all GeoJSON features currently visible in the viewport on the given layers, + * respecting layer visibility and any active filters. Returns [] if unsupported. + * * @property {() => string} getAreaDimensions * Get the dimensions of the visible map area as a formatted string (e.g., '400m by 750m'). * From 9767d4aeb9b7689cab7923ff507389b223d3b197 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Wed, 29 Apr 2026 15:08:38 +0100 Subject: [PATCH 02/10] Keyboard active and focus states added --- demo/js/index.js | 32 +- docs/api.md | 2 - docs/api/map-style-config.md | 13 +- docs/api/marker-config.md | 14 +- docs/api/symbol-config.md | 36 +-- docs/api/symbol-registry.md | 38 ++- .../datasets/src/components/svgProperties.js | 4 +- .../beta/datasets/src/panels/Key.module.scss | 4 +- plugins/interact/src/InteractInit.test.js | 3 + .../interact/src/hooks/useHighlightSync.js | 18 +- .../src/hooks/useHighlightSync.test.js | 188 +++++++----- .../src/hooks/useInteractionHandlers.js | 10 +- plugins/interact/src/hooks/useMapItemList.js | 140 ++++++--- .../interact/src/hooks/useMapItemList.test.js | 286 ++++++++++++++---- plugins/interact/src/reducer.js | 8 +- plugins/interact/src/utils/buildStylesMap.js | 27 +- .../interact/src/utils/buildStylesMap.test.js | 51 ++-- providers/maplibre/src/mapEvents.js | 16 +- providers/maplibre/src/mapEvents.test.js | 17 +- providers/maplibre/src/maplibreProvider.js | 4 +- .../maplibre/src/utils/highlightFeatures.js | 89 +++--- .../src/utils/highlightFeatures.test.js | 162 +++++++--- providers/maplibre/src/utils/maplibreFixes.js | 10 + .../maplibre/src/utils/maplibreFixes.test.js | 31 ++ providers/maplibre/src/utils/symbolImages.js | 71 +++-- src/App/components/Markers/Markers.jsx | 25 +- src/App/components/Viewport/Features.jsx | 9 +- .../components/Viewport/Features.module.scss | 9 + src/App/components/Viewport/Features.test.jsx | 59 +++- src/App/components/Viewport/Viewport.jsx | 6 +- src/App/hooks/useFeatureFocus.js | 33 +- src/App/hooks/useFeatureFocus.test.js | 191 ++++++++++-- src/App/hooks/useFeatureItems.js | 6 +- src/App/hooks/useFeatureItems.test.js | 54 +++- src/config/events.js | 2 + src/config/mapTheme.js | 12 +- src/config/symbolConfig.js | 36 ++- src/services/symbolRegistry.js | 45 ++- src/services/symbolRegistry.test.js | 66 ++-- src/types.js | 22 +- src/utils/symbolUtils.test.js | 2 - 41 files changed, 1304 insertions(+), 547 deletions(-) diff --git a/demo/js/index.js b/demo/js/index.js index 805c7121..2019f621 100755 --- a/demo/js/index.js +++ b/demo/js/index.js @@ -26,11 +26,12 @@ import searchPlugin from '/plugins/search/src/index.js' import createInteractPlugin from '/plugins/interact/src/index.js' import createFramePlugin from '/plugins/beta/frame/src/index.js' -const pointData = {type: 'FeatureCollection',features: [{type: 'Feature',properties: {category:'prehistoric'},geometry: {coordinates: [-2.4558622,54.5617135],type: 'Point'}},{type: 'Feature',properties: {category:'roman'},geometry: {coordinates: [-2.439823,54.5525437],type: 'Point'}},{type: 'Feature',properties: {category:'medieval'},geometry: {coordinates: [-2.4481939,54.5575261],type: 'Point'}}]} +const pointData = {type: 'FeatureCollection',features: [{type: 'Feature',properties: { category:'prehistoric', name: 'Prehistoric feature' }, geometry: { coordinates: [-2.4558622,54.5617135], type: 'Point' }},{ type: 'Feature', properties: { category: 'roman', name: 'Roman feature' }, geometry: { coordinates: [-2.439823,54.5525437], type: 'Point' }},{ type: 'Feature', properties: { category:'medieval', name: 'Medieval feature' }, geometry: { coordinates: [-2.4481939,54.5575261], type: 'Point'} }]} const interactPlugin = createInteractPlugin({ layers: [{ layerId: 'historic-monuments-prehistoric-symbol', + labelProperty: 'name' // idProperty: 'gid' },{ layerId: 'land-covers-110', @@ -47,23 +48,28 @@ const interactPlugin = createInteractPlugin({ },{ layerId: 'land-covers-other', // idProperty: 'gid' - },{ - layerId: 'OS/TopographicArea_1/Agricultural Land', - idProperty: 'TOID' - },{ + }, + { layerId: 'hedge-control', + labelProperty: 'id', idProperty: 'id' - },{ + }, + // { + // layerId: 'OS/TopographicArea_1/Agricultural Land', + // idProperty: 'TOID' + // }, + { layerId: 'fill-inactive.cold', idProperty: 'id' },{ layerId: 'stroke-inactive.cold', idProperty: 'id' - }], + } + ], debug: true, - interactionModes: ['selectMarker'], // e.g. ['selectMarker'], ['selectFeature'], ['placeMarker'], or combinations - // multiSelect: true, - contiguous: true, + interactionModes: ['selectMarker', 'selectFeature'], // e.g. ['selectMarker'], ['selectFeature'], ['placeMarker'], or combinations + multiSelect: true, + // contiguous: true, deselectOnClickOutside: true }) @@ -305,10 +311,8 @@ interactiveMap.on('map:ready', function (e) { // aspectRatio: 1 // }) interactPlugin.enable() - interactiveMap.addMarker('my-marker-1', [-2.4555608,54.5655407], { showLabel: true, label: 'My label' }) - interactiveMap.addMarker('my-marker-2', [-2.4511636,54.5638338], { - symbol: 'square' - }) + interactiveMap.addMarker('my-marker-1', [-2.4555608,54.5655407], { label: 'My label', showLabel: true }) + interactiveMap.addMarker('my-marker-2', [-2.4511636,54.5638338], { label: 'Another marker', symbol: 'square' }) }) interactiveMap.on('datasets:ready', function () { diff --git a/docs/api.md b/docs/api.md index b6503bb5..d5f355c9 100644 --- a/docs/api.md +++ b/docs/api.md @@ -343,8 +343,6 @@ App-wide defaults for symbol and marker appearance. | `symbol` | `'pin'` | | `backgroundColor` | `'#ca3535'` | | `foregroundColor` | `'#ffffff'` | -| `haloWidth` | `'1'` | -| `selectedWidth` | `'6'` | ```js new InteractiveMap('map', { diff --git a/docs/api/map-style-config.md b/docs/api/map-style-config.md index 47aaa917..4fb9874c 100644 --- a/docs/api/map-style-config.md +++ b/docs/api/map-style-config.md @@ -38,7 +38,7 @@ Colour scheme for the app UI chrome — panels, buttons, and controls. Set to `' ### `mapColorScheme` **Type:** `'light' | 'dark'` -Colour scheme for elements rendered on top of the map. Sets the default values of `haloColor`, `selectedColor`, and `foregroundColor` when those are not explicitly provided. Set to `'dark'` when the basemap is dark (e.g. night or aerial) so that overlays remain legible against it. +Colour scheme for elements rendered on top of the map. Sets the default values of `haloColor`, `selectedColor`, `activeColor`, and `foregroundColor` when those are not explicitly provided. Set to `'dark'` when the basemap is dark (e.g. night or aerial) so that overlays remain legible against it. - `'light'` (default) — dark overlays (`#0b0c0c`) on a light basemap, white halo - `'dark'` — light overlays (`#ffffff`) on a dark or aerial basemap, dark halo @@ -92,12 +92,21 @@ Falls back to the `mapColorScheme` default when not set (`#ffffff` for light, `# ### `selectedColor` **Type:** `string` -Theme colour for selected state — used by map overlay components to indicate a selected feature. +Theme colour for committed selection — used by map overlay components to indicate a selected feature. Falls back to the `mapColorScheme` default when not set (`#0b0c0c` for light, `#ffffff` for dark). Injected as the `--map-overlay-selected-color` CSS custom property. --- +### `activeColor` +**Type:** `string` + +Theme colour for the active (keyboard cursor) focus ring — shown on the item currently under the keyboard cursor, whether or not it is committed to the selection. + +Falls back to `#ffdd00` (GOV.UK yellow) for both light and dark schemes when not set. + +--- + ### `foregroundColor` **Type:** `string` diff --git a/docs/api/marker-config.md b/docs/api/marker-config.md index b69d3fb2..db0e9af6 100644 --- a/docs/api/marker-config.md +++ b/docs/api/marker-config.md @@ -52,11 +52,11 @@ Inner SVG path content (no `` wrapper) to render instead of a registered sy // Using built-in tokens with per-style colours markers.add('id', coords, { symbolSvgContent: ` - - + + `, - viewBox: '0 0 38 38', + viewBox: '0 0 44 44', anchor: [0.5, 1], backgroundColor: { outdoor: '#d4351c', dark: '#ff6b6b' } }) @@ -72,16 +72,16 @@ markers.add('id', coords, { }) ``` -`{{selectedColor}}` and `{{selectedWidth}}` are required to render the selected ring around the marker when it is in its selected state. +`{{selectedColor}}` and `{{activeColor}}` are required to render the selection and focus rings. > [!NOTE] -> `selectedColor` cannot be set per marker — it is controlled by `MapStyleConfig.selectedColor`. +> `selectedColor` and `activeColor` cannot be set per marker — they are controlled by `MapStyleConfig`. --- ### `viewBox` **Type:** `string` -**Default:** registered symbol's viewBox, or `'0 0 38 38'` +**Default:** registered symbol's viewBox, or `'0 0 44 44'` SVG `viewBox` attribute for the symbol. Use alongside `symbolSvgContent` when your paths use a different coordinate space. @@ -133,6 +133,6 @@ Whether to render a visible label bubble above the marker symbol. When `false`, ### Colour and graphic properties -`backgroundColor`, `foregroundColor`, `haloWidth`, `graphic`, `selectedWidth`, and any custom tokens are all supported. +`backgroundColor`, `foregroundColor`, `graphic`, and any custom tokens are all supported. See [Symbol Config](./symbol-config.md) for the full property reference. diff --git a/docs/api/symbol-config.md b/docs/api/symbol-config.md index 03d4482e..2fe8b2c1 100644 --- a/docs/api/symbol-config.md +++ b/docs/api/symbol-config.md @@ -6,7 +6,7 @@ Symbol properties control the appearance of markers and point dataset features. Each property is optional. A value set directly on a marker or dataset layer takes priority over everything else. If a property is not set there, the value registered with the symbol is used. If the symbol has no value for that property, the app-wide `symbolDefaults` from the constructor applies. If none of those are set, the built-in fallback listed under each property below is used. -`haloColor`, `selectedColor`, `haloWidth`, and `selectedWidth` are required tokens in the SVG structure (see [SVG structure](#svg-structure)). Include them in any custom `symbolSvgContent` — the app resolves their values automatically. Note that `haloColor` and `selectedColor` are always derived from the active map style and cannot be configured. +`haloColor`, `selectedColor`, and `activeColor` are required tokens in the SVG structure (see [SVG structure](#svg-structure)). Include them in any custom `symbolSvgContent` — the app resolves their values automatically. All three are always derived from the active map style and cannot be configured. Ring and halo stroke widths are hardcoded — use the exported `RING_WIDTH` and `HALO_WIDTH` constants from `symbolConfig.js` to keep your custom SVG consistent with the built-in symbols. ## Style-keyed colours @@ -37,11 +37,11 @@ Inner SVG path content (no `` wrapper) to render as the symbol. Use `{{toke ```js { symbolSvgContent: ` - - + + `, - viewBox: '0 0 38 38', + viewBox: '0 0 44 44', anchor: [0.5, 1] } ``` @@ -52,7 +52,7 @@ See [SVG structure](#svg-structure) for the standard three-layer pattern. ### `viewBox` **Type:** `string` -**Default:** registered symbol's viewBox, or `'0 0 38 38'` +**Default:** registered symbol's viewBox, or `'0 0 44 44'` SVG `viewBox` attribute. Use alongside `symbolSvgContent` when your paths use a different coordinate space. @@ -87,22 +87,6 @@ Foreground fill colour — the inner graphic element (e.g. the dot inside a pin) --- -### `haloWidth` -**Type:** `number` -**Default:** `1` - -Stroke width of the halo around the symbol background shape. Can be set in constructor `symbolDefaults`, at symbol registration, or per marker/dataset layer. - ---- - -### `selectedWidth` -**Type:** `number` -**Default:** `6` - -Stroke width of the selection ring shown when a marker is selected. Can be set in constructor `symbolDefaults` or per marker/dataset layer. - ---- - ### `graphic` **Type:** `string` @@ -147,14 +131,14 @@ Symbols are defined as inner SVG path content (no `` wrapper) using `{{toke ```js svg: ` - - + + ` ``` -- **Layer 1** — selection ring (stroke only, fill none) — hidden in normal rendering, visible when selected -- **Layer 2** — background shape with halo stroke +- **Layer 1** — ring layer: `fill` is the committed-selection ring (`{{selectedColor}}`), `stroke` is the active/focus ring (`{{activeColor}}`). Both hidden in normal rendering. `paint-order="stroke fill"` ensures the stroke extends outward without clipping the fill. +- **Layer 2** — background shape with fixed halo stroke - **Layer 3** — foreground graphic (e.g. inner dot) -> `{{haloColor}}` and `{{selectedColor}}` are always injected from the active map style. They must be present in the SVG but cannot be configured. +> `{{haloColor}}`, `{{selectedColor}}`, and `{{activeColor}}` are always injected from the active map style. They must be present in the SVG but cannot be configured per symbol or marker. diff --git a/docs/api/symbol-registry.md b/docs/api/symbol-registry.md index 1ddc3306..d7cbed3b 100644 --- a/docs/api/symbol-registry.md +++ b/docs/api/symbol-registry.md @@ -33,7 +33,7 @@ Returns the merged app-wide defaults (hardcoded `symbolDefaults.js` + constructo ```js const defaults = services.symbolRegistry.getDefaults() -// { symbol: 'pin', backgroundColor: '#ca3535', selectedColor: { outdoor: '#0b0c0c', dark: '#ffffff' }, ... } +// { symbol: 'pin', backgroundColor: '#ca3535', foregroundColor: '#ffffff', ... } ``` --- @@ -46,19 +46,19 @@ Register a custom symbol. Once registered it can be referenced by ID via `Marker |----------|------|----------|-------------| | `id` | `string` | Yes | Unique symbol identifier | | `svg` | `string` | Yes | Inner SVG path content with `{{token}}` placeholders — see [SVG structure](./symbol-config.md#svg-structure) | -| `viewBox` | `string` | Yes | SVG viewBox, e.g. `'0 0 38 38'` | +| `viewBox` | `string` | Yes | SVG viewBox, e.g. `'0 0 44 44'` | | `anchor` | `[number, number]` | Yes | Normalised [x, y] anchor point | -| *(token)* | `string \| Record` | No | Default token value for this symbol, e.g. `backgroundColor: '#1d70b8'`. `selectedColor` and `selectedWidth` are ignored here — set them via constructor `symbolDefaults`. | +| *(token)* | `string \| Record` | No | Default token value for this symbol, e.g. `backgroundColor: '#1d70b8'`. `selectedColor` and `activeColor` are ignored here — they are always derived from the active map style. | ```js services.symbolRegistry.register({ id: 'star', - viewBox: '0 0 38 38', + viewBox: '0 0 44 44', anchor: [0.5, 0.5], backgroundColor: '#1d70b8', svg: ` - - + + ` }) @@ -88,28 +88,42 @@ const symbols = services.symbolRegistry.list() --- -### `resolve(symbolDef, styleColors, mapStyleId)` +### `resolve(symbolDef, styleColors, mapStyle)` -Resolves a symbol's SVG for **normal (unselected) rendering**. The `{{selectedColor}}` token is always replaced with an empty string — the selection ring is structurally present but invisible. +Resolves a symbol's SVG for **normal (unselected, inactive) rendering**. Both `{{selectedColor}}` and `{{activeColor}}` are replaced with `'none'`, so neither ring is visible. ```js const svg = services.symbolRegistry.resolve( services.symbolRegistry.get('pin'), { backgroundColor: '#d4351c' }, - 'outdoor' + mapStyle ) ``` --- -### `resolveSelected(symbolDef, styleColors, mapStyleId)` +### `resolveActive(symbolDef, styleColors, mapStyle)` -Resolves a symbol's SVG for **selected rendering**. The `{{selectedColor}}` token uses the cascade value. Use this when rendering the highlight layer for an interact or datasets selection. +Resolves a symbol's SVG for **active (keyboard cursor) rendering**. Both `{{selectedColor}}` and `{{activeColor}}` are live — the committed ring and the focus ring are both visible simultaneously, so an active item always shows both rings regardless of whether it is also selected. + +```js +const svg = services.symbolRegistry.resolveActive( + services.symbolRegistry.get('pin'), + { backgroundColor: '#d4351c' }, + mapStyle +) +``` + +--- + +### `resolveSelected(symbolDef, styleColors, mapStyle)` + +Resolves a symbol's SVG for **committed-selection rendering**. `{{selectedColor}}` is live (the black committed ring is visible) but `{{activeColor}}` is forced to `'none'` — use this when an item is selected but not the current keyboard cursor. ```js const svg = services.symbolRegistry.resolveSelected( services.symbolRegistry.get('pin'), { backgroundColor: '#d4351c' }, - 'outdoor' + mapStyle ) ``` diff --git a/plugins/beta/datasets/src/components/svgProperties.js b/plugins/beta/datasets/src/components/svgProperties.js index 73dbe1e6..54e4555b 100644 --- a/plugins/beta/datasets/src/components/svgProperties.js +++ b/plugins/beta/datasets/src/components/svgProperties.js @@ -1,6 +1,6 @@ -export const SVG_SIZE = 20 +export const SVG_SIZE = 20 // Width and height attributes of the svg element export const SVG_CENTER = SVG_SIZE / 2 -const SVG_SYMBOL_SIZE = 38 +const SVG_SYMBOL_SIZE = 44 // Width and height attributes of the svg element if its a marker or point feature symbol export const svgProps = { xmlns: 'http://www.w3.org/2000/svg', diff --git a/plugins/beta/datasets/src/panels/Key.module.scss b/plugins/beta/datasets/src/panels/Key.module.scss index 878be028..be34ce9e 100644 --- a/plugins/beta/datasets/src/panels/Key.module.scss +++ b/plugins/beta/datasets/src/panels/Key.module.scss @@ -61,9 +61,9 @@ .am-c-datasets-key-symbol { position: relative; flex-shrink: 0; - margin: 0 13px 0 2px; + margin: 0 13px 0 3px; } .am-c-datasets-key-symbol--point { - margin: -9px 3px -9px -7px; + margin: -12px 1px -12px -9px; } diff --git a/plugins/interact/src/InteractInit.test.js b/plugins/interact/src/InteractInit.test.js index 102cbdcf..24b3079b 100644 --- a/plugins/interact/src/InteractInit.test.js +++ b/plugins/interact/src/InteractInit.test.js @@ -4,11 +4,13 @@ import { InteractInit } from './InteractInit.jsx' import { useInteractionHandlers } from './hooks/useInteractionHandlers.js' import { useHighlightSync } from './hooks/useHighlightSync.js' import { useHoverCursor } from './hooks/useHoverCursor.js' +import { useMapItemList } from './hooks/useMapItemList.js' import { attachEvents } from './events.js' jest.mock('./hooks/useInteractionHandlers.js') jest.mock('./hooks/useHighlightSync.js') jest.mock('./hooks/useHoverCursor.js') +jest.mock('./hooks/useMapItemList.js') jest.mock('./events.js') describe('InteractInit', () => { @@ -23,6 +25,7 @@ describe('InteractInit', () => { useInteractionHandlers.mockReturnValue({ handleInteraction: handleInteractionMock }) useHighlightSync.mockReturnValue(undefined) useHoverCursor.mockReturnValue(undefined) + useMapItemList.mockReturnValue(undefined) attachEvents.mockReturnValue(cleanupMock) props = { diff --git a/plugins/interact/src/hooks/useHighlightSync.js b/plugins/interact/src/hooks/useHighlightSync.js index 800769e3..a86ce0a4 100755 --- a/plugins/interact/src/hooks/useHighlightSync.js +++ b/plugins/interact/src/hooks/useHighlightSync.js @@ -10,7 +10,7 @@ export const useHighlightSync = ({ events, eventBus }) => { - const { layers } = pluginState + const { layers, listboxActiveItem } = pluginState // Memoize stylesMap so it only recalculates when style or layers change const stylesMap = useMemo(() => { @@ -22,7 +22,10 @@ export const useHighlightSync = ({ // Force re-application of all selected features const updateHighlightedFeatures = () => { - const bounds = mapProvider.updateHighlightedFeatures?.(selectedFeatures, stylesMap) + const activeFeatures = listboxActiveItem + ? [{ featureId: listboxActiveItem.featureId, layerId: listboxActiveItem.layerId, idProperty: listboxActiveItem.idProperty, geometry: listboxActiveItem.geometry }] + : [] + const bounds = mapProvider.updateHighlightedFeatures?.(selectedFeatures, activeFeatures, stylesMap) dispatch({ type: 'UPDATE_SELECTED_BOUNDS', @@ -35,14 +38,15 @@ export const useHighlightSync = ({ return undefined // Explicit return to match the cleanup function return below } - // Update updateHighlightedFeatures on interaction updateHighlightedFeatures() - // Update updateHighlightedFeatures on stylechange - eventBus.on(events.MAP_DATA_CHANGE, updateHighlightedFeatures) + // Re-apply after style reload — highlight layers are removed when style reloads. + // MAP_DATA_CHANGE is NOT used here because addLayer/moveLayer fire styledata, + // which would create an infinite update loop via MAP_DATA_CHANGE. + eventBus.on(events.MAP_STYLE_CHANGE, updateHighlightedFeatures) return () => { - eventBus.off(events.MAP_DATA_CHANGE, updateHighlightedFeatures) + eventBus.off(events.MAP_STYLE_CHANGE, updateHighlightedFeatures) } - }, [selectedFeatures, mapProvider, stylesMap]) + }, [selectedFeatures, listboxActiveItem, mapProvider, stylesMap, eventBus]) } diff --git a/plugins/interact/src/hooks/useHighlightSync.test.js b/plugins/interact/src/hooks/useHighlightSync.test.js index 3189902d..90f2653e 100644 --- a/plugins/interact/src/hooks/useHighlightSync.test.js +++ b/plugins/interact/src/hooks/useHighlightSync.test.js @@ -6,43 +6,43 @@ jest.mock('../utils/buildStylesMap.js', () => ({ buildStylesMap: jest.fn(() => ({ layer1: { stroke: 'red', fill: 'blue' } })) })) -describe('useHighlightSync', () => { - let mockDeps - let capturedEventHandler - - const render = (overrides = {}) => - renderHook(() => useHighlightSync({ ...mockDeps, ...overrides })) - - beforeEach(() => { - jest.clearAllMocks() - capturedEventHandler = null - - mockDeps = { - mapProvider: { - updateHighlightedFeatures: jest.fn(() => ({ sw: [0, 0], ne: [1, 1] })) - }, - mapStyle: { id: 'default-style' }, - pluginState: { - layers: [{ layerId: 'layer1' }] - }, - selectedFeatures: [], - dispatch: jest.fn(), - events: { MAP_DATA_CHANGE: 'map:datachange' }, - eventBus: { - on: jest.fn((event, handler) => { - if (event === 'map:datachange') { - capturedEventHandler = handler - } - }), - off: jest.fn() - } +const STYLE_CHANGE = 'map:stylechange' + +let mockDeps +let capturedEventHandler + +const render = (overrides = {}) => + renderHook(() => useHighlightSync({ ...mockDeps, ...overrides })) + +beforeEach(() => { + jest.clearAllMocks() + capturedEventHandler = null + + mockDeps = { + mapProvider: { + updateHighlightedFeatures: jest.fn(() => ({ sw: [0, 0], ne: [1, 1] })) + }, + mapStyle: { id: 'default-style' }, + pluginState: { + layers: [{ layerId: 'layer1' }] + }, + selectedFeatures: [], + dispatch: jest.fn(), + events: { MAP_STYLE_CHANGE: STYLE_CHANGE }, + eventBus: { + on: jest.fn((event, handler) => { + if (event === STYLE_CHANGE) { + capturedEventHandler = handler + } + }), + off: jest.fn() } - }) + } +}) - /* ------------------------------------------------------------------ */ - /* Highlighting */ - /* ------------------------------------------------------------------ */ +// ─── useHighlightSync — highlighting ───────────────────────────────────────── +describe('useHighlightSync — highlighting', () => { it('updates map highlights and dispatches bounds', () => { mockDeps.selectedFeatures = [{ featureId: 'F1', layerId: 'layer1' }] @@ -50,9 +50,9 @@ describe('useHighlightSync', () => { expect(mockDeps.mapProvider.updateHighlightedFeatures).toHaveBeenCalledWith( mockDeps.selectedFeatures, + [], expect.any(Object) ) - expect(mockDeps.dispatch).toHaveBeenCalledWith({ type: 'UPDATE_SELECTED_BOUNDS', payload: { sw: [0, 0], ne: [1, 1] } @@ -70,11 +70,11 @@ describe('useHighlightSync', () => { payload: null }) }) +}) - /* ------------------------------------------------------------------ */ - /* Styles memoization */ - /* ------------------------------------------------------------------ */ +// ─── useHighlightSync — styles memoization ─────────────────────────────────── +describe('useHighlightSync — styles memoization', () => { it('rebuilds styles when mapStyle changes', () => { mockDeps.selectedFeatures = [{ featureId: 'F1' }] @@ -84,43 +84,33 @@ describe('useHighlightSync', () => { ) buildStylesMap.mockClear() - rerender({ mapStyle: { id: 'satellite' } }) - expect(buildStylesMap).toHaveBeenCalledWith( - expect.anything(), - { id: 'satellite' } - ) + expect(buildStylesMap).toHaveBeenCalledWith(expect.anything(), { id: 'satellite' }) }) it('rebuilds styles when layers change', () => { mockDeps.selectedFeatures = [{ featureId: 'F1' }] const { rerender } = renderHook( - ({ layers }) => - useHighlightSync({ - ...mockDeps, - pluginState: { layers } - }), + ({ layers }) => useHighlightSync({ ...mockDeps, pluginState: { layers } }), { initialProps: { layers: [{ layerId: 'layer1' }] } } ) buildStylesMap.mockClear() - rerender({ layers: [{ layerId: 'layer1' }, { layerId: 'layer2' }] }) expect(buildStylesMap).toHaveBeenCalled() }) +}) - /* ------------------------------------------------------------------ */ - /* Map data change events */ - /* ------------------------------------------------------------------ */ +// ─── useHighlightSync — style-change event ─────────────────────────────────── - it('refreshes highlights on MAP_DATA_CHANGE', () => { +describe('useHighlightSync — style-change event', () => { + it('refreshes highlights on MAP_STYLE_CHANGE', () => { mockDeps.selectedFeatures = [{ featureId: 'F1', layerId: 'layer1' }] render() - mockDeps.mapProvider.updateHighlightedFeatures.mockClear() act(() => capturedEventHandler()) @@ -132,19 +122,15 @@ describe('useHighlightSync', () => { mockDeps.selectedFeatures = [{ featureId: 'F1', layerId: 'layer1' }] const { unmount } = render() - unmount() - expect(mockDeps.eventBus.off).toHaveBeenCalledWith( - 'map:datachange', - expect.any(Function) - ) + expect(mockDeps.eventBus.off).toHaveBeenCalledWith(STYLE_CHANGE, expect.any(Function)) }) +}) - /* ------------------------------------------------------------------ */ - /* Guards */ - /* ------------------------------------------------------------------ */ +// ─── useHighlightSync — guards ─────────────────────────────────────────────── +describe('useHighlightSync — guards', () => { it('does nothing when mapProvider is null', () => { mockDeps.mapProvider = null mockDeps.selectedFeatures = [{ featureId: 'F1' }] @@ -171,41 +157,101 @@ describe('useHighlightSync', () => { expect(mockDeps.mapProvider.updateHighlightedFeatures).not.toHaveBeenCalled() expect(buildStylesMap).not.toHaveBeenCalled() }) +}) - /* ------------------------------------------------------------------ */ - /* Selection updates */ - /* ------------------------------------------------------------------ */ +// ─── useHighlightSync — selection updates ──────────────────────────────────── +describe('useHighlightSync — selection updates', () => { it('updates highlights when selection changes', () => { const { rerender } = renderHook( - ({ selectedFeatures }) => - useHighlightSync({ ...mockDeps, selectedFeatures }), + ({ selectedFeatures }) => useHighlightSync({ ...mockDeps, selectedFeatures }), { initialProps: { selectedFeatures: [{ featureId: 'F1' }] } } ) mockDeps.mapProvider.updateHighlightedFeatures.mockClear() - rerender({ selectedFeatures: [{ featureId: 'F1' }, { featureId: 'F2' }] }) expect(mockDeps.mapProvider.updateHighlightedFeatures).toHaveBeenCalledWith( [{ featureId: 'F1' }, { featureId: 'F2' }], + [], expect.anything() ) }) it('clears highlights when selection becomes empty', () => { const { rerender } = renderHook( - ({ selectedFeatures }) => - useHighlightSync({ ...mockDeps, selectedFeatures }), + ({ selectedFeatures }) => useHighlightSync({ ...mockDeps, selectedFeatures }), { initialProps: { selectedFeatures: [{ featureId: 'F1' }] } } ) mockDeps.mapProvider.updateHighlightedFeatures.mockClear() - rerender({ selectedFeatures: [] }) expect(mockDeps.mapProvider.updateHighlightedFeatures).toHaveBeenCalledWith( [], + [], + expect.anything() + ) + }) +}) + +// ─── useHighlightSync — listboxActiveItem ──────────────────────────────────── + +describe('useHighlightSync — listboxActiveItem', () => { + it('passes listboxActiveItem as activeFeatures (second arg)', () => { + mockDeps.pluginState = { + layers: [{ layerId: 'layer1' }], + listboxActiveItem: { featureId: 'A1', layerId: 'layer1', idProperty: 'id', geometry: { type: 'Point' } } + } + mockDeps.selectedFeatures = [] + + render() + + expect(mockDeps.mapProvider.updateHighlightedFeatures).toHaveBeenCalledWith( + [], + [{ featureId: 'A1', layerId: 'layer1', idProperty: 'id', geometry: { type: 'Point' } }], + expect.anything() + ) + }) + + it('passes selectedFeatures and listboxActiveItem as separate arguments', () => { + mockDeps.pluginState = { + layers: [{ layerId: 'layer1' }], + listboxActiveItem: { featureId: 'A1', layerId: 'layer1', idProperty: 'id', geometry: { type: 'Point' } } + } + mockDeps.selectedFeatures = [{ featureId: 'F1', layerId: 'layer1' }] + + render() + + expect(mockDeps.mapProvider.updateHighlightedFeatures).toHaveBeenCalledWith( + [{ featureId: 'F1', layerId: 'layer1' }], + [{ featureId: 'A1', layerId: 'layer1', idProperty: 'id', geometry: { type: 'Point' } }], + expect.anything() + ) + }) + + it('re-highlights when listboxActiveItem changes', () => { + const { rerender } = renderHook( + ({ pluginState }) => useHighlightSync({ ...mockDeps, pluginState }), + { + initialProps: { + pluginState: { layers: [{ layerId: 'layer1' }], listboxActiveItem: null } + } + } + ) + + mockDeps.mapProvider.updateHighlightedFeatures.mockClear() + + rerender({ + pluginState: { + layers: [{ layerId: 'layer1' }], + listboxActiveItem: { featureId: 'A1', layerId: 'layer1', idProperty: 'id', geometry: { type: 'Point' } } + } + }) + + expect(mockDeps.mapProvider.updateHighlightedFeatures).toHaveBeenCalledWith( + [], + [{ featureId: 'A1', layerId: 'layer1', idProperty: 'id', geometry: { type: 'Point' } }], expect.anything() ) }) diff --git a/plugins/interact/src/hooks/useInteractionHandlers.js b/plugins/interact/src/hooks/useInteractionHandlers.js index 9c8f6228..0910026d 100755 --- a/plugins/interact/src/hooks/useInteractionHandlers.js +++ b/plugins/interact/src/hooks/useInteractionHandlers.js @@ -122,6 +122,11 @@ export const useInteractionHandlers = ({ mapState, pluginState, services, mapPro }, [interactionModes, dispatch, markers, markerOptions, eventBus, deselectOnClickOutside]) const handleInteraction = useCallback(({ point, coords }) => { + const debugFeatures = pluginState?.debug ? getFeaturesAtPoint(mapProvider, point, { radius: tolerance }) : null + if (debugFeatures) { + console.log(`--- Features at ${coords} ---`, debugFeatures) // NOSONAR + } + if (interactionModes.includes('selectMarker')) { const markerHit = findMarkerAtPoint(markers, point, scale) if (markerHit) { @@ -131,10 +136,7 @@ export const useInteractionHandlers = ({ mapState, pluginState, services, mapPro } if (interactionModes.includes('selectFeature') && layers.length > 0) { - const allFeatures = getFeaturesAtPoint(mapProvider, point, { radius: tolerance }) - if (pluginState?.debug) { - console.log(`--- Features at ${coords} ---`, allFeatures) - } + const allFeatures = debugFeatures ?? getFeaturesAtPoint(mapProvider, point, { radius: tolerance }) const match = findMatchingFeature(allFeatures, layerConfigMap) if (match) { processFeatureMatch(match) diff --git a/plugins/interact/src/hooks/useMapItemList.js b/plugins/interact/src/hooks/useMapItemList.js index ac4e05e2..6cca1a33 100644 --- a/plugins/interact/src/hooks/useMapItemList.js +++ b/plugins/interact/src/hooks/useMapItemList.js @@ -1,8 +1,11 @@ -import { useEffect } from 'react' +import { useEffect, useRef } from 'react' import { EVENTS } from '../../../../src/config/events.js' import { isStandaloneLabel } from '../../../../src/utils/symbolUtils.js' import { buildLayerConfigMap } from '../utils/featureQueries.js' +const getFeatureId = (feature, config) => + config ? (feature.properties?.[config.idProperty] ?? feature.id) : null + const isInViewport = (el) => { const container = el.closest('.im-c-viewport__markers') if (!container) { @@ -17,36 +20,44 @@ const isInViewport = (el) => { const collectVisibleMarkers = (markers) => { const items = [] for (const marker of markers.items) { - if (isStandaloneLabel(marker)) { - continue - } + if (!marker.label) { continue } const el = markers.markerRefs?.get(marker.id) - if (!el || !isInViewport(el)) { - continue + if (!isStandaloneLabel(marker) && el && isInViewport(el)) { + items.push({ id: marker.id, label: marker.label }) } - items.push({ id: marker.id, label: marker.label || marker.id }) } return items } +const toFeatureItem = (feature, layerConfigMap, seenIds) => { + const config = layerConfigMap[feature.layer?.id] + if (!config?.labelProperty) { + return null + } + const id = getFeatureId(feature, config) + const stringId = id == null ? null : String(id) + if (stringId == null || seenIds.has(stringId)) { + return null + } + seenIds.add(stringId) + const layerLabel = config.label || config.layerId + const label = config.labelProperty + ? (feature.properties?.[config.labelProperty] ?? stringId) + : `${layerLabel} ${stringId}` + return { id: stringId, label } +} + const collectVisibleFeatures = (mapProvider, layers) => { const items = [] + const seenIds = new Set() const layerIds = layers.map(layer => layer.layerId) const layerConfigMap = buildLayerConfigMap(layers) const features = mapProvider.getVisibleFeatures(layerIds) for (const feature of features) { - const config = layerConfigMap[feature.layer?.id] - if (!config) { - continue - } - const id = feature.properties?.[config.idProperty] ?? feature.id - if (id == null) { - continue + const item = toFeatureItem(feature, layerConfigMap, seenIds) + if (item) { + items.push(item) } - const label = config.labelProperty - ? (feature.properties?.[config.labelProperty] ?? String(id)) - : String(id) - items.push({ id: String(id), label }) } return items } @@ -54,22 +65,15 @@ const collectVisibleFeatures = (mapProvider, layers) => { const findFeatureById = (features, layerConfigMap, targetId) => { for (const feature of features) { const config = layerConfigMap[feature.layer?.id] - if (!config) { - continue - } - const id = feature.properties?.[config.idProperty] ?? feature.id - if (id != null && String(id) === String(targetId)) { - return { feature, config } + const rawId = getFeatureId(feature, config) + if (rawId != null && String(rawId) === String(targetId)) { + return { feature, config, rawId } } } return null } -export function useMapItemList ({ mapState, pluginState, services, mapProvider }) { - const { markers } = mapState - const { dispatch, interactionModes, layers } = pluginState - const { eventBus } = services - +function useItemListSync ({ markers, interactionModes, layers, mapProvider, multiSelect, eventBus }) { useEffect(() => { const handleMoveEnd = () => { const items = [] @@ -79,22 +83,32 @@ export function useMapItemList ({ mapState, pluginState, services, mapProvider } if (interactionModes.includes('selectFeature') && layers.length > 0) { items.push(...collectVisibleFeatures(mapProvider, layers)) } - eventBus.emit(EVENTS.MAP_SET_FEATURES, { items }) + eventBus.emit(EVENTS.MAP_SET_FEATURES, { items, multiselectable: multiSelect }) } - eventBus.on(EVENTS.MAP_MOVE_END, handleMoveEnd) - return () => { eventBus.off(EVENTS.MAP_MOVE_END, handleMoveEnd) } - }, [markers, interactionModes, layers, mapProvider, eventBus]) + eventBus.on(EVENTS.MAP_DATA_CHANGE, handleMoveEnd) + return () => { + eventBus.off(EVENTS.MAP_MOVE_END, handleMoveEnd) + eventBus.off(EVENTS.MAP_DATA_CHANGE, handleMoveEnd) + } + }, [markers, interactionModes, layers, mapProvider, multiSelect, eventBus]) +} +// Shows the selection ring without firing interact:selectionchange. +// Marker rings are handled by Markers.jsx listening to map:setactivefeature directly. +// Feature rings are handled by useHighlightSync reading listboxActiveItem from plugin state. +function useActiveItemHandler ({ markers, interactionModes, layers, mapProvider, eventBus, dispatch, listboxActiveItemRef }) { useEffect(() => { const handle = ({ id }) => { if (id === null) { - dispatch({ type: 'CLEAR_SELECTED_FEATURES' }) + listboxActiveItemRef.current = null + dispatch({ type: 'SET_LISTBOX_ACTIVE', payload: null }) return } const markerMatch = markers.items.find(m => m.id === id) if (markerMatch) { - dispatch({ type: 'SELECT_MARKER', payload: { markerId: id, multiSelect: false } }) + listboxActiveItemRef.current = { id, isMarker: true } + dispatch({ type: 'SET_LISTBOX_ACTIVE', payload: null }) return } if (interactionModes.includes('selectFeature') && layers.length > 0) { @@ -103,23 +117,51 @@ export function useMapItemList ({ mapState, pluginState, services, mapProvider } const features = mapProvider.getVisibleFeatures(layerIds) const match = findFeatureById(features, layerConfigMap, id) if (match) { - dispatch({ - type: 'TOGGLE_SELECTED_FEATURES', - payload: { - featureId: id, - multiSelect: false, - replaceAll: true, - layerId: match.config.layerId, - idProperty: match.config.idProperty, - properties: match.feature.properties, - geometry: match.feature.geometry - } - }) + const payload = { + featureId: match.rawId, + layerId: match.config.layerId, + idProperty: match.config.idProperty, + geometry: match.feature.geometry + } + listboxActiveItemRef.current = { id, isMarker: false, ...payload, properties: match.feature.properties } + dispatch({ type: 'SET_LISTBOX_ACTIVE', payload }) } } } - eventBus.on(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) return () => { eventBus.off(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) } - }, [markers, interactionModes, layers, mapProvider, eventBus, dispatch]) + }, [markers, interactionModes, layers, mapProvider, eventBus, dispatch, listboxActiveItemRef]) +} + +// Promotes the listbox-active item to a real selection, firing interact:selectionchange. +function useSelectItemHandler ({ eventBus, dispatch, listboxActiveItemRef, multiSelect }) { + useEffect(() => { + const handleConfirm = () => { + const item = listboxActiveItemRef.current + if (!item) { + return + } + if (item.isMarker) { + dispatch({ type: 'TOGGLE_SELECTED_MARKERS', payload: { markerId: item.id, multiSelect } }) + } else { + const { featureId, layerId, idProperty, geometry, properties } = item + dispatch({ + type: 'TOGGLE_SELECTED_FEATURES', + payload: { featureId, layerId, idProperty, geometry, properties, multiSelect, replaceAll: !multiSelect } + }) + } + } + eventBus.on(EVENTS.MAP_SELECT_FEATURE, handleConfirm) + return () => { eventBus.off(EVENTS.MAP_SELECT_FEATURE, handleConfirm) } + }, [eventBus, dispatch, listboxActiveItemRef, multiSelect]) +} + +export function useMapItemList ({ mapState, pluginState, services, mapProvider }) { + const { markers } = mapState + const { dispatch, interactionModes, layers, multiSelect } = pluginState + const { eventBus } = services + const listboxActiveItemRef = useRef(null) + useItemListSync({ markers, interactionModes, layers, mapProvider, multiSelect, eventBus }) + useActiveItemHandler({ markers, interactionModes, layers, mapProvider, eventBus, dispatch, listboxActiveItemRef }) + useSelectItemHandler({ eventBus, dispatch, listboxActiveItemRef, multiSelect }) } diff --git a/plugins/interact/src/hooks/useMapItemList.test.js b/plugins/interact/src/hooks/useMapItemList.test.js index 04c7f44a..281faa37 100644 --- a/plugins/interact/src/hooks/useMapItemList.test.js +++ b/plugins/interact/src/hooks/useMapItemList.test.js @@ -1,9 +1,13 @@ import { renderHook, act } from '@testing-library/react' import { useMapItemList } from './useMapItemList.js' +const MARKER_LABEL = 'Marker One' + const MOVE_END = 'map:moveend' +const DATA_CHANGE = 'map:datachange' const SET_FEATURES = 'map:setfeatures' const SET_ACTIVE = 'map:setactivefeature' +const CONFIRM = 'map:selectfeature' const makeEventBus = () => { const listeners = {} @@ -41,13 +45,13 @@ const makeMapProvider = (features = []) => ({ getVisibleFeatures: jest.fn(() => features) }) -const setup = ({ interactionModes = [], markers, layers = [], mapProvider, eventBus, dispatch } = {}) => { +const setup = ({ interactionModes = [], markers, layers = [], mapProvider, eventBus, dispatch, multiSelect = false } = {}) => { const eb = eventBus ?? makeEventBus() const mp = mapProvider ?? makeMapProvider() const dp = dispatch ?? jest.fn() const { result, unmount } = renderHook(() => useMapItemList({ mapState: { markers: markers ?? makeMarkers() }, - pluginState: { interactionModes, layers, dispatch: dp }, + pluginState: { interactionModes, layers, dispatch: dp, multiSelect }, services: { eventBus: eb }, mapProvider: mp })) @@ -62,11 +66,42 @@ describe('useMapItemList — lifecycle', () => { expect(eb.on).toHaveBeenCalledWith(MOVE_END, expect.any(Function)) }) + it('subscribes to map:datachange on mount', () => { + const { eb } = setup() + expect(eb.on).toHaveBeenCalledWith(DATA_CHANGE, expect.any(Function)) + }) + it('unsubscribes from map:moveend on unmount', () => { const { eb, unmount } = setup() unmount() expect(eb.off).toHaveBeenCalledWith(MOVE_END, expect.any(Function)) }) + + it('unsubscribes from map:datachange on unmount', () => { + const { eb, unmount } = setup() + unmount() + expect(eb.off).toHaveBeenCalledWith(DATA_CHANGE, expect.any(Function)) + }) +}) + +// ─── useMapItemList — datachange trigger ───────────────────────────────── + +describe('useMapItemList — datachange trigger', () => { + afterEach(() => { document.body.innerHTML = '' }) + + it('re-emits items on map:datachange the same as map:moveend', () => { + const { el, container } = makeMarkerEl({ inViewport: true }) + const markers = makeMarkers([{ id: 'm1', label: MARKER_LABEL, symbol: 'pin', isVisible: true }]) + markers.markerRefs.set('m1', el) + + const { eb } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(DATA_CHANGE, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: 'm1', label: MARKER_LABEL }], multiselectable: false + }) + container.remove() + }) }) // ─── useMapItemList — selectMarker mode ─────────────────────────────────── @@ -76,14 +111,14 @@ describe('useMapItemList — selectMarker mode', () => { it('emits visible markers as items on moveend', () => { const { el, container } = makeMarkerEl({ inViewport: true }) - const markers = makeMarkers([{ id: 'm1', label: 'Marker One', symbol: 'pin', isVisible: true }]) + const markers = makeMarkers([{ id: 'm1', label: MARKER_LABEL, symbol: 'pin', isVisible: true }]) markers.markerRefs.set('m1', el) const { eb } = setup({ interactionModes: ['selectMarker'], markers }) act(() => eb.emit(MOVE_END, {})) expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [{ id: 'm1', label: 'Marker One' }] + items: [{ id: 'm1', label: MARKER_LABEL }], multiselectable: false }) container.remove() }) @@ -96,7 +131,7 @@ describe('useMapItemList — selectMarker mode', () => { const { eb } = setup({ interactionModes: ['selectMarker'], markers }) act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) container.remove() }) @@ -108,11 +143,11 @@ describe('useMapItemList — selectMarker mode', () => { const { eb } = setup({ interactionModes: ['selectMarker'], markers }) act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) container.remove() }) - it('falls back to marker id when label is absent', () => { + it('excludes markers without a label', () => { const { el, container } = makeMarkerEl({ inViewport: true }) const markers = makeMarkers([{ id: 'm1', symbol: 'pin', isVisible: true }]) markers.markerRefs.set('m1', el) @@ -120,16 +155,14 @@ describe('useMapItemList — selectMarker mode', () => { const { eb } = setup({ interactionModes: ['selectMarker'], markers }) act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [{ id: 'm1', label: 'm1' }] - }) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) container.remove() }) }) -// ─── useMapItemList — selectFeature mode ────────────────────────────────── +// ─── useMapItemList — selectFeature mode: label resolution ─────────────── -describe('useMapItemList — selectFeature mode', () => { +describe('useMapItemList — selectFeature mode: label resolution', () => { const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] it('emits layer features as items on moveend', () => { @@ -146,64 +179,76 @@ describe('useMapItemList — selectFeature mode', () => { expect(mp.getVisibleFeatures).toHaveBeenCalledWith(['roads']) expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [{ id: '1', label: 'High Street' }] + items: [{ id: '1', label: 'High Street' }], multiselectable: false }) }) it('falls back to idProperty value when labelProperty is absent on feature', () => { - const features = [ - { layer: { id: 'roads' }, properties: { road_id: '2' } } - ] - const { eb } = setup({ - interactionModes: ['selectFeature'], - layers, - mapProvider: makeMapProvider(features) - }) - + const features = [{ layer: { id: 'roads' }, properties: { road_id: '2' } }] + const { eb } = setup({ interactionModes: ['selectFeature'], layers, mapProvider: makeMapProvider(features) }) act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [{ id: '2', label: '2' }] + items: [{ id: '2', label: '2' }], multiselectable: false }) }) - it('uses idProperty as label when layer has no labelProperty configured', () => { - const layersNoLabel = [{ layerId: 'roads', idProperty: 'road_id' }] - const features = [ - { layer: { id: 'roads' }, properties: { road_id: '3' } } - ] + it('excludes features from layers with no labelProperty configured', () => { + const features = [{ layer: { id: 'roads' }, properties: { road_id: '3' } }] const { eb } = setup({ interactionModes: ['selectFeature'], - layers: layersNoLabel, + layers: [{ layerId: 'roads', idProperty: 'road_id' }], mapProvider: makeMapProvider(features) }) - act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [{ id: '3', label: '3' }] + items: [], multiselectable: false }) }) - it('skips features with no matching layer config', () => { - const features = [ - { layer: { id: 'unknown-layer' }, properties: { road_id: '4' } } - ] + it('excludes features from layers with a label but no labelProperty', () => { + const features = [{ layer: { id: 'roads' }, properties: { road_id: '4' } }] const { eb } = setup({ interactionModes: ['selectFeature'], - layers, + layers: [{ layerId: 'roads', label: 'Roads', idProperty: 'road_id' }], mapProvider: makeMapProvider(features) }) - act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [], multiselectable: false + }) + }) +}) + +// ─── useMapItemList — selectFeature mode: guards ───────────────────────── + +describe('useMapItemList — selectFeature mode: guards', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + it('skips features with no matching layer config', () => { + const features = [{ layer: { id: 'unknown-layer' }, properties: { road_id: '4' } }] + const { eb } = setup({ interactionModes: ['selectFeature'], layers, mapProvider: makeMapProvider(features) }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) }) it('emits empty items when layers array is empty', () => { const { eb } = setup({ interactionModes: ['selectFeature'], layers: [] }) act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) + }) + + it('includes multiselectable: true in payload when multiSelect is enabled', () => { + const features = [{ layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High St' } }] + const { eb } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features), + multiSelect: true + }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: '1', label: 'High St' }], multiselectable: true + }) }) }) @@ -230,7 +275,7 @@ describe('useMapItemList — combined modes', () => { act(() => eb.emit(MOVE_END, {})) expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [{ id: 'm1', label: 'A Marker' }, { id: '1', label: 'Main Rd' }] + items: [{ id: 'm1', label: 'A Marker' }, { id: '1', label: 'Main Rd' }], multiselectable: false }) container.remove() }) @@ -238,7 +283,7 @@ describe('useMapItemList — combined modes', () => { it('emits empty items when no interaction modes are active', () => { const { eb } = setup({ interactionModes: [] }) act(() => eb.emit(MOVE_END, {})) - expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [] }) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) }) }) @@ -254,23 +299,121 @@ describe('useMapItemList — map:setactivefeature listener', () => { expect(eb.off).toHaveBeenCalledWith(SET_ACTIVE, expect.any(Function)) // NOSONAR }) - it('dispatches CLEAR_SELECTED_FEATURES when id is null', () => { + it('dispatches SET_LISTBOX_ACTIVE null when id is null', () => { const { eb, dp } = setup({ interactionModes: ['selectMarker'] }) act(() => eb.emit(SET_ACTIVE, { id: null })) // NOSONAR - expect(dp).toHaveBeenCalledWith({ type: 'CLEAR_SELECTED_FEATURES' }) + expect(dp).toHaveBeenCalledWith({ type: 'SET_LISTBOX_ACTIVE', payload: null }) }) - it('dispatches SELECT_MARKER when id matches a marker', () => { + it('dispatches SET_LISTBOX_ACTIVE null when id matches a marker (ring handled by Markers.jsx)', () => { const markers = makeMarkers([{ id: 'm1', label: 'Marker', symbol: 'pin', isVisible: true }]) const { eb, dp } = setup({ interactionModes: ['selectMarker'], markers }) act(() => eb.emit(SET_ACTIVE, { id: 'm1' })) // NOSONAR + expect(dp).toHaveBeenCalledWith({ type: 'SET_LISTBOX_ACTIVE', payload: null }) + }) + + it('dispatches SET_LISTBOX_ACTIVE with feature payload when id matches a feature', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High St' }, geometry: { type: 'Point' } } + ] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + act(() => eb.emit(SET_ACTIVE, { id: '1' })) // NOSONAR expect(dp).toHaveBeenCalledWith({ - type: 'SELECT_MARKER', + type: 'SET_LISTBOX_ACTIVE', + payload: { + featureId: '1', + layerId: 'roads', + idProperty: 'road_id', + geometry: { type: 'Point' } + } + }) + }) + + it('preserves raw numeric featureId in SET_LISTBOX_ACTIVE payload (MapLibre filter type-strictness)', () => { + const layers = [{ layerId: 'hedges', idProperty: 'id' }] + const features = [ + { layer: { id: 'hedges' }, properties: { id: 27665979 }, geometry: { type: 'LineString' } } + ] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + act(() => eb.emit(SET_ACTIVE, { id: '27665979' })) + expect(dp).toHaveBeenCalledWith({ + type: 'SET_LISTBOX_ACTIVE', + payload: { + featureId: 27665979, + layerId: 'hedges', + idProperty: 'id', + geometry: { type: 'LineString' } + } + }) + }) + + it('does not dispatch when feature id is not found in visible features', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id' }] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider([]) + }) + act(() => eb.emit(SET_ACTIVE, { id: 'missing' })) // NOSONAR + expect(dp).not.toHaveBeenCalled() + }) +}) + +// ─── useMapItemList — confirm: lifecycle and guards ────────────────────── + +describe('useMapItemList — confirm: lifecycle and guards', () => { + it('subscribes to map:confirmfeature on mount and unsubscribes on unmount', () => { + const { eb, unmount } = setup() + expect(eb.on).toHaveBeenCalledWith(CONFIRM, expect.any(Function)) + unmount() + expect(eb.off).toHaveBeenCalledWith(CONFIRM, expect.any(Function)) + }) + + it('does nothing when no item is active', () => { + const { eb, dp } = setup() + act(() => eb.emit(CONFIRM)) + expect(dp).not.toHaveBeenCalled() + }) + + it('keeps the active item after confirm so repeated confirms dispatch again', () => { + const markers = makeMarkers([{ id: 'm1', label: 'Marker', symbol: 'pin', isVisible: true }]) + const { eb, dp } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(SET_ACTIVE, { id: 'm1' })) + act(() => eb.emit(CONFIRM)) + dp.mockClear() + act(() => eb.emit(CONFIRM)) + expect(dp).toHaveBeenCalledWith({ type: 'SELECT_MARKER', payload: { markerId: 'm1', multiSelect: false } }) + }) +}) + +// ─── useMapItemList — confirm: dispatches ──────────────────────────────── + +describe('useMapItemList — confirm: marker dispatches', () => { + afterEach(() => { document.body.innerHTML = '' }) + + it('dispatches TOGGLE_SELECTED_MARKERS after activating a marker', () => { + const markers = makeMarkers([{ id: 'm1', label: 'Marker', symbol: 'pin', isVisible: true }]) + const { eb, dp } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(SET_ACTIVE, { id: 'm1' })) + act(() => eb.emit(CONFIRM)) + expect(dp).toHaveBeenCalledWith({ + type: 'TOGGLE_SELECTED_MARKERS', payload: { markerId: 'm1', multiSelect: false } }) }) +}) - it('dispatches TOGGLE_SELECTED_FEATURES when id matches a feature', () => { +describe('useMapItemList — confirm: feature dispatches', () => { + it('dispatches TOGGLE_SELECTED_FEATURES after activating a feature', () => { const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] const features = [ { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High St' }, geometry: { type: 'Point' } } @@ -280,7 +423,8 @@ describe('useMapItemList — map:setactivefeature listener', () => { layers, mapProvider: makeMapProvider(features) }) - act(() => eb.emit(SET_ACTIVE, { id: '1' })) // NOSONAR + act(() => eb.emit(SET_ACTIVE, { id: '1' })) + act(() => eb.emit(CONFIRM)) expect(dp).toHaveBeenCalledWith({ type: 'TOGGLE_SELECTED_FEATURES', payload: { @@ -295,14 +439,48 @@ describe('useMapItemList — map:setactivefeature listener', () => { }) }) - it('does not dispatch when feature id is not found in visible features', () => { - const layers = [{ layerId: 'roads', idProperty: 'road_id' }] + it('dispatches with multiSelect: true and replaceAll: false when plugin multiSelect is enabled', () => { + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High St' }, geometry: { type: 'Point' } } + ] const { eb, dp } = setup({ interactionModes: ['selectFeature'], layers, - mapProvider: makeMapProvider([]) + mapProvider: makeMapProvider(features), + multiSelect: true + }) + act(() => eb.emit(SET_ACTIVE, { id: '1' })) + act(() => eb.emit(CONFIRM)) + expect(dp).toHaveBeenCalledWith({ + type: 'TOGGLE_SELECTED_FEATURES', + payload: expect.objectContaining({ multiSelect: true, replaceAll: false }) + }) + }) + + it('preserves raw numeric featureId in TOGGLE_SELECTED_FEATURES payload', () => { + const layers = [{ layerId: 'hedges', idProperty: 'id' }] + const features = [ + { layer: { id: 'hedges' }, properties: { id: 27665979 }, geometry: { type: 'LineString' } } + ] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + act(() => eb.emit(SET_ACTIVE, { id: '27665979' })) + act(() => eb.emit(CONFIRM)) + expect(dp).toHaveBeenCalledWith({ + type: 'TOGGLE_SELECTED_FEATURES', + payload: { + featureId: 27665979, + multiSelect: false, + replaceAll: true, + layerId: 'hedges', + idProperty: 'id', + properties: { id: 27665979 }, + geometry: { type: 'LineString' } + } }) - act(() => eb.emit(SET_ACTIVE, { id: 'missing' })) // NOSONAR - expect(dp).not.toHaveBeenCalled() }) }) diff --git a/plugins/interact/src/reducer.js b/plugins/interact/src/reducer.js index 6da54bdc..8ba81f53 100755 --- a/plugins/interact/src/reducer.js +++ b/plugins/interact/src/reducer.js @@ -9,7 +9,8 @@ const initialState = { selectedFeatures: [], selectedMarkers: [], selectionBounds: null, - closeOnAction: true // Done or Cancel + closeOnAction: true, // Done or Cancel + listboxActiveItem: null // { featureId, layerId, idProperty, geometry } | null — ring shown but no selectionchange } const enable = (state, payload) => { @@ -107,6 +108,8 @@ const clearSelectedFeatures = (state) => { } } +const setListboxActive = (state, payload) => ({ ...state, listboxActiveItem: payload ?? null }) + /** * Explicitly select a marker. Has no effect if already selected. * In single-select mode, clears selectedFeatures and replaces the selection. @@ -148,7 +151,8 @@ const actions = { UPDATE_SELECTED_BOUNDS: updateSelectedBounds, CLEAR_SELECTED_FEATURES: clearSelectedFeatures, SELECT_MARKER: selectMarker, - UNSELECT_MARKER: unselectMarker + UNSELECT_MARKER: unselectMarker, + SET_LISTBOX_ACTIVE: setListboxActive } export { diff --git a/plugins/interact/src/utils/buildStylesMap.js b/plugins/interact/src/utils/buildStylesMap.js index cf29e561..d5846d92 100755 --- a/plugins/interact/src/utils/buildStylesMap.js +++ b/plugins/interact/src/utils/buildStylesMap.js @@ -1,16 +1,21 @@ import { getValueForStyle } from '../../../../src/utils/getValueForStyle.js' - -const DEFAULT_STROKE_WIDTH = 3 +import { THEME_COLORS } from '../../../../src/config/mapTheme.js' +import { DEFAULTS } from '../defaults.js' /** * Builds a map of layerId → resolved highlight style for the given data layers. * - * Stroke colour resolution order: - * layer.selectedStroke → mapStyle.selectedColor → mapColorScheme scheme default + * Colour resolution order (both active and selection strokes): + * layer override → mapStyle override → mapColorScheme scheme default * * @param {Object[]} dataLayers * @param {Object} mapStyle - Current map style config - * @returns {Object} layerId → { stroke, fill, strokeWidth } + * @returns {Object} layerId → { + * stroke: string, — active (keyboard cursor) line colour + * selectionStroke: string, — committed-selection line colour + * fill: string, — committed-selection fill colour (transparent when not set) + * strokeWidth: number — line-width applied to all highlight line layers (both active and selected states) + * } */ export const buildStylesMap = (dataLayers, mapStyle) => { const stylesMap = {} @@ -19,15 +24,19 @@ export const buildStylesMap = (dataLayers, mapStyle) => { return stylesMap } - const schemeSelectedColor = mapStyle.mapColorScheme === 'dark' ? '#ffffff' : '#0b0c0c' + const scheme = THEME_COLORS[mapStyle.mapColorScheme] ?? THEME_COLORS.light + const schemeActiveColor = mapStyle.activeColor ?? scheme.activeColor + const schemeSelectedColor = mapStyle.selectedColor ?? scheme.selectedColor dataLayers.forEach(layer => { - const stroke = layer.selectedStroke || mapStyle.selectedColor || schemeSelectedColor + const activeStroke = layer.activeStroke || schemeActiveColor + const selectionStroke = layer.selectedStroke || schemeSelectedColor const fill = layer.selectedFill || 'transparent' - const strokeWidth = layer.selectedStrokeWidth || DEFAULT_STROKE_WIDTH + const strokeWidth = layer.selectedStrokeWidth || DEFAULTS.selectedStrokeWidth stylesMap[layer.layerId] = { - stroke: getValueForStyle(stroke, mapStyle.id), + stroke: getValueForStyle(activeStroke, mapStyle.id), + selectionStroke: getValueForStyle(selectionStroke, mapStyle.id), fill: getValueForStyle(fill, mapStyle.id), strokeWidth } diff --git a/plugins/interact/src/utils/buildStylesMap.test.js b/plugins/interact/src/utils/buildStylesMap.test.js index f028a969..01451fda 100644 --- a/plugins/interact/src/utils/buildStylesMap.test.js +++ b/plugins/interact/src/utils/buildStylesMap.test.js @@ -1,5 +1,6 @@ import { buildStylesMap } from './buildStylesMap.js' import { DEFAULTS } from '../defaults.js' +import { THEME_COLORS } from '../../../../src/config/mapTheme.js' import { getValueForStyle } from '../../../../src/utils/getValueForStyle.js' jest.mock('../../../../src/utils/getValueForStyle.js', () => ({ @@ -17,54 +18,50 @@ describe('buildStylesMap', () => { expect(buildStylesMap([], { id: 'default' })).toEqual({}) }) - it('builds correct stylesMap with defaults and layer overrides', () => { + it('builds correct stylesMap with layer overrides', () => { const dataLayers = [ - { layerId: 'custom1', selectedStroke: 'red', selectedFill: 'blue', selectedStrokeWidth: 5 }, - { layerId: 'custom2' } // uses defaults + { layerId: 'custom1', activeStroke: 'yellow', selectedStroke: 'red', selectedFill: 'blue', selectedStrokeWidth: 5 } ] - const mapStyle = { id: 'default-style' } - - const result = buildStylesMap(dataLayers, mapStyle) - - // Layer-specific values + const result = buildStylesMap(dataLayers, { id: 'default-style' }) expect(result.custom1).toEqual({ - stroke: 'red', + stroke: 'yellow', + selectionStroke: 'red', fill: 'blue', strokeWidth: 5 }) + }) - // Default fallback values - expect(result.custom2.stroke).toBe('#0b0c0c') - expect(result.custom2.fill).toBe('transparent') - expect(result.custom2.strokeWidth).toBe(DEFAULTS.selectedStrokeWidth) + it('falls back to scheme defaults when no layer or mapStyle overrides', () => { + const dataLayers = [{ layerId: 'layer1' }] + const result = buildStylesMap(dataLayers, { id: 'default-style' }) + expect(result.layer1.stroke).toBe(THEME_COLORS.light.activeColor) + expect(result.layer1.selectionStroke).toBe(THEME_COLORS.light.selectedColor) + expect(result.layer1.fill).toBe('transparent') + expect(result.layer1.strokeWidth).toBe(DEFAULTS.selectedStrokeWidth) }) - it('uses mapStyle.selectedColor as default stroke when no layer override', () => { + it('uses mapStyle.activeColor and selectedColor when provided', () => { const dataLayers = [{ layerId: 'layer1' }] - const mapStyle = { id: 'test', selectedColor: '#336699' } + const mapStyle = { id: 'test', activeColor: '#00ff00', selectedColor: '#336699' } const result = buildStylesMap(dataLayers, mapStyle) - expect(result.layer1.stroke).toBe('#336699') - expect(result.layer1.fill).toBe('transparent') + expect(result.layer1.stroke).toBe('#00ff00') + expect(result.layer1.selectionStroke).toBe('#336699') }) - it('uses scheme default when mapStyle has no selectedColor', () => { + it('uses dark scheme defaults when mapColorScheme is dark', () => { const dataLayers = [{ layerId: 'layer1' }] - expect(buildStylesMap(dataLayers, { id: 'light' }).layer1.stroke).toBe('#0b0c0c') - expect(buildStylesMap(dataLayers, { id: 'dark', mapColorScheme: 'dark' }).layer1.stroke).toBe('#ffffff') + const result = buildStylesMap(dataLayers, { id: 'dark', mapColorScheme: 'dark' }) + expect(result.layer1.stroke).toBe(THEME_COLORS.dark.activeColor) + expect(result.layer1.selectionStroke).toBe(THEME_COLORS.dark.selectedColor) }) it('calls getValueForStyle for stroke and fill with mapStyle.id', () => { const dataLayers = [ - { layerId: 'layer1', selectedStroke: 'strokeVal', selectedFill: 'fillVal' } + { layerId: 'layer1', activeStroke: 'strokeVal', selectedFill: 'fillVal' } ] - const mapStyle = { id: 'mapStyleId' } - - buildStylesMap(dataLayers, mapStyle) - + buildStylesMap(dataLayers, { id: 'mapStyleId' }) expect(getValueForStyle).toHaveBeenCalledWith('strokeVal', 'mapStyleId') expect(getValueForStyle).toHaveBeenCalledWith('fillVal', 'mapStyleId') - - // Ensure strokeWidth is not passed to getValueForStyle const numberCalls = getValueForStyle.mock.calls.filter(call => typeof call[0] === 'number') expect(numberCalls).toHaveLength(0) }) diff --git a/providers/maplibre/src/mapEvents.js b/providers/maplibre/src/mapEvents.js index b44bef1d..4af74786 100755 --- a/providers/maplibre/src/mapEvents.js +++ b/providers/maplibre/src/mapEvents.js @@ -51,7 +51,6 @@ export function attachMapEvents ({ map.on('moveend', onMoveEnd) handlers.push(['moveend', onMoveEnd]) - debouncers.push(onMoveEnd) // move (throttled) const onMove = throttle(() => { @@ -60,21 +59,27 @@ export function attachMapEvents ({ map.on('zoom', onMove) handlers.push(['zoom', onMove]) - debouncers.push(onMove) // render const onRender = () => emitEvent(events.MAP_RENDER) map.on('render', onRender) handlers.push(['render', onRender]) - // data change (debounced) + // data change (debounced) — styledata covers layer/visibility changes, + // sourcedata covers GeoJSON source mutations (e.g. draw plugin adding features) const onDataChange = debounce(() => { emitEvent(events.MAP_DATA_CHANGE, getMapState()) }, DEBOUNCE_IDLE_TIME) + const onSourceData = (e) => { + if (e.isSourceLoaded) { + onDataChange() + } + } + map.on('styledata', onDataChange) - handlers.push(['styledata', onDataChange]) - debouncers.push(onDataChange) + map.on('sourcedata', onSourceData) + handlers.push(['styledata', onDataChange], ['sourcedata', onSourceData]) // style change const onStyleChange = () => emitEvent(events.MAP_STYLE_CHANGE) @@ -89,6 +94,7 @@ export function attachMapEvents ({ map.on('click', onClick) handlers.push(['click', onClick]) + debouncers.push(onMoveEnd, onMove, onDataChange) // Cleanup return { diff --git a/providers/maplibre/src/mapEvents.test.js b/providers/maplibre/src/mapEvents.test.js index a07a0440..e705264a 100644 --- a/providers/maplibre/src/mapEvents.test.js +++ b/providers/maplibre/src/mapEvents.test.js @@ -49,7 +49,7 @@ describe('attachMapEvents', () => { }) test('registers listeners for all map events', () => { - ;['load', 'movestart', 'moveend', 'zoom', 'render', 'styledata', 'style.load', 'click'].forEach(e => + ;['load', 'movestart', 'moveend', 'zoom', 'render', 'styledata', 'sourcedata', 'style.load', 'click'].forEach(e => expect(map.on).toHaveBeenCalledWith(e, expect.any(Function)) ) expect(map.once).toHaveBeenCalledWith('idle', expect.any(Function)) @@ -99,6 +99,17 @@ describe('attachMapEvents', () => { ) }) + test('sourcedata emits MAP_DATA_CHANGE when source is loaded', () => { + handler('sourcedata')({ isSourceLoaded: true }) + expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_DATA_CHANGE, expect.any(Object)) + }) + + test('sourcedata does not emit MAP_DATA_CHANGE while source is still loading', () => { + eventBus.emit.mockClear() + handler('sourcedata')({ isSourceLoaded: false }) + expect(eventBus.emit).not.toHaveBeenCalled() + }) + test('click emits MAP_CLICK with point and coords', () => { handler('click')({ point: { x: 10, y: 20 }, lngLat: { lng: -1.5, lat: 52.3 } }) expect(eventBus.emit).toHaveBeenCalledWith(events.MAP_CLICK, { @@ -117,8 +128,8 @@ describe('attachMapEvents', () => { expect(moveEndFn.cancel).toHaveBeenCalled() expect(moveFn.cancel).toHaveBeenCalled() expect(dataChangeFn.cancel).toHaveBeenCalled() - expect(map.off).toHaveBeenCalledTimes(8) - ;['load', 'movestart', 'moveend', 'zoom', 'render', 'styledata', 'style.load', 'click'].forEach(e => + expect(map.off).toHaveBeenCalledTimes(9) + ;['load', 'movestart', 'moveend', 'zoom', 'render', 'styledata', 'sourcedata', 'style.load', 'click'].forEach(e => expect(map.off).toHaveBeenCalledWith(e, expect.any(Function)) ) }) diff --git a/providers/maplibre/src/maplibreProvider.js b/providers/maplibre/src/maplibreProvider.js index aedabb10..0052047d 100755 --- a/providers/maplibre/src/maplibreProvider.js +++ b/providers/maplibre/src/maplibreProvider.js @@ -223,9 +223,9 @@ export default class MapLibreProvider { * @param {any} stylesMap - Style configuration for highlighting. * @returns {any} */ - updateHighlightedFeatures (selectedFeatures, stylesMap) { + updateHighlightedFeatures (selectedFeatures, activeFeatures, stylesMap) { const { LngLatBounds } = this.maplibreModule - return updateHighlightedFeatures({ LngLatBounds, map: this.map, selectedFeatures, stylesMap }) + return updateHighlightedFeatures({ LngLatBounds, map: this.map, selectedFeatures, activeFeatures, stylesMap }) } // ========================== diff --git a/providers/maplibre/src/utils/highlightFeatures.js b/providers/maplibre/src/utils/highlightFeatures.js index ab39dac0..170feda5 100755 --- a/providers/maplibre/src/utils/highlightFeatures.js +++ b/providers/maplibre/src/utils/highlightFeatures.js @@ -32,12 +32,11 @@ const groupFeaturesBySource = (map, selectedFeatures) => { return featuresBySource } -const cleanupStaleSources = (map, previousSources, currentSources) => { +const cleanupStaleSources = (map, previousSources, currentSources, prefix) => { previousSources.forEach(src => { if (!currentSources.has(src)) { - const base = `highlight-${src}` - const layers = [`${base}-fill`, `${base}-line`, `${base}-symbol`] - layers.forEach(id => { + const base = `${prefix}-${src}` + ;[`${base}-fill`, `${base}-line`, `${base}-symbol`].forEach(id => { if (map.getLayer(id)) { map.setFilter(id, ['==', 'id', '']) } @@ -97,67 +96,89 @@ const calculateBounds = (LngLatBounds, renderedFeatures) => { return [bounds.getWest(), bounds.getSouth(), bounds.getEast(), bounds.getNorth()] } -const getSelectedImageId = (map, imageId) => map._symbolImageMap?.[imageId] ?? null +const getActiveImageId = (map, imageId) => map._activeSymbolImageMap?.[imageId] ?? null +const getSelectedImageId = (map, imageId) => map._selectedSymbolImageMap?.[imageId] ?? null -/** - * Update highlighted features using pure filters. - * Supports fill, line and symbol geometry, multi-source, cleanup, and bounds. - */ -export function updateHighlightedFeatures ({ LngLatBounds, map, selectedFeatures, stylesMap }) { - if (!map) { - return null - } - - const featuresBySource = groupFeaturesBySource(map, selectedFeatures) - const renderedFeatures = [] +const SELECTED_PREFIX = 'selected-highlight' +const applyFeatureHighlights = (map, features, stylesMap, prefix, getSymbolImageId) => { + const featuresBySource = groupFeaturesBySource(map, features) const currentSources = new Set(Object.keys(featuresBySource)) - const previousSources = map._highlightedSources || new Set() + const storageKey = `_${prefix.replaceAll('-', '')}Sources` + const previousSources = map[storageKey] || new Set() - cleanupStaleSources(map, previousSources, currentSources) - map._highlightedSources = currentSources + cleanupStaleSources(map, previousSources, currentSources, prefix) + map[storageKey] = currentSources - // Apply highlights for current sources currentSources.forEach(sourceId => { const { ids, fillIds, idProperty, layerId, hasFillGeometry } = featuresBySource[sourceId] const baseLayer = map.getLayer(layerId) const srcLayer = baseLayer.sourceLayer - - // Use the actual feature geometry to determine highlight type const geom = hasFillGeometry ? 'fill' : baseLayer.type - const base = `highlight-${sourceId}` + const base = `${prefix}-${sourceId}` + const { stroke, selectionStroke, strokeWidth, fill } = stylesMap[layerId] + const lineColor = prefix === SELECTED_PREFIX ? selectionStroke : stroke const idExpression = idProperty ? ['get', idProperty] : ['id'] const filter = ['in', idExpression, ['literal', [...ids]]] if (geom === 'fill') { - const { stroke, strokeWidth, fill } = stylesMap[layerId] const fillFilter = ['in', idExpression, ['literal', [...fillIds]]] - const linePaint = { 'line-color': stroke, 'line-width': strokeWidth } // Only apply fill highlight to polygon features, not to any co-selected line features applyHighlightLayer(map, `${base}-fill`, 'fill', sourceId, srcLayer, { 'fill-color': fill }, fillFilter) - applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, linePaint, filter) + applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, { 'line-color': lineColor, 'line-width': strokeWidth }, filter) } if (geom === 'line') { - const { stroke, strokeWidth } = stylesMap[layerId] - const linePaint = { 'line-color': stroke, 'line-width': strokeWidth } - // Clear any fill highlight from a previous polygon selection on the same source + // Clear any fill highlight from a previous polygon on the same source if (map.getLayer(`${base}-fill`)) { map.setFilter(`${base}-fill`, ['==', 'id', '']) } - applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, linePaint, filter) + applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, { 'line-color': lineColor, 'line-width': strokeWidth }, filter) } if (geom === 'symbol') { const imageId = map.getLayoutProperty(layerId, 'icon-image') - const selectedImageId = getSelectedImageId(map, imageId) - if (selectedImageId) { - applySymbolHighlightLayer(map, `${base}-symbol`, sourceId, srcLayer, layerId, selectedImageId, filter) + const symbolImageId = getSymbolImageId(map, imageId) + if (symbolImageId) { + applySymbolHighlightLayer(map, `${base}-symbol`, sourceId, srcLayer, layerId, symbolImageId, filter) } } + }) + + return featuresBySource +} - // Bounds only from rendered tiles +/** + * Update highlighted features using pure filters. + * activeFeatures (keyboard cursor) render with the active ring (yellow); selectedFeatures with the selected ring (black). + * Supports fill, line and symbol geometry, multi-source, cleanup, and bounds. + */ +export function updateHighlightedFeatures ({ LngLatBounds, map, selectedFeatures, activeFeatures, stylesMap }) { + if (!map) { + return null + } + + // Active cursor features — rendered first so selected layers appear on top + if (activeFeatures?.length) { + applyFeatureHighlights(map, activeFeatures, stylesMap, 'highlight', getActiveImageId) + } else { + cleanupStaleSources(map, map._highlightSources || new Set(), new Set(), 'highlight') + map._highlightSources = new Set() + } + + // Committed selection features + const featuresBySource = selectedFeatures?.length + ? applyFeatureHighlights(map, selectedFeatures, stylesMap, SELECTED_PREFIX, getSelectedImageId) + : (() => { + cleanupStaleSources(map, map._selectedhighlightSources || new Set(), new Set(), SELECTED_PREFIX) + map._selectedhighlightSources = new Set() + return {} + })() + + // Bounds only from selected features + const renderedFeatures = [] + Object.entries(featuresBySource).forEach(([, { ids, idProperty, layerId }]) => { renderedFeatures.push( ...map .queryRenderedFeatures({ layers: [layerId] }) diff --git a/providers/maplibre/src/utils/highlightFeatures.test.js b/providers/maplibre/src/utils/highlightFeatures.test.js index 67f80c36..4a788599 100644 --- a/providers/maplibre/src/utils/highlightFeatures.test.js +++ b/providers/maplibre/src/utils/highlightFeatures.test.js @@ -1,19 +1,22 @@ import { updateHighlightedFeatures } from './highlightFeatures.js' -function lngLatBounds () { - this.coords = [] - this.extend = (c) => this.coords.push(c) - this.getWest = () => Math.min(...this.coords.map(c => c[0])) - this.getSouth = () => Math.min(...this.coords.map(c => c[1])) - this.getEast = () => Math.max(...this.coords.map(c => c[0])) - this.getNorth = () => Math.max(...this.coords.map(c => c[1])) +class LngLatBounds { + constructor () { this.coords = [] } + extend (c) { this.coords.push(c) } + getWest () { return Math.min(...this.coords.map(c => c[0])) } + getSouth () { return Math.min(...this.coords.map(c => c[1])) } + getEast () { return Math.max(...this.coords.map(c => c[0])) } + getNorth () { return Math.max(...this.coords.map(c => c[1])) } } +const SYMBOL_IMAGE = 'symbol-abc123' const EMPTY_FILTER = ['==', 'id', ''] const STALE_SYMBOL_LAYER = 'highlight-stale-symbol' +const SEL_STALE_SYMBOL_LAYER = 'selected-highlight-stale-symbol' const makeMap = (overrides = {}) => ({ - _highlightedSources: new Set(), + _highlightSources: new Set(), + _selectedhighlightSources: new Set(), getLayer: jest.fn(), addLayer: jest.fn(), moveLayer: jest.fn(), @@ -25,7 +28,9 @@ const makeMap = (overrides = {}) => ({ ...overrides }) -describe('Highlighting Utils — fill and line', () => { +// ─── Active (cursor) features — highlight-* layers ──────────────────────────── + +describe('Highlighting Utils — active (cursor) fill and line', () => { let map const ALL_BRANCHES_FEATURES = [ @@ -38,7 +43,7 @@ describe('Highlighting Utils — fill and line', () => { const ALL_BRANCHES_STYLES = { l1: { stroke: 'red', fill: 'blue' }, l2: { stroke: 'green' } } beforeEach(() => { - map = makeMap({ _highlightedSources: new Set(['stale']) }) + map = makeMap({ _highlightSources: new Set(['stale']) }) }) test('All branches', () => { @@ -52,42 +57,82 @@ describe('Highlighting Utils — fill and line', () => { return null }) - const coordMax = 10 - const coordMid = 5 - map.queryRenderedFeatures.mockReturnValue([ - { id: 1, geometry: { coordinates: [coordMax, coordMax] } }, - { id: 2, properties: { customId: 4 }, geometry: { coordinates: [[0, 0], [coordMid, coordMid]] } } - ]) - - const bounds = updateHighlightedFeatures({ LngLatBounds: lngLatBounds, map, selectedFeatures: ALL_BRANCHES_FEATURES, stylesMap: ALL_BRANCHES_STYLES }) + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: ALL_BRANCHES_FEATURES, stylesMap: ALL_BRANCHES_STYLES }) expect(map.setFilter).toHaveBeenCalledWith('highlight-stale-fill', EMPTY_FILTER) expect(map.setFilter).toHaveBeenCalledWith(STALE_SYMBOL_LAYER, EMPTY_FILTER) expect(map.setFilter).toHaveBeenCalledWith('highlight-s2-fill', EMPTY_FILTER) expect(map.setFilter).toHaveBeenCalledWith('highlight-s2-line', expect.arrayContaining([['get', 'customId']])) - expect(bounds).toEqual([0, 0, 10, 10]) }) - test('null _highlightedSources falls back to empty set; line geom skips absent fill layer', () => { - map._highlightedSources = null + test('null _highlightSources falls back to empty set; line geom skips absent fill layer', () => { + map._highlightSources = null map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR - updateHighlightedFeatures({ LngLatBounds: lngLatBounds, map, selectedFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) expect(map.setFilter).not.toHaveBeenCalledWith('highlight-s1-fill', expect.anything()) }) test('persistent source skips cleanup; missing stale layers skip setFilter', () => { - map._highlightedSources = new Set(['stale', 's1']) + map._highlightSources = new Set(['stale', 's1']) map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR - updateHighlightedFeatures({ LngLatBounds: lngLatBounds, map, selectedFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) expect(map.setFilter).not.toHaveBeenCalledWith(expect.stringContaining('stale'), expect.anything()) }) }) +// ─── Selected (committed) features — selected-highlight-* layers ───────────── + +describe('Highlighting Utils — selected (committed) fill and line', () => { + let map + + const SELECTED_FEATURES = [ + { featureId: 1, layerId: 'l1', geometry: { type: 'Polygon' } } + ] + + const STYLES = { l1: { stroke: 'red', selectionStroke: '#ffdd00', fill: 'blue', strokeWidth: 3 } } + + beforeEach(() => { + map = makeMap({ _selectedhighlightSources: new Set(['stale']) }) + }) + + test('creates selected-highlight-* layers with selectionStroke color', () => { + map.getLayer.mockImplementation(id => { // NOSONAR + if (id.includes('stale')) { return {} } + if (id === 'l1') { return { source: 's1', type: 'fill' } } + return null + }) + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: SELECTED_FEATURES, stylesMap: STYLES }) + expect(map.setFilter).toHaveBeenCalledWith(SEL_STALE_SYMBOL_LAYER, EMPTY_FILTER) + expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ id: 'selected-highlight-s1-line' })) + const linePaintCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'selected-highlight-s1-line' && c[1] === 'line-color') + expect(linePaintCall).toBeTruthy() + expect(linePaintCall[2]).toBe('#ffdd00') + }) + + test('bounds are calculated from selected features only', () => { + const features = [ + { featureId: 1, layerId: 'l1', geometry: { type: 'Polygon' } }, + { featureId: 2, layerId: 'l1', geometry: { type: 'Polygon' } } + ] + map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR + const coordMax = 10 + const coordMid = 5 + map.queryRenderedFeatures.mockReturnValue([ + { id: 1, geometry: { coordinates: [coordMax, coordMax] } }, + { id: 2, geometry: { coordinates: [[0, 0], [coordMid, coordMid]] } } + ]) + const bounds = updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: features, stylesMap: STYLES }) + expect(bounds).toEqual([0, 0, 10, 10]) + }) +}) + +// ─── Layer management ───────────────────────────────────────────────────────── + describe('Highlighting Utils — layer management', () => { let map beforeEach(() => { - map = makeMap({ _highlightedSources: new Set(['stale']) }) + map = makeMap({ _highlightSources: new Set(['stale']) }) }) test('reuses existing highlight layer; new layer spreads sourceLayer', () => { @@ -97,19 +142,20 @@ describe('Highlighting Utils — layer management', () => { if (id === 'highlight-s1-line') { return {} } return null }) - updateHighlightedFeatures({ LngLatBounds: lngLatBounds, map, selectedFeatures: [{ featureId: 1, layerId: 'l1' }, { featureId: 2, layerId: 'l2' }], stylesMap: { l1: { stroke: 'blue' }, l2: { stroke: 'green' } } }) + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }, { featureId: 2, layerId: 'l2' }], stylesMap: { l1: { stroke: 'blue' }, l2: { stroke: 'green' } } }) expect(map.addLayer).toHaveBeenCalledTimes(1) expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ 'source-layer': 'tiles' })) }) test('returns null when no rendered features match', () => { - expect(updateHighlightedFeatures({ LngLatBounds: lngLatBounds, map, selectedFeatures: [], stylesMap: {} })).toBeNull() + expect(updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: [], stylesMap: {} })).toBeNull() }) }) -describe('Highlighting Utils — symbol layers', () => { - const SYMBOL_IMAGE = 'symbol-abc123' - const SELECTED_IMAGE = 'symbol-sel-abc123' +// ─── Symbol layers ──────────────────────────────────────────────────────────── + +describe('Highlighting Utils — symbol layers (active cursor)', () => { + const ACTIVE_IMAGE = 'symbol-act-abc123' const HIGHLIGHT_LAYER = 'highlight-s1-symbol' const ICON_IMAGE = 'icon-image' const ICON_ANCHOR = 'icon-anchor' @@ -119,22 +165,22 @@ describe('Highlighting Utils — symbol layers', () => { beforeEach(() => { map = makeMap() - map._symbolImageMap = { [SYMBOL_IMAGE]: SELECTED_IMAGE } + map._activeSymbolImageMap = { [SYMBOL_IMAGE]: ACTIVE_IMAGE } map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'symbol' } : null) // NOSONAR map.getLayoutProperty.mockReturnValue(SYMBOL_IMAGE) }) - const run = (selectedFeatures = [POINT_FEATURE]) => - updateHighlightedFeatures({ LngLatBounds: lngLatBounds, map, selectedFeatures, stylesMap: { l1: {} } }) + const run = (activeFeatures = [POINT_FEATURE]) => + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures, stylesMap: { l1: {} } }) - test('creates symbol highlight layer with selected image variant', () => { + test('creates symbol highlight layer with cursor image variant', () => { run() expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ id: HIGHLIGHT_LAYER, type: 'symbol', - layout: expect.objectContaining({ [ICON_IMAGE]: SELECTED_IMAGE }) + layout: expect.objectContaining({ [ICON_IMAGE]: ACTIVE_IMAGE }) })) - expect(map.setLayoutProperty).toHaveBeenCalledWith(HIGHLIGHT_LAYER, ICON_IMAGE, SELECTED_IMAGE) + expect(map.setLayoutProperty).toHaveBeenCalledWith(HIGHLIGHT_LAYER, ICON_IMAGE, ACTIVE_IMAGE) }) test('reads icon-anchor from original layer', () => { @@ -171,20 +217,54 @@ describe('Highlighting Utils — symbol layers', () => { }) run() expect(map.addLayer).not.toHaveBeenCalled() - expect(map.setLayoutProperty).toHaveBeenCalledWith(HIGHLIGHT_LAYER, ICON_IMAGE, SELECTED_IMAGE) + expect(map.setLayoutProperty).toHaveBeenCalledWith(HIGHLIGHT_LAYER, ICON_IMAGE, ACTIVE_IMAGE) }) - test('skips highlight when icon-image has no entry in _symbolImageMap', () => { - map.getLayoutProperty.mockReturnValue('symbol-abc123') - map._symbolImageMap = {} // no mapping registered + test('skips highlight when icon-image has no entry in _activeSymbolImageMap', () => { + map.getLayoutProperty.mockReturnValue(SYMBOL_IMAGE) + map._activeSymbolImageMap = {} // no mapping registered run() expect(map.addLayer).not.toHaveBeenCalled() }) test('cleans up stale symbol highlight layer', () => { - map._highlightedSources = new Set(['stale']) + map._highlightSources = new Set(['stale']) map.getLayer.mockImplementation(id => id === STALE_SYMBOL_LAYER ? { type: 'symbol' } : null) // NOSONAR run([]) expect(map.setFilter).toHaveBeenCalledWith(STALE_SYMBOL_LAYER, EMPTY_FILTER) }) }) + +describe('Highlighting Utils — symbol layers (committed selection)', () => { + const SELECTED_IMAGE = 'symbol-sel-abc123' + const SEL_HIGHLIGHT_LAYER = 'selected-highlight-s1-symbol' + const ICON_IMAGE = 'icon-image' + const POINT_FEATURE = { featureId: 1, layerId: 'l1', geometry: { type: 'Point' } } + + let map + + beforeEach(() => { + map = makeMap() + map._selectedSymbolImageMap = { [SYMBOL_IMAGE]: SELECTED_IMAGE } + map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'symbol' } : null) // NOSONAR + map.getLayoutProperty.mockReturnValue(SYMBOL_IMAGE) + }) + + const run = (selectedFeatures = [POINT_FEATURE]) => + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures, stylesMap: { l1: {} } }) + + test('creates selected-highlight symbol layer with selected image variant', () => { + run() + expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ + id: SEL_HIGHLIGHT_LAYER, + type: 'symbol', + layout: expect.objectContaining({ [ICON_IMAGE]: SELECTED_IMAGE }) + })) + }) + + test('skips highlight when icon-image has no entry in _selectedSymbolImageMap', () => { + map._selectedSymbolImageMap = {} + run() + expect(map.addLayer).not.toHaveBeenCalled() + }) +}) diff --git a/providers/maplibre/src/utils/maplibreFixes.js b/providers/maplibre/src/utils/maplibreFixes.js index 33d375bb..99c4231e 100755 --- a/providers/maplibre/src/utils/maplibreFixes.js +++ b/providers/maplibre/src/utils/maplibreFixes.js @@ -5,6 +5,16 @@ export function cleanCanvas (map) { canvas.setAttribute('tabindex', -1) // If removed altogether Chrome can add a focus-visible style canvas.removeAttribute('aria-label') canvas.style.display = 'block' + // The map container is aria-hidden, but MapLibre's canvas inside it can still + // receive focus, triggering: "Blocked aria-hidden on an element because its + // descendant retained focus." Immediately blurring and returning focus to + // relatedTarget prevents the canvas from holding focus inside an aria-hidden subtree. + canvas.addEventListener('focus', (event) => { // NOSONAR + canvas.blur() + if (event.relatedTarget) { + event.relatedTarget.focus() + } + }) } // Fix touch preventDefault console error diff --git a/providers/maplibre/src/utils/maplibreFixes.test.js b/providers/maplibre/src/utils/maplibreFixes.test.js index 617302af..4114b140 100644 --- a/providers/maplibre/src/utils/maplibreFixes.test.js +++ b/providers/maplibre/src/utils/maplibreFixes.test.js @@ -16,6 +16,37 @@ describe('cleanCanvas', () => { expect(canvas.hasAttribute('aria-label')).toBe(false) expect(canvas.style.display).toBe('block') }) + + it('redirects focus back to the previously focused element when canvas receives focus', () => { + const canvas = document.createElement('canvas') + const map = { getCanvas: () => canvas } + const prev = document.createElement('button') + prev.focus = jest.fn() + document.body.appendChild(canvas) + + cleanCanvas(map) + + const event = new FocusEvent('focus', { relatedTarget: prev }) + const blurSpy = jest.spyOn(canvas, 'blur') + canvas.dispatchEvent(event) + + expect(blurSpy).toHaveBeenCalled() + expect(prev.focus).toHaveBeenCalled() + }) + + it('blurs canvas without restoring focus when there is no previous element', () => { + const canvas = document.createElement('canvas') + const map = { getCanvas: () => canvas } + document.body.appendChild(canvas) + + cleanCanvas(map) + + const event = new FocusEvent('focus', { relatedTarget: null }) + const blurSpy = jest.spyOn(canvas, 'blur') + canvas.dispatchEvent(event) + + expect(blurSpy).toHaveBeenCalled() + }) }) describe('applyPreventDefaultFix', () => { diff --git a/providers/maplibre/src/utils/symbolImages.js b/providers/maplibre/src/utils/symbolImages.js index d7f5d59a..7f2f4372 100644 --- a/providers/maplibre/src/utils/symbolImages.js +++ b/providers/maplibre/src/utils/symbolImages.js @@ -61,16 +61,16 @@ export const anchorToMaplibre = ([ax, ay]) => { * @param {number} [pixelRatio=2] - Device pixel ratio × map size scale factor * @returns {string|null} */ -export const getSymbolImageId = (dataset, mapStyle, symbolRegistry, selected = false, pixelRatio = 2) => { +export const getSymbolImageId = (dataset, mapStyle, symbolRegistry, active = false, pixelRatio = 2) => { const symbolDef = getSymbolDef(dataset, symbolRegistry) if (!symbolDef) { return null } const styleColors = getSymbolStyleColors(dataset) - const resolved = selected - ? symbolRegistry.resolveSelected(symbolDef, styleColors, mapStyle) + const resolved = active + ? symbolRegistry.resolveActive(symbolDef, styleColors, mapStyle) : symbolRegistry.resolve(symbolDef, styleColors, mapStyle) - return `symbol-${selected ? 'sel-' : ''}${hashString(resolved)}-${pixelRatio}x` + return `symbol-${active ? 'act-' : ''}${hashString(resolved)}-${pixelRatio}x` } // ─── Rasterisation ──────────────────────────────────────────────────────────── @@ -78,17 +78,39 @@ export const getSymbolImageId = (dataset, mapStyle, symbolRegistry, selected = f // Module-level cache: imageId → ImageData. Avoids re-rasterising identical symbols. const imageDataCache = new Map() -const rasteriseSymbolImage = async (dataset, mapStyle, symbolRegistry, selected, pixelRatio) => { +/** + * Rasterise one variant of a symbol to ImageData for use as a MapLibre image. + * Results are cached by imageId so identical symbols are only rendered once. + * + * @param {Object} dataset - Dataset or marker config with symbol properties + * @param {Object} mapStyle - Current map style config + * @param {Object} symbolRegistry + * @param {'normal'|'active'|'selected'} variant + * - `'normal'` — no rings (default display) + * - `'active'` — both rings (keyboard cursor, yellow + black) + * - `'selected'` — selected ring only (black) + * @param {number} pixelRatio - Device pixel ratio × map size scale factor + * @returns {Promise<{imageId: string, imageData: ImageData}|null>} + */ +const rasteriseSymbolImage = async (dataset, mapStyle, symbolRegistry, variant, pixelRatio) => { const symbolDef = getSymbolDef(dataset, symbolRegistry) if (!symbolDef) { return null } const styleColors = getSymbolStyleColors(dataset) - const resolvedContent = selected - ? symbolRegistry.resolveSelected(symbolDef, styleColors, mapStyle) - : symbolRegistry.resolve(symbolDef, styleColors, mapStyle) + let resolvedContent, prefix + if (variant === 'active') { + resolvedContent = symbolRegistry.resolveActive(symbolDef, styleColors, mapStyle) + prefix = 'act-' + } else if (variant === 'selected') { + resolvedContent = symbolRegistry.resolveSelected(symbolDef, styleColors, mapStyle) + prefix = 'sel-' + } else { + resolvedContent = symbolRegistry.resolve(symbolDef, styleColors, mapStyle) + prefix = '' + } - const imageId = `symbol-${selected ? 'sel-' : ''}${hashString(resolvedContent)}-${pixelRatio}x` + const imageId = `symbol-${prefix}${hashString(resolvedContent)}-${pixelRatio}x` let imageData = imageDataCache.get(imageId) if (!imageData) { @@ -105,9 +127,9 @@ const rasteriseSymbolImage = async (dataset, mapStyle, symbolRegistry, selected, } /** - * Register normal and selected symbol images for the given pre-resolved symbol configs. + * Register normal, active (both rings) and selected (black ring) symbol images. * Skips images that are already registered (safe to call on style change). - * Updates `map._symbolImageMap` with normal→selected image ID pairs. + * Updates `map._activeSymbolImageMap` (normal→active) and `map._selectedSymbolImageMap` (normal→selected). * * Callers are responsible for resolving sublayers before calling this function * (see `getSymbolConfigs` in the datasets plugin adapter). @@ -124,23 +146,28 @@ export const registerSymbols = async (map, symbolConfigs, mapStyle, symbolRegist return } - // Reset the normal→selected image ID lookup so stale entries don't persist after a style change - map._symbolImageMap = {} + map._activeSymbolImageMap = {} + map._selectedSymbolImageMap = {} await Promise.all(symbolConfigs.flatMap(config => { const normalId = getSymbolImageId(config, mapStyle, symbolRegistry, false, pixelRatio) - const selectedId = getSymbolImageId(config, mapStyle, symbolRegistry, true, pixelRatio) - if (normalId && selectedId) { - map._symbolImageMap[normalId] = selectedId + const activeId = getSymbolImageId(config, mapStyle, symbolRegistry, true, pixelRatio) + if (normalId && activeId) { + map._activeSymbolImageMap[normalId] = activeId } - return [false, true].map(async (selected) => { - const imageId = selected ? selectedId : normalId - if (!imageId || map.hasImage(imageId)) { + return ['normal', 'active', 'selected'].map(async (variant) => { + const imageId = variant === 'active' ? activeId : normalId + if (variant !== 'selected' && (!imageId || map.hasImage(imageId))) { return } - const result = await rasteriseSymbolImage(config, mapStyle, symbolRegistry, selected, pixelRatio) - if (result && !map.hasImage(result.imageId)) { - map.addImage(result.imageId, result.imageData, { pixelRatio }) + const result = await rasteriseSymbolImage(config, mapStyle, symbolRegistry, variant, pixelRatio) + if (result) { + if (variant === 'selected' && normalId) { + map._selectedSymbolImageMap[normalId] = result.imageId + } + if (!map.hasImage(result.imageId)) { + map.addImage(result.imageId, result.imageData, { pixelRatio }) + } } }) })) diff --git a/src/App/components/Markers/Markers.jsx b/src/App/components/Markers/Markers.jsx index 9f344d40..68069ba3 100755 --- a/src/App/components/Markers/Markers.jsx +++ b/src/App/components/Markers/Markers.jsx @@ -6,12 +6,13 @@ import { useMap } from '../../store/mapContext.js' import { useService } from '../../store/serviceContext.js' import { scaleFactor } from '../../../config/appConfig.js' import { isStandaloneLabel } from '../../../utils/symbolUtils.js' +import { EVENTS } from '../../../config/events.js' import LabelMarker from './LabelMarker.jsx' import SymbolLabelMarker from './SymbolLabelMarker.jsx' import SymbolMarker from './SymbolMarker.jsx' // Marker properties handled internally — excluded from style value resolution -const INTERNAL_KEYS = new Set(['id', 'coords', 'x', 'y', 'isVisible', 'symbol', 'symbolSvgContent', 'viewBox', 'anchor', 'selectedColor', 'selectedWidth', 'label', 'showLabel']) +const INTERNAL_KEYS = new Set(['id', 'coords', 'x', 'y', 'isVisible', 'symbol', 'symbolSvgContent', 'viewBox', 'anchor', 'selectedColor', 'label', 'showLabel']) const resolveSymbolDef = (marker, defaults, symbolRegistry) => { const svgContent = marker.symbolSvgContent || defaults.symbolSvgContent @@ -21,19 +22,24 @@ const resolveSymbolDef = (marker, defaults, symbolRegistry) => { } const resolveViewBox = (marker, defaults, symbolDef) => - marker.viewBox || defaults.viewBox || symbolDef?.viewBox || '0 0 38 38' + marker.viewBox || defaults.viewBox || symbolDef?.viewBox || '0 0 44 44' const resolveAnchor = (marker, defaults, symbolDef) => marker.anchor ?? defaults.anchor ?? symbolDef?.anchor ?? [0.5, 0.5] -const resolveSymbolProps = (marker, defaults, symbolRegistry, mapStyle, mapSize, isSelected) => { +const resolveSymbolProps = (marker, defaults, symbolRegistry, mapStyle, mapSize, isSelected, isActive) => { const symbolDef = resolveSymbolDef(marker, defaults, symbolRegistry) const styleValues = Object.fromEntries( Object.entries(marker).filter(([k]) => !INTERNAL_KEYS.has(k)) ) - const resolvedSvg = isSelected - ? symbolRegistry.resolveSelected(symbolDef, styleValues, mapStyle) - : symbolRegistry.resolve(symbolDef, styleValues, mapStyle) + let resolvedSvg + if (isActive) { + resolvedSvg = symbolRegistry.resolveActive(symbolDef, styleValues, mapStyle) + } else if (isSelected) { + resolvedSvg = symbolRegistry.resolveSelected(symbolDef, styleValues, mapStyle) + } else { + resolvedSvg = symbolRegistry.resolve(symbolDef, styleValues, mapStyle) + } const viewBox = resolveViewBox(marker, defaults, symbolDef) const [,, svgWidth, svgHeight] = viewBox.split(' ').map(Number) const anchor = resolveAnchor(marker, defaults, symbolDef) @@ -52,16 +58,20 @@ export const Markers = () => { const [canSelectMarker, setCanSelectMarker] = useState(false) const [selectedMarkers, setSelectedMarkers] = useState([]) + const [activeMarkerId, setActiveMarkerId] = useState(null) const viewportRef = useRef(null) useEffect(() => { const handleActive = ({ active, interactionModes = [] }) => setCanSelectMarker(active && interactionModes.includes('selectMarker')) const handleSelectionChange = ({ selectedMarkers: next = [] }) => setSelectedMarkers(next) + const handleSetActive = ({ id: markerId }) => setActiveMarkerId(markerId) eventBus.on('interact:active', handleActive) eventBus.on('interact:selectionchange', handleSelectionChange) + eventBus.on(EVENTS.MAP_SET_ACTIVE_FEATURE, handleSetActive) return () => { eventBus.off('interact:active', handleActive) eventBus.off('interact:selectionchange', handleSelectionChange) + eventBus.off(EVENTS.MAP_SET_ACTIVE_FEATURE, handleSetActive) } }, [eventBus]) @@ -82,12 +92,13 @@ export const Markers = () => { <> {markers.items.map(marker => { const isSelected = selectedMarkers.includes(marker.id) + const isActive = marker.id === activeMarkerId if (isStandaloneLabel(marker)) { return } - const symbolProps = resolveSymbolProps(marker, defaults, symbolRegistry, mapStyle, mapSize, isSelected) + const symbolProps = resolveSymbolProps(marker, defaults, symbolRegistry, mapStyle, mapSize, isSelected, isActive) if (marker.showLabel && marker.label) { return diff --git a/src/App/components/Viewport/Features.jsx b/src/App/components/Viewport/Features.jsx index 7d92bc5f..1766e1b2 100644 --- a/src/App/components/Viewport/Features.jsx +++ b/src/App/components/Viewport/Features.jsx @@ -1,7 +1,7 @@ import React, { forwardRef } from 'react' import { useConfig } from '../../store/configContext.js' -export const Features = forwardRef(({ activeFeatureId, items = [], onFocus }, ref) => { +export const Features = forwardRef(({ activeFeatureId, selectedIds = [], multiselectable = false, items = [], onFocus, onBlur }, ref) => { const { id } = useConfig() const hasItems = items.length > 0 return ( @@ -12,14 +12,17 @@ export const Features = forwardRef(({ activeFeatureId, items = [], onFocus }, re tabIndex={hasItems ? '0' : '-1'} aria-hidden={hasItems ? undefined : true} aria-label='Map features' - aria-activedescendant={activeFeatureId || undefined} + aria-multiselectable={multiselectable || undefined} + aria-activedescendant={activeFeatureId ? `${id}-feature-${activeFeatureId}` : undefined} className='im-c-features' onFocus={onFocus} + onBlur={onBlur} > {items.map(item => (
  • {item.label}
    • diff --git a/src/App/components/Viewport/Features.module.scss b/src/App/components/Viewport/Features.module.scss index ff2bfe35..c279f617 100644 --- a/src/App/components/Viewport/Features.module.scss +++ b/src/App/components/Viewport/Features.module.scss @@ -19,4 +19,13 @@ &[data-focus-visible="true"][aria-activedescendant]::after { opacity: 0; } + + [role="option"] { + position: absolute; + width: 1px; + height: 1px; + overflow: hidden; + clip-path: inset(50%); + white-space: nowrap; + } } diff --git a/src/App/components/Viewport/Features.test.jsx b/src/App/components/Viewport/Features.test.jsx index e96dc356..e752125b 100644 --- a/src/App/components/Viewport/Features.test.jsx +++ b/src/App/components/Viewport/Features.test.jsx @@ -6,6 +6,8 @@ import { useConfig } from '../../store/configContext.js' jest.mock('../../store/configContext.js', () => ({ useConfig: jest.fn() })) const APP_ID = 'test-app' +const LISTBOX = '[role="listbox"]' // NOSONAR +const OPTION = '[role="option"]' // NOSONAR const ITEMS = [ { id: 'f1', label: 'Feature One' }, { id: 'f2', label: 'Feature Two' } @@ -21,7 +23,7 @@ describe('Features — rendering', () => { it('renders a listbox with the correct id', () => { const { container } = render() expect(container.querySelector(`#${APP_ID}-features`)).toBeTruthy() - expect(container.querySelector('[role="listbox"]')).toBeTruthy() // NOSONAR + expect(container.querySelector(LISTBOX)).toBeTruthy() // NOSONAR }) it('renders no options when items is empty', () => { @@ -29,43 +31,69 @@ describe('Features — rendering', () => { expect(container.querySelectorAll('[role="option"]')).toHaveLength(0) // NOSONAR }) - it('renders one option per item with correct id and label', () => { + it('renders one option per item with correct id, data-id and label', () => { const { container } = render() - const options = container.querySelectorAll('[role="option"]') // NOSONAR + const options = container.querySelectorAll(OPTION) expect(options).toHaveLength(2) expect(options[0].getAttribute('id')).toBe(`${APP_ID}-feature-f1`) + expect(options[0].dataset.id).toBe('f1') expect(options[0].textContent).toBe('Feature One') expect(options[1].getAttribute('id')).toBe(`${APP_ID}-feature-f2`) + expect(options[1].dataset.id).toBe('f2') expect(options[1].textContent).toBe('Feature Two') }) - it('sets aria-selected on the active item', () => { - const { container } = render() - const options = container.querySelectorAll('[role="option"]') // NOSONAR + it('sets aria-selected on items present in selectedIds', () => { + const { container } = render() + const options = container.querySelectorAll(OPTION) + expect(options[0]).toHaveAttribute('aria-selected', 'true') + expect(options[1]).toHaveAttribute('aria-selected', 'false') + }) + + it('sets aria-selected on multiple items when selectedIds has multiple entries', () => { + const { container } = render() + const options = container.querySelectorAll(OPTION) expect(options[0]).toHaveAttribute('aria-selected', 'true') + expect(options[1]).toHaveAttribute('aria-selected', 'true') + }) + + it('does not set aria-selected from activeFeatureId alone', () => { + const { container } = render() + const options = container.querySelectorAll(OPTION) + expect(options[0]).toHaveAttribute('aria-selected', 'false') expect(options[1]).toHaveAttribute('aria-selected', 'false') }) - it('sets aria-activedescendant when activeFeatureId is provided', () => { + it('sets aria-activedescendant to the option element id when activeFeatureId is provided', () => { const { container } = render() - expect(container.querySelector('[role="listbox"]').getAttribute('aria-activedescendant')).toBe('f2') // NOSONAR + expect(container.querySelector(LISTBOX).getAttribute('aria-activedescendant')).toBe('test-app-feature-f2') // NOSONAR }) it('omits aria-activedescendant when activeFeatureId is absent', () => { const { container } = render() - expect(container.querySelector('[role="listbox"]').getAttribute('aria-activedescendant')).toBeNull() // NOSONAR + expect(container.querySelector(LISTBOX).getAttribute('aria-activedescendant')).toBeNull() // NOSONAR + }) + + it('sets aria-multiselectable when multiselectable is true', () => { + const { container } = render() + expect(container.querySelector(LISTBOX).getAttribute('aria-multiselectable')).toBe('true') // NOSONAR + }) + + it('omits aria-multiselectable when multiselectable is false', () => { + const { container } = render() + expect(container.querySelector(LISTBOX).getAttribute('aria-multiselectable')).toBeNull() // NOSONAR }) it('is tabIndex 0 and not aria-hidden when items are present', () => { const { container } = render() - const ul = container.querySelector('[role="listbox"]') // NOSONAR + const ul = container.querySelector(LISTBOX) // NOSONAR expect(ul.getAttribute('tabIndex')).toBe('0') expect(ul.getAttribute('aria-hidden')).toBeNull() }) it('is tabIndex -1 and aria-hidden when items is empty', () => { const { container } = render() - const ul = container.querySelector('[role="listbox"]') // NOSONAR + const ul = container.querySelector(LISTBOX) // NOSONAR expect(ul.getAttribute('tabIndex')).toBe('-1') expect(ul.getAttribute('aria-hidden')).toBe('true') }) @@ -77,7 +105,14 @@ describe('Features — interactions', () => { it('calls onFocus when the listbox receives focus', () => { const onFocus = jest.fn() const { container } = render() - fireEvent.focus(container.querySelector('[role="listbox"]')) // NOSONAR + fireEvent.focus(container.querySelector(LISTBOX)) // NOSONAR expect(onFocus).toHaveBeenCalled() }) + + it('calls onBlur when the listbox loses focus', () => { + const onBlur = jest.fn() + const { container } = render() + fireEvent.blur(container.querySelector(LISTBOX)) // NOSONAR + expect(onBlur).toHaveBeenCalled() + }) }) diff --git a/src/App/components/Viewport/Viewport.jsx b/src/App/components/Viewport/Viewport.jsx index 3ed20823..17b72da5 100755 --- a/src/App/components/Viewport/Viewport.jsx +++ b/src/App/components/Viewport/Viewport.jsx @@ -32,8 +32,8 @@ export const Viewport = () => { // Local state for keyboard hint visibility const [keyboardHintVisible, setKeyboardHintVisible] = useState(false) - const featureItems = useFeatureItems(eventBus) - const { activeFeatureId, onFocus: onFeaturesFocus } = useFeatureFocus({ viewportRef: layoutRefs.viewportRef, featuresRef, items: featureItems, eventBus }) + const { items: featureItems, multiselectable } = useFeatureItems(eventBus) + const { activeFeatureId, selectedIds, onFocus: onFeaturesFocus, onBlur: onFeaturesBlur } = useFeatureFocus({ viewportRef: layoutRefs.viewportRef, featuresRef, items: featureItems, eventBus }) // Attach map keyboard controls useKeyboardShortcuts(layoutRefs.viewportRef) @@ -92,7 +92,7 @@ export const Viewport = () => { - + ) } diff --git a/src/App/hooks/useFeatureFocus.js b/src/App/hooks/useFeatureFocus.js index 396c584c..9daeb362 100644 --- a/src/App/hooks/useFeatureFocus.js +++ b/src/App/hooks/useFeatureFocus.js @@ -14,17 +14,27 @@ const getNavigatedId = (id, key, items) => { export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBus }) { const [activeFeatureId, setActiveFeatureId] = useState(null) + const [selectedIds, setSelectedIds] = useState([]) const activeFeatureIdRef = useRef(null) - useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) + useEffect(() => { + activeFeatureIdRef.current = activeFeatureId + }, [activeFeatureId]) useEffect(() => { if (!eventBus) { return undefined } - const handle = ({ id }) => { setActiveFeatureId(id) } - eventBus.on(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) - return () => { eventBus.off(EVENTS.MAP_SET_ACTIVE_FEATURE, handle) } + const handleSetActive = ({ id }) => { setActiveFeatureId(id) } + const handleSelectionChange = ({ selectedFeatures = [], selectedMarkers = [] }) => { + setSelectedIds([...selectedFeatures.map(f => String(f.featureId)), ...selectedMarkers]) + } + eventBus.on(EVENTS.MAP_SET_ACTIVE_FEATURE, handleSetActive) + eventBus.on('interact:selectionchange', handleSelectionChange) + return () => { + eventBus.off(EVENTS.MAP_SET_ACTIVE_FEATURE, handleSetActive) + eventBus.off('interact:selectionchange', handleSelectionChange) + } }, [eventBus]) useEffect(() => { @@ -36,14 +46,18 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu const handleKeyDown = (event) => { if (event.key === 'Escape') { event.preventDefault() - setActiveFeatureId(null) - eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: null }) + event.stopPropagation() viewportRef.current?.focus() } else if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { event.preventDefault() + event.stopPropagation() const newId = getNavigatedId(activeFeatureIdRef.current, event.key, items) setActiveFeatureId(newId) eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: newId }) + } else if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault() + event.stopPropagation() + eventBus?.emit(EVENTS.MAP_SELECT_FEATURE) } else { // No action } @@ -61,5 +75,10 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu } } - return { activeFeatureId, onFocus } + const onBlur = () => { + setActiveFeatureId(null) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: null }) + } + + return { activeFeatureId, selectedIds, onFocus, onBlur } } diff --git a/src/App/hooks/useFeatureFocus.test.js b/src/App/hooks/useFeatureFocus.test.js index 24f0d319..40bb1e84 100644 --- a/src/App/hooks/useFeatureFocus.test.js +++ b/src/App/hooks/useFeatureFocus.test.js @@ -8,6 +8,8 @@ const ITEMS = [ ] const SET_ACTIVE = 'map:setactivefeature' // NOSONAR +const SELECT = 'map:selectfeature' +const SELECTION_CHANGE = 'interact:selectionchange' const makeEventBus = () => { const listeners = {} @@ -25,7 +27,12 @@ const makeRefs = ({ viewportFocus } = {}) => ({ }) const fireKey = (el, key) => { - act(() => { el.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true })) }) + let event + act(() => { + event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }) + el.dispatchEvent(event) + }) + return event } // ─── useFeatureFocus — initial state ───────────────────────────────────────── @@ -40,6 +47,11 @@ describe('useFeatureFocus — initial state', () => { const { result } = renderHook(() => useFeatureFocus(makeRefs())) expect(typeof result.current.onFocus).toBe('function') }) + + it('exposes onBlur function', () => { + const { result } = renderHook(() => useFeatureFocus(makeRefs())) + expect(typeof result.current.onBlur).toBe('function') + }) }) // ─── useFeatureFocus — onFocus ──────────────────────────────────────────────── @@ -58,6 +70,32 @@ describe('useFeatureFocus — onFocus', () => { }) }) +// ─── useFeatureFocus — onBlur ───────────────────────────────────────────────── + +describe('useFeatureFocus — onBlur', () => { + it('clears activeFeatureId when focus leaves the listbox', () => { + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS })) + act(() => result.current.onFocus()) + expect(result.current.activeFeatureId).toBe('a') + act(() => result.current.onBlur()) + expect(result.current.activeFeatureId).toBeNull() + }) + + it('emits map:setactivefeature with null on blur', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS, eventBus: eb })) + act(() => result.current.onFocus()) + eb.emit.mockClear() + act(() => result.current.onBlur()) + expect(eb.emit).toHaveBeenCalledWith(SET_ACTIVE, { id: null }) + }) + + it('does not throw when eventBus is not provided', () => { + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS })) + expect(() => act(() => result.current.onBlur())).not.toThrow() + }) +}) + // ─── useFeatureFocus — keydown listener lifecycle ──────────────────────────── describe('useFeatureFocus — keydown listener lifecycle', () => { @@ -100,7 +138,7 @@ describe('useFeatureFocus — unhandled keys', () => { // ─── useFeatureFocus — Escape key ──────────────────────────────────────────── describe('useFeatureFocus — Escape key', () => { - it('clears activeFeatureId and focuses viewport', () => { + it('focuses the viewport on Escape (active state is cleared by the subsequent onBlur)', () => { const viewportFocus = jest.fn() const refs = makeRefs({ viewportFocus }) const el = refs.featuresRef.current @@ -108,7 +146,6 @@ describe('useFeatureFocus — Escape key', () => { const { result } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) act(() => result.current.onFocus()) fireKey(el, 'Escape') - expect(result.current.activeFeatureId).toBeNull() expect(viewportFocus).toHaveBeenCalled() el.remove() }) @@ -121,6 +158,18 @@ describe('useFeatureFocus — Escape key', () => { expect(() => fireKey(el, 'Escape')).not.toThrow() el.remove() }) + + it('stops propagation on Escape so the viewport keyboard handler does not also handle it', () => { + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) + const event = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }) + const stopSpy = jest.spyOn(event, 'stopPropagation') + act(() => el.dispatchEvent(event)) + expect(stopSpy).toHaveBeenCalled() + el.remove() + }) }) // ─── useFeatureFocus — ArrowDown navigation ─────────────────────────────────── @@ -171,6 +220,16 @@ describe('useFeatureFocus — ArrowDown navigation', () => { expect(result.current.activeFeatureId).toBeNull() unmount(); el.remove() }) + + it('stops propagation on ArrowDown so the viewport keyboard handler does not pan the map', () => { + const { result, el, unmount } = setup() + act(() => result.current.onFocus()) + const event = new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true, cancelable: true }) + const stopSpy = jest.spyOn(event, 'stopPropagation') + act(() => el.dispatchEvent(event)) + expect(stopSpy).toHaveBeenCalled() + unmount(); el.remove() + }) }) // ─── useFeatureFocus — ArrowUp navigation ──────────────────────────────────── @@ -238,16 +297,16 @@ describe('useFeatureFocus — eventBus: map:setactivefeature listener', () => { }) }) -describe('useFeatureFocus — eventBus: map:setactivefeature emit', () => { - const setupWithBus = () => { - const eb = makeEventBus() - const refs = makeRefs() - const el = refs.featuresRef.current - document.body.appendChild(el) - const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS, eventBus: eb })) - return { eb, el, result, unmount } - } +const setupWithBus = () => { + const eb = makeEventBus() + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS, eventBus: eb })) + return { eb, el, result, unmount } +} +describe('useFeatureFocus — eventBus: map:setactivefeature emit', () => { afterEach(() => { document.body.innerHTML = '' }) it('emits map:setactivefeature with first item id on onFocus', () => { @@ -265,15 +324,6 @@ describe('useFeatureFocus — eventBus: map:setactivefeature emit', () => { unmount() }) - it('emits map:setactivefeature with null on Escape', () => { - const { eb, result, el, unmount } = setupWithBus() - act(() => result.current.onFocus()) - eb.emit.mockClear() - fireKey(el, 'Escape') - expect(eb.emit).toHaveBeenCalledWith(SET_ACTIVE, { id: null }) - unmount() - }) - it('does not emit when eventBus is not provided', () => { const refs = makeRefs() const el = refs.featuresRef.current @@ -286,3 +336,102 @@ describe('useFeatureFocus — eventBus: map:setactivefeature emit', () => { unmount() }) }) + +// ─── useFeatureFocus — interact:selectionchange listener ───────────────────── + +describe('useFeatureFocus — interact:selectionchange listener', () => { + it('subscribes to interact:selectionchange on mount and unsubscribes on unmount', () => { + const eb = makeEventBus() + const { unmount } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + expect(eb.on).toHaveBeenCalledWith(SELECTION_CHANGE, expect.any(Function)) + unmount() + expect(eb.off).toHaveBeenCalledWith(SELECTION_CHANGE, expect.any(Function)) + }) + + it('sets selectedIds from selected features', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 27665979, layerId: 'hedges' }] })) + expect(result.current.selectedIds).toEqual(['27665979']) + }) + + it('sets selectedIds from multiple selected features', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'f1', layerId: 'roads' }, { featureId: 'f2', layerId: 'roads' }] })) + expect(result.current.selectedIds).toEqual(['f1', 'f2']) + }) + + it('sets selectedIds from selected markers', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedMarkers: ['m1', 'm2'] })) + expect(result.current.selectedIds).toEqual(['m1', 'm2']) + }) + + it('sets selectedIds from both features and markers when both are selected', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'f1', layerId: 'roads' }], selectedMarkers: ['m1'] })) + expect(result.current.selectedIds).toEqual(['f1', 'm1']) + }) + + it('clears selectedIds when selection becomes empty', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'f1', layerId: 'roads' }] })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [], selectedMarkers: [] })) + expect(result.current.selectedIds).toEqual([]) + }) + + it('does not move activeFeatureId when selectionchange fires (cursor is keyboard-only)', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS, eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'a', layerId: 'roads' }] })) + expect(result.current.activeFeatureId).toBeNull() + }) +}) + +// ─── useFeatureFocus — Enter and Space keys ─────────────────────────────────── + +describe('useFeatureFocus — Enter and Space keys', () => { + afterEach(() => { document.body.innerHTML = '' }) + + it('emits map:selectfeature on Enter', () => { + const { eb, result, el, unmount } = setupWithBus() + act(() => result.current.onFocus()) + eb.emit.mockClear() + fireKey(el, 'Enter') + expect(eb.emit).toHaveBeenCalledWith(SELECT) + unmount() + }) + + it('emits map:selectfeature on Space', () => { + const { eb, result, el, unmount } = setupWithBus() + act(() => result.current.onFocus()) + eb.emit.mockClear() + fireKey(el, ' ') + expect(eb.emit).toHaveBeenCalledWith(SELECT) + unmount() + }) + + it('stops propagation on Enter', () => { + const { el, result, unmount } = setupWithBus() + act(() => result.current.onFocus()) + const event = new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true }) + const stopSpy = jest.spyOn(event, 'stopPropagation') + act(() => el.dispatchEvent(event)) + expect(stopSpy).toHaveBeenCalled() + unmount() + }) + + it('stops propagation on Space', () => { + const { el, result, unmount } = setupWithBus() + act(() => result.current.onFocus()) + const event = new KeyboardEvent('keydown', { key: ' ', bubbles: true, cancelable: true }) + const stopSpy = jest.spyOn(event, 'stopPropagation') + act(() => el.dispatchEvent(event)) + expect(stopSpy).toHaveBeenCalled() + unmount() + }) +}) diff --git a/src/App/hooks/useFeatureItems.js b/src/App/hooks/useFeatureItems.js index 8c475ff3..e4971826 100644 --- a/src/App/hooks/useFeatureItems.js +++ b/src/App/hooks/useFeatureItems.js @@ -2,13 +2,15 @@ import { useState, useEffect } from 'react' export function useFeatureItems (eventBus) { const [items, setItems] = useState([]) + const [multiselectable, setMultiselectable] = useState(false) useEffect(() => { if (!eventBus) { return undefined } - const handle = ({ items: next = [] }) => { + const handle = ({ items: next = [], multiselectable: nextMultiselectable = false }) => { setItems(next) + setMultiselectable(nextMultiselectable) } eventBus.on('map:setfeatures', handle) return () => { @@ -16,5 +18,5 @@ export function useFeatureItems (eventBus) { } }, [eventBus]) - return items + return { items, multiselectable } } diff --git a/src/App/hooks/useFeatureItems.test.js b/src/App/hooks/useFeatureItems.test.js index 18ab0d64..1a6f1c63 100644 --- a/src/App/hooks/useFeatureItems.test.js +++ b/src/App/hooks/useFeatureItems.test.js @@ -1,6 +1,8 @@ import { renderHook, act } from '@testing-library/react' import { useFeatureItems } from './useFeatureItems.js' +const SET_FEATURES = 'map:setfeatures' // NOSONAR + const makeEventBus = () => { const listeners = {} return { @@ -13,14 +15,16 @@ const makeEventBus = () => { // ─── useFeatureItems — initial state ───────────────────────────────────────── describe('useFeatureItems — initial state', () => { - it('returns an empty array before any event is received', () => { + it('returns empty items and multiselectable false before any event', () => { const { result } = renderHook(() => useFeatureItems(makeEventBus())) - expect(result.current).toEqual([]) + expect(result.current.items).toEqual([]) + expect(result.current.multiselectable).toBe(false) }) - it('returns an empty array when eventBus is undefined', () => { + it('returns empty items and multiselectable false when eventBus is undefined', () => { const { result } = renderHook(() => useFeatureItems(undefined)) - expect(result.current).toEqual([]) + expect(result.current.items).toEqual([]) + expect(result.current.multiselectable).toBe(false) }) }) @@ -30,14 +34,14 @@ describe('useFeatureItems — event subscription', () => { it('subscribes to map:setfeatures on mount', () => { const eb = makeEventBus() renderHook(() => useFeatureItems(eb)) - expect(eb.on).toHaveBeenCalledWith('map:setfeatures', expect.any(Function)) // NOSONAR + expect(eb.on).toHaveBeenCalledWith(SET_FEATURES, expect.any(Function)) }) it('unsubscribes on unmount', () => { const eb = makeEventBus() const { unmount } = renderHook(() => useFeatureItems(eb)) unmount() - expect(eb.off).toHaveBeenCalledWith('map:setfeatures', expect.any(Function)) // NOSONAR + expect(eb.off).toHaveBeenCalledWith(SET_FEATURES, expect.any(Function)) }) it('does not subscribe when eventBus is undefined', () => { @@ -47,29 +51,47 @@ describe('useFeatureItems — event subscription', () => { }) }) -// ─── useFeatureItems — updates ──────────────────────────────────────────────── +// ─── useFeatureItems — items updates ───────────────────────────────────────── -describe('useFeatureItems — updates', () => { +describe('useFeatureItems — items updates', () => { it('updates items when map:setfeatures is emitted', () => { const eb = makeEventBus() const { result } = renderHook(() => useFeatureItems(eb)) const items = [{ id: 'a', label: 'Feature A' }, { id: 'b', label: 'Feature B' }] - act(() => eb.emit('map:setfeatures', { items })) // NOSONAR - expect(result.current).toEqual(items) + act(() => eb.emit(SET_FEATURES, { items })) + expect(result.current.items).toEqual(items) }) it('clears items when emitted with an empty array', () => { const eb = makeEventBus() const { result } = renderHook(() => useFeatureItems(eb)) - act(() => eb.emit('map:setfeatures', { items: [{ id: 'a', label: 'A' }] })) // NOSONAR - act(() => eb.emit('map:setfeatures', { items: [] })) // NOSONAR - expect(result.current).toEqual([]) + act(() => eb.emit(SET_FEATURES, { items: [{ id: 'a', label: 'A' }] })) + act(() => eb.emit(SET_FEATURES, { items: [] })) + expect(result.current.items).toEqual([]) + }) + + it('defaults items to empty array when items key is missing from payload', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureItems(eb)) + act(() => eb.emit(SET_FEATURES, {})) + expect(result.current.items).toEqual([]) + }) +}) + +// ─── useFeatureItems — multiselectable updates ─────────────────────────────── + +describe('useFeatureItems — multiselectable updates', () => { + it('sets multiselectable true when emitted with multiselectable: true', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureItems(eb)) + act(() => eb.emit(SET_FEATURES, { items: [], multiselectable: true })) + expect(result.current.multiselectable).toBe(true) }) - it('defaults to empty array when items key is missing from payload', () => { + it('defaults multiselectable to false when not present in payload', () => { const eb = makeEventBus() const { result } = renderHook(() => useFeatureItems(eb)) - act(() => eb.emit('map:setfeatures', {})) // NOSONAR - expect(result.current).toEqual([]) + act(() => eb.emit(SET_FEATURES, { items: [] })) + expect(result.current.multiselectable).toBe(false) }) }) diff --git a/src/config/events.js b/src/config/events.js index b971ecc1..4a01b7ff 100644 --- a/src/config/events.js +++ b/src/config/events.js @@ -127,6 +127,8 @@ export const EVENTS = { MAP_SET_FEATURES: 'map:setfeatures', /** @internal Set the active feature in the accessible features list. Payload: { id: string | null } */ MAP_SET_ACTIVE_FEATURE: 'map:setactivefeature', + /** @internal Select the active listbox feature as the real selection (Enter key). Payload: none */ + MAP_SELECT_FEATURE: 'map:selectfeature', /** @internal Set pixel ratio. Payload: pixelRatio */ MAP_SET_PIXEL_RATIO: 'map:setpixelratio', /** @internal Fit the map to a bounding box. Payload: [west, south, east, north] */ diff --git a/src/config/mapTheme.js b/src/config/mapTheme.js index b68b378b..c8083c6b 100644 --- a/src/config/mapTheme.js +++ b/src/config/mapTheme.js @@ -4,18 +4,20 @@ * used both by the symbol SVG renderer (JS canvas) and injected as * CSS custom properties onto the app container for CSS-rendered elements. * - * Per-style overrides take precedence: set `haloColor`, `selectedColor`, or + * Per-style overrides take precedence: set `haloColor`, `selectedColor`, `activeColor`, or * `foregroundColor` directly on a `MapStyleConfig` to override these defaults. */ -export const SCHEME_COLORS = { +export const THEME_COLORS = { light: { haloColor: '#ffffff', selectedColor: '#0b0c0c', + activeColor: '#ffdd00', foregroundColor: '#0b0c0c' }, dark: { haloColor: '#0b0c0c', selectedColor: '#ffffff', + activeColor: '#ffdd00', foregroundColor: '#ffffff' } } @@ -30,10 +32,11 @@ export const SCHEME_COLORS = { * @returns {{ haloColor: string, selectedColor: string, foregroundColor: string }} */ export const resolveMapTheme = (mapStyle) => { - const scheme = SCHEME_COLORS[mapStyle?.mapColorScheme] ?? SCHEME_COLORS.light + const scheme = THEME_COLORS[mapStyle?.mapColorScheme] ?? THEME_COLORS.light return { haloColor: mapStyle?.haloColor ?? scheme.haloColor, selectedColor: mapStyle?.selectedColor ?? scheme.selectedColor, + activeColor: mapStyle?.activeColor ?? scheme.activeColor, foregroundColor: mapStyle?.foregroundColor ?? scheme.foregroundColor } } @@ -47,10 +50,11 @@ export const resolveMapTheme = (mapStyle) => { * @returns {Object} CSS custom property object */ export const getMapThemeVars = (mapStyle) => { - const { haloColor, selectedColor, foregroundColor } = resolveMapTheme(mapStyle) + const { haloColor, selectedColor, activeColor, foregroundColor } = resolveMapTheme(mapStyle) return { '--map-overlay-halo-color': haloColor, '--map-overlay-selected-color': selectedColor, + '--map-overlay-active-color': activeColor, '--map-overlay-foreground-color': foregroundColor } } diff --git a/src/config/symbolConfig.js b/src/config/symbolConfig.js index ba7aae0f..97a3f83e 100644 --- a/src/config/symbolConfig.js +++ b/src/config/symbolConfig.js @@ -7,11 +7,15 @@ export const symbolDefaults = { symbol: 'pin', backgroundColor: '#ca3535', - foregroundColor: '#ffffff', - haloWidth: '1', - selectedWidth: '6' + foregroundColor: '#ffffff' } +/** Stroke width of the ring layer (selection + active rings) in SVG units */ +export const RING_WIDTH = 6 + +/** Stroke width of the halo (background shape outline) in SVG units */ +export const HALO_WIDTH = 2 + /** * Built-in graphic path data strings for use with the `graphic` token. * @@ -46,35 +50,35 @@ export const graphics = { } // ─── Built-in symbol definitions ───────────────────────────────────────────── -// Each symbol uses a 38×38 viewBox. SVG templates use {{token}} placeholders +// Each symbol uses a 44×44 viewBox. SVG templates use {{token}} placeholders // resolved at render time by the symbolRegistry. export const pin = { id: 'pin', - viewBox: '0 0 38 38', + viewBox: '0 0 44 44', anchor: [0.5, 0.9], // NOSONAR graphic: graphics.dot, - svg: ` - - ` + svg: ` + + ` } export const circle = { id: 'circle', - viewBox: '0 0 38 38', + viewBox: '0 0 44 44', anchor: [0.5, 0.5], graphic: graphics.dot, - svg: ` - - ` + svg: ` + + ` } export const square = { id: 'square', - viewBox: '0 0 38 38', + viewBox: '0 0 44 44', anchor: [0.5, 0.5], graphic: graphics.dot, - svg: ` - - ` + svg: ` + + ` } diff --git a/src/services/symbolRegistry.js b/src/services/symbolRegistry.js index 39f57d06..eb0b4d66 100644 --- a/src/services/symbolRegistry.js +++ b/src/services/symbolRegistry.js @@ -1,6 +1,6 @@ import { getValueForStyle } from '../utils/getValueForStyle.js' import { symbolDefaults, pin, circle, square, graphics } from '../config/symbolConfig.js' -import { SCHEME_COLORS } from '../config/mapTheme.js' +import { THEME_COLORS } from '../config/mapTheme.js' const symbols = new Map() let _constructorDefaults = {} @@ -8,14 +8,12 @@ let _constructorDefaults = {} // Keys that are structural — not token values for SVG substitution const STRUCTURAL = new Set(['id', 'svg', 'viewBox', 'anchor', 'symbol', 'symbolSvgContent']) -// selectedWidth is app-wide — not overridable at symbol registration level. -// selectedColor is a map style concern — always injected from mapStyle, never from cascade. -const REGISTRY_EXCLUDED = new Set([...STRUCTURAL, 'selectedWidth']) +// selectedColor and activeColor are map style concerns — always injected from mapStyle, never from cascade. function resolveValues (symbolDef, markerValues, mapStyle) { const mapStyleId = mapStyle?.id const symbolTokens = Object.fromEntries( - Object.entries(symbolDef || {}).filter(([k]) => !REGISTRY_EXCLUDED.has(k)) + Object.entries(symbolDef || {}).filter(([k]) => !STRUCTURAL.has(k)) ) const constructorTokens = Object.fromEntries( Object.entries(_constructorDefaults).filter(([k]) => !STRUCTURAL.has(k)) @@ -24,10 +22,11 @@ function resolveValues (symbolDef, markerValues, mapStyle) { Object.entries(markerValues).filter(([, v]) => v != null) ) const merged = { ...symbolDefaults, ...constructorTokens, ...symbolTokens, ...defined } - // haloColor and selectedColor are map style concerns — always injected from mapStyle, never from the cascade - const scheme = SCHEME_COLORS[mapStyle?.mapColorScheme] ?? SCHEME_COLORS.light + // haloColor, selectedColor and activeColor are map style concerns — always injected from mapStyle, never from the cascade + const scheme = THEME_COLORS[mapStyle?.mapColorScheme] ?? THEME_COLORS.light merged.haloColor = mapStyle?.haloColor ?? scheme.haloColor merged.selectedColor = mapStyle?.selectedColor ?? scheme.selectedColor + merged.activeColor = mapStyle?.activeColor ?? scheme.activeColor if (typeof merged.graphic === 'string' && graphics[merged.graphic]) { merged.graphic = graphics[merged.graphic] } @@ -77,33 +76,51 @@ export const symbolRegistry = { }, /** - * Resolve a symbol's SVG string for normal (unselected) rendering. - * The selected ring is always hidden regardless of cascade values. + * Resolve a symbol's SVG string for normal (unselected, inactive) rendering. + * Both selectedColor and activeColor are set to 'none' — all rings hidden. * * @param {Object} symbolDef - Symbol definition * @param {Object} styleColors - Token overrides - * @param {Object} mapStyle - Current map style config (provides selectedColor, haloColor) + * @param {Object} mapStyle - Current map style config (provides selectedColor, activeColor, haloColor) * @returns {string} Resolved SVG string */ resolve (symbolDef, styleColors, mapStyle) { const colors = resolveValues(symbolDef, styleColors || {}, mapStyle) if (!symbolDef) { return '' } - colors.selectedColor = '' + colors.selectedColor = 'none' + colors.activeColor = 'none' return resolveLayer(symbolDef.svg, colors) }, /** - * Resolve a symbol's SVG string for selected rendering. - * selectedColor comes from mapStyle.selectedColor (or the hardcoded fallback). + * Resolve a symbol's SVG string for active (keyboard cursor) rendering. + * Both selectedColor (committed ring) and activeColor (focus ring) are shown simultaneously, + * so an item that is active always displays both rings regardless of selection state. * * @param {Object} symbolDef - Symbol definition * @param {Object} styleColors - Token overrides - * @param {Object} mapStyle - Current map style config (provides selectedColor, haloColor) + * @param {Object} mapStyle - Current map style config (provides selectedColor, activeColor, haloColor) + * @returns {string} Resolved SVG string + */ + resolveActive (symbolDef, styleColors, mapStyle) { + const colors = resolveValues(symbolDef, styleColors || {}, mapStyle) + if (!symbolDef) { return '' } + return resolveLayer(symbolDef.svg, colors) + }, + + /** + * Resolve a symbol's SVG string for committed-selection rendering. + * selectedColor (committed ring) is shown; activeColor is set to 'none'. + * + * @param {Object} symbolDef - Symbol definition + * @param {Object} styleColors - Token overrides + * @param {Object} mapStyle - Current map style config (provides selectedColor, activeColor, haloColor) * @returns {string} Resolved SVG string */ resolveSelected (symbolDef, styleColors, mapStyle) { const colors = resolveValues(symbolDef, styleColors || {}, mapStyle) if (!symbolDef) { return '' } + colors.activeColor = 'none' return resolveLayer(symbolDef.svg, colors) } } diff --git a/src/services/symbolRegistry.test.js b/src/services/symbolRegistry.test.js index 7402f28e..5ee9a444 100644 --- a/src/services/symbolRegistry.test.js +++ b/src/services/symbolRegistry.test.js @@ -1,10 +1,12 @@ import { symbolRegistry } from './symbolRegistry.js' import { symbolDefaults } from '../config/symbolConfig.js' -import { SCHEME_COLORS } from '../config/mapTheme.js' +import { THEME_COLORS } from '../config/mapTheme.js' import { getValueForStyle } from '../utils/getValueForStyle.js' const STYLE_ID = 'test' const mapStyle = { id: STYLE_ID } +const COLOR_OVERRIDE = '#ff0000' +const FILL_OVERRIDE = `fill="${COLOR_OVERRIDE}"` beforeEach(() => { symbolRegistry.setDefaults({}) @@ -58,20 +60,20 @@ describe('symbolRegistry — setDefaults / getDefaults', () => { }) it('constructor defaults override hardcoded defaults', () => { - symbolRegistry.setDefaults({ backgroundColor: '#ff0000', symbol: 'circle' }) + symbolRegistry.setDefaults({ backgroundColor: COLOR_OVERRIDE, symbol: 'circle' }) const defaults = symbolRegistry.getDefaults() - expect(defaults.backgroundColor).toBe('#ff0000') + expect(defaults.backgroundColor).toBe(COLOR_OVERRIDE) expect(defaults.symbol).toBe('circle') }) it('constructor defaults do not affect unset properties', () => { - symbolRegistry.setDefaults({ backgroundColor: '#ff0000' }) + symbolRegistry.setDefaults({ backgroundColor: COLOR_OVERRIDE }) const defaults = symbolRegistry.getDefaults() expect(defaults.foregroundColor).toBe(symbolDefaults.foregroundColor) }) it('setDefaults with null or undefined resets to hardcoded defaults', () => { - symbolRegistry.setDefaults({ backgroundColor: '#ff0000' }) + symbolRegistry.setDefaults({ backgroundColor: COLOR_OVERRIDE }) symbolRegistry.setDefaults(null) expect(symbolRegistry.getDefaults().backgroundColor).toBe(symbolDefaults.backgroundColor) }) @@ -81,24 +83,23 @@ describe('symbolRegistry — resolve', () => { const BACKGROUND_SVG = '' const symbolDef = { id: 'test', - svg: '' + svg: '' } it('injects default token values when no overrides given', () => { const resolved = symbolRegistry.resolve(symbolDef, {}, mapStyle) expect(resolved).toContain(`fill="${getValueForStyle(symbolDefaults.backgroundColor, STYLE_ID)}"`) expect(resolved).toContain(`fill="${getValueForStyle(symbolDefaults.foregroundColor, STYLE_ID)}"`) - expect(resolved).toContain(`stroke-width="${symbolDefaults.haloWidth}"`) }) - it('always produces empty selectedColor token — ring is hidden', () => { + it('always produces empty selectedColor and activeColor tokens — rings are hidden', () => { const resolved = symbolRegistry.resolve(symbolDef, {}, mapStyle) - expect(resolved).toContain('stroke=""') + expect(resolved).toContain('stroke="none"') }) it('uses light scheme haloColor when mapStyle has no haloColor', () => { const resolved = symbolRegistry.resolve(symbolDef, {}, mapStyle) - expect(resolved).toContain(`stroke="${SCHEME_COLORS.light.haloColor}"`) + expect(resolved).toContain(`stroke="${THEME_COLORS.light.haloColor}"`) }) it('uses mapStyle.haloColor when provided', () => { @@ -107,8 +108,8 @@ describe('symbolRegistry — resolve', () => { }) it('overrides default backgroundColor with a plain string', () => { - const resolved = symbolRegistry.resolve(symbolDef, { backgroundColor: '#ff0000' }, mapStyle) - expect(resolved).toContain('fill="#ff0000"') + const resolved = symbolRegistry.resolve(symbolDef, { backgroundColor: COLOR_OVERRIDE }, mapStyle) + expect(resolved).toContain(FILL_OVERRIDE) }) it('overrides default with a style-keyed color', () => { @@ -163,44 +164,49 @@ describe('symbolRegistry — resolve', () => { }) }) -describe('symbolRegistry — resolveSelected', () => { +describe('symbolRegistry — resolveActive (keyboard cursor state)', () => { const symbolDef = { - id: 'test-sel', - svg: '' + id: 'test-active', + svg: '' } - it('uses light scheme selectedColor when mapStyle has no selectedColor', () => { - const resolved = symbolRegistry.resolveSelected(symbolDef, {}, mapStyle) - expect(resolved).toContain(`stroke="${SCHEME_COLORS.light.selectedColor}"`) + it('renders selectedColor from scheme when mapStyle has no selectedColor', () => { + const resolved = symbolRegistry.resolveActive(symbolDef, {}, mapStyle) + expect(resolved).toContain(`fill="${THEME_COLORS.light.selectedColor}"`) + }) + + it('renders activeColor from scheme when mapStyle has no activeColor', () => { + const resolved = symbolRegistry.resolveActive(symbolDef, {}, mapStyle) + expect(resolved).toContain(`stroke="${THEME_COLORS.light.activeColor}"`) }) it('uses mapStyle.selectedColor when provided', () => { - const resolved = symbolRegistry.resolveSelected(symbolDef, {}, { id: STYLE_ID, selectedColor: '#ff0000' }) - expect(resolved).toContain('stroke="#ff0000"') + const resolved = symbolRegistry.resolveActive(symbolDef, {}, { id: STYLE_ID, selectedColor: COLOR_OVERRIDE }) + expect(resolved).toContain(FILL_OVERRIDE) }) - it('uses selectedWidth from symbolDefaults', () => { - const resolved = symbolRegistry.resolveSelected(symbolDef, {}, mapStyle) - expect(resolved).toContain(`stroke-width="${symbolDefaults.selectedWidth}"`) + it('uses mapStyle.activeColor when provided', () => { + const resolved = symbolRegistry.resolveActive(symbolDef, {}, { id: STYLE_ID, activeColor: '#00ff00' }) + expect(resolved).toContain('stroke="#00ff00"') }) it('handles null styleColors — uses cascade defaults', () => { - const resolved = symbolRegistry.resolveSelected(symbolDef, null, mapStyle) - expect(resolved).toContain(`stroke="${SCHEME_COLORS.light.selectedColor}"`) + const resolved = symbolRegistry.resolveActive(symbolDef, null, mapStyle) + expect(resolved).toContain(`fill="${THEME_COLORS.light.selectedColor}"`) }) it('returns empty string for null symbolDef', () => { - expect(symbolRegistry.resolveSelected(null, {}, mapStyle)).toBe('') + expect(symbolRegistry.resolveActive(null, {}, mapStyle)).toBe('') }) it('symbol-level selectedColor is ignored — mapStyle wins', () => { const defWithSelected = { ...symbolDef, selectedColor: '#00ff00' } - const resolved = symbolRegistry.resolveSelected(defWithSelected, {}, { id: STYLE_ID, selectedColor: '#ff0000' }) - expect(resolved).toContain('stroke="#ff0000"') + const resolved = symbolRegistry.resolveActive(defWithSelected, {}, { id: STYLE_ID, selectedColor: COLOR_OVERRIDE }) + expect(resolved).toContain(FILL_OVERRIDE) }) it('still resolves other tokens correctly', () => { - const resolved = symbolRegistry.resolveSelected(symbolDef, { backgroundColor: '#d4351c' }, mapStyle) + const resolved = symbolRegistry.resolveActive(symbolDef, { backgroundColor: '#d4351c' }, mapStyle) expect(resolved).toContain('fill="#d4351c"') }) }) @@ -257,6 +263,6 @@ describe('symbolRegistry — graphic token', () => { const pin = symbolRegistry.get('pin') const resolved = symbolRegistry.resolve(pin, {}, mapStyle) expect(resolved).toContain(`d="${pin.graphic}"`) - expect(resolved).toContain('translate(19, 16) scale(0.8) translate(-8, -8)') + expect(resolved).toContain('translate(22, 19) scale(0.8) translate(-8, -8)') }) }) diff --git a/src/types.js b/src/types.js index e48ec442..7eff076e 100644 --- a/src/types.js +++ b/src/types.js @@ -377,7 +377,7 @@ * Alt text for logo. * * @property {'light' | 'dark'} [mapColorScheme] - * Map colour scheme. Sets the default values of `haloColor`, `selectedColor`, and `foregroundColor` + * Map colour scheme. Sets the default values of `haloColor`, `selectedColor`, `activeColor`, and `foregroundColor` * when not explicitly provided, and signals to map overlay components which tonal range to use. * `'light'` (default): dark overlays on a light basemap. `'dark'`: light overlays on a dark or aerial basemap. * @@ -390,10 +390,14 @@ * Injected as the `--map-overlay-halo-color` CSS custom property. * * @property {string} [selectedColor] - * Theme colour for selected state — used by map overlay components to indicate a selected feature. + * Theme colour for committed selection — used by map overlay components to indicate a selected feature. * Falls back to `#0b0c0c` (light) or `#ffffff` (dark). * Injected as the `--map-overlay-selected-color` CSS custom property. * + * @property {string} [activeColor] + * Theme colour for the active (keyboard cursor) state — the focus ring shown when navigating features + * with the keyboard. Falls back to `#ffdd00` for both light and dark schemes. + * * @property {string} [foregroundColor] * Foreground colour for elements rendered on top of the map (e.g. text or iconography in overlay components). * Falls back to `#0b0c0c` (light) or `#ffffff` (dark). @@ -415,7 +419,7 @@ * @property {string} [symbolSvgContent] * Default inner SVG path content. When set, overrides `symbol`. * - * @property {string} [viewBox='0 0 38 38'] + * @property {string} [viewBox='0 0 44 44'] * Default SVG viewBox. * * @property {[number, number]} [anchor=[0.5, 0.5]] @@ -427,15 +431,10 @@ * @property {string | Record} [foregroundColor='#ffffff'] * Default foreground fill colour. * - * @property {string} [haloWidth='1'] - * Default halo stroke width in SVG units. - * * @property {string} [graphic] * Default SVG `d` attribute value for the foreground graphic path. Each built-in symbol sets * its own default (a small dot). Override to swap the graphic across all markers globally. * - * @property {string} [selectedWidth='6'] - * Selection ring stroke width in SVG units. App-wide only — ignored at symbol registration and marker creation level. */ /** @@ -468,8 +467,8 @@ * When set, `symbol` is ignored. * * @property {string} [viewBox] - * SVG viewBox attribute for the symbol, e.g. `'0 0 38 38'`. - * Defaults to the registered symbol's viewBox, or `'0 0 38 38'`. + * SVG viewBox attribute for the symbol, e.g. `'0 0 44 44'`. + * Defaults to the registered symbol's viewBox, or `'0 0 44 44'`. * * @property {[number, number]} [anchor] * Anchor point as a normalised [x, y] pair where [0, 0] is top-left and [1, 1] is bottom-right. @@ -482,9 +481,6 @@ * @property {string | Record} [foregroundColor] * Foreground fill colour of the symbol (e.g. the inner dot on a pin). * - * @property {string} [haloWidth] - * Stroke width of the halo in SVG units. Defaults to `'1'`. - * * @property {string} [graphic] * SVG `d` attribute value for the foreground graphic path. Replaces the foreground shape of the * symbol while keeping its background, halo and selection ring intact. Each built-in symbol diff --git a/src/utils/symbolUtils.test.js b/src/utils/symbolUtils.test.js index 620e229d..6207749e 100644 --- a/src/utils/symbolUtils.test.js +++ b/src/utils/symbolUtils.test.js @@ -107,13 +107,11 @@ describe('getSymbolStyleColors', () => { symbol: 'pin', symbolBackgroundColor: '#ff0000', symbolForegroundColor: '#ffffff', - symbolHaloWidth: '2', symbolGraphic: 'cross' } expect(getSymbolStyleColors(dataset)).toEqual({ backgroundColor: '#ff0000', foregroundColor: '#ffffff', - haloWidth: '2', graphic: 'cross' }) }) From fe010b6da3ed94a91b0fc54ac19af1274098dcec Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Wed, 29 Apr 2026 15:48:14 +0100 Subject: [PATCH 03/10] Stroke width dfaults and docs updated --- docs/api/symbol-config.md | 4 +- plugins/interact/src/defaults.js | 3 +- plugins/interact/src/utils/buildStylesMap.js | 8 +- .../interact/src/utils/buildStylesMap.test.js | 11 ++- .../maplibre/src/utils/highlightFeatures.js | 84 +++++++++++-------- src/config/symbolConfig.js | 11 ++- 6 files changed, 69 insertions(+), 52 deletions(-) diff --git a/docs/api/symbol-config.md b/docs/api/symbol-config.md index 2fe8b2c1..e664e078 100644 --- a/docs/api/symbol-config.md +++ b/docs/api/symbol-config.md @@ -6,7 +6,7 @@ Symbol properties control the appearance of markers and point dataset features. Each property is optional. A value set directly on a marker or dataset layer takes priority over everything else. If a property is not set there, the value registered with the symbol is used. If the symbol has no value for that property, the app-wide `symbolDefaults` from the constructor applies. If none of those are set, the built-in fallback listed under each property below is used. -`haloColor`, `selectedColor`, and `activeColor` are required tokens in the SVG structure (see [SVG structure](#svg-structure)). Include them in any custom `symbolSvgContent` — the app resolves their values automatically. All three are always derived from the active map style and cannot be configured. Ring and halo stroke widths are hardcoded — use the exported `RING_WIDTH` and `HALO_WIDTH` constants from `symbolConfig.js` to keep your custom SVG consistent with the built-in symbols. +`haloColor`, `selectedColor`, and `activeColor` are required tokens in the SVG structure (see [SVG structure](#svg-structure)). Include them in any custom `symbolSvgContent` — the app resolves their values automatically. All three are derived from the active map style — configure them via `MapStyleConfig`, not per symbol or marker. ## Style-keyed colours @@ -137,7 +137,7 @@ svg: ` ` ``` -- **Layer 1** — ring layer: `fill` is the committed-selection ring (`{{selectedColor}}`), `stroke` is the active/focus ring (`{{activeColor}}`). Both hidden in normal rendering. `paint-order="stroke fill"` ensures the stroke extends outward without clipping the fill. +- **Layer 1** — ring layer: `fill` is the selected ring (`{{selectedColor}}`), `stroke` is the active ring (`{{activeColor}}`). Both hidden in normal rendering. `paint-order="stroke fill"` ensures the stroke extends outward without clipping the fill. - **Layer 2** — background shape with fixed halo stroke - **Layer 3** — foreground graphic (e.g. inner dot) diff --git a/plugins/interact/src/defaults.js b/plugins/interact/src/defaults.js index f50e3172..26a13fa8 100755 --- a/plugins/interact/src/defaults.js +++ b/plugins/interact/src/defaults.js @@ -4,6 +4,5 @@ export const DEFAULTS = { multiSelect: false, contiguous: false, deselectOnClickOutside: false, - marker: {}, - selectedStrokeWidth: 3 + marker: {} } diff --git a/plugins/interact/src/utils/buildStylesMap.js b/plugins/interact/src/utils/buildStylesMap.js index d5846d92..720ee2bf 100755 --- a/plugins/interact/src/utils/buildStylesMap.js +++ b/plugins/interact/src/utils/buildStylesMap.js @@ -1,6 +1,6 @@ import { getValueForStyle } from '../../../../src/utils/getValueForStyle.js' import { THEME_COLORS } from '../../../../src/config/mapTheme.js' -import { DEFAULTS } from '../defaults.js' +import { SELECTED_STROKE_WIDTH, ACTIVE_STROKE_WIDTH } from '../../../../src/config/symbolConfig.js' /** * Builds a map of layerId → resolved highlight style for the given data layers. @@ -32,13 +32,15 @@ export const buildStylesMap = (dataLayers, mapStyle) => { const activeStroke = layer.activeStroke || schemeActiveColor const selectionStroke = layer.selectedStroke || schemeSelectedColor const fill = layer.selectedFill || 'transparent' - const strokeWidth = layer.selectedStrokeWidth || DEFAULTS.selectedStrokeWidth + const strokeWidth = layer.selectedStrokeWidth || SELECTED_STROKE_WIDTH + const activeStrokeWidth = strokeWidth + ACTIVE_STROKE_WIDTH // ACTIVE_STROKE_WIDTH is the total overhang (extends SELECTED_STROKE_WIDTH each side) stylesMap[layer.layerId] = { stroke: getValueForStyle(activeStroke, mapStyle.id), selectionStroke: getValueForStyle(selectionStroke, mapStyle.id), fill: getValueForStyle(fill, mapStyle.id), - strokeWidth + strokeWidth, + activeStrokeWidth } }) diff --git a/plugins/interact/src/utils/buildStylesMap.test.js b/plugins/interact/src/utils/buildStylesMap.test.js index 01451fda..249dc2da 100644 --- a/plugins/interact/src/utils/buildStylesMap.test.js +++ b/plugins/interact/src/utils/buildStylesMap.test.js @@ -1,12 +1,14 @@ import { buildStylesMap } from './buildStylesMap.js' -import { DEFAULTS } from '../defaults.js' import { THEME_COLORS } from '../../../../src/config/mapTheme.js' +import { SELECTED_STROKE_WIDTH, ACTIVE_STROKE_WIDTH } from '../../../../src/config/symbolConfig.js' import { getValueForStyle } from '../../../../src/utils/getValueForStyle.js' jest.mock('../../../../src/utils/getValueForStyle.js', () => ({ getValueForStyle: jest.fn((value) => value) })) +const CUSTOM_STROKE_WIDTH = 5 + describe('buildStylesMap', () => { beforeEach(() => { jest.clearAllMocks() @@ -20,14 +22,15 @@ describe('buildStylesMap', () => { it('builds correct stylesMap with layer overrides', () => { const dataLayers = [ - { layerId: 'custom1', activeStroke: 'yellow', selectedStroke: 'red', selectedFill: 'blue', selectedStrokeWidth: 5 } + { layerId: 'custom1', activeStroke: 'yellow', selectedStroke: 'red', selectedFill: 'blue', selectedStrokeWidth: CUSTOM_STROKE_WIDTH } ] const result = buildStylesMap(dataLayers, { id: 'default-style' }) expect(result.custom1).toEqual({ stroke: 'yellow', selectionStroke: 'red', fill: 'blue', - strokeWidth: 5 + strokeWidth: CUSTOM_STROKE_WIDTH, + activeStrokeWidth: CUSTOM_STROKE_WIDTH + ACTIVE_STROKE_WIDTH }) }) @@ -37,7 +40,7 @@ describe('buildStylesMap', () => { expect(result.layer1.stroke).toBe(THEME_COLORS.light.activeColor) expect(result.layer1.selectionStroke).toBe(THEME_COLORS.light.selectedColor) expect(result.layer1.fill).toBe('transparent') - expect(result.layer1.strokeWidth).toBe(DEFAULTS.selectedStrokeWidth) + expect(result.layer1.strokeWidth).toBe(SELECTED_STROKE_WIDTH) }) it('uses mapStyle.activeColor and selectedColor when provided', () => { diff --git a/providers/maplibre/src/utils/highlightFeatures.js b/providers/maplibre/src/utils/highlightFeatures.js index 170feda5..bdda2fdf 100755 --- a/providers/maplibre/src/utils/highlightFeatures.js +++ b/providers/maplibre/src/utils/highlightFeatures.js @@ -1,3 +1,5 @@ +const ICON_IMAGE = 'icon-image' + const groupFeaturesBySource = (map, selectedFeatures) => { const featuresBySource = {} @@ -70,13 +72,13 @@ const applySymbolHighlightLayer = (map, id, sourceId, srcLayer, originalLayerId, source: sourceId, ...(srcLayer && { 'source-layer': srcLayer }), layout: { - 'icon-image': imageId, + [ICON_IMAGE]: imageId, 'icon-anchor': map.getLayoutProperty(originalLayerId, 'icon-anchor') ?? 'center', 'icon-allow-overlap': true } }) } - map.setLayoutProperty(id, 'icon-image', imageId) + map.setLayoutProperty(id, ICON_IMAGE, imageId) map.setFilter(id, filter) map.moveLayer(id) } @@ -101,50 +103,58 @@ const getSelectedImageId = (map, imageId) => map._selectedSymbolImageMap?.[image const SELECTED_PREFIX = 'selected-highlight' -const applyFeatureHighlights = (map, features, stylesMap, prefix, getSymbolImageId) => { - const featuresBySource = groupFeaturesBySource(map, features) - const currentSources = new Set(Object.keys(featuresBySource)) - const storageKey = `_${prefix.replaceAll('-', '')}Sources` - const previousSources = map[storageKey] || new Set() - - cleanupStaleSources(map, previousSources, currentSources, prefix) - map[storageKey] = currentSources - - currentSources.forEach(sourceId => { - const { ids, fillIds, idProperty, layerId, hasFillGeometry } = featuresBySource[sourceId] - const baseLayer = map.getLayer(layerId) - const srcLayer = baseLayer.sourceLayer - const geom = hasFillGeometry ? 'fill' : baseLayer.type - const base = `${prefix}-${sourceId}` - const { stroke, selectionStroke, strokeWidth, fill } = stylesMap[layerId] - const lineColor = prefix === SELECTED_PREFIX ? selectionStroke : stroke - - const idExpression = idProperty ? ['get', idProperty] : ['id'] - const filter = ['in', idExpression, ['literal', [...ids]]] +const applySymbolGeomHighlight = (map, base, sourceId, srcLayer, layerId, filter, getSymbolImageId) => { + const imageId = map.getLayoutProperty(layerId, ICON_IMAGE) + const symbolImageId = getSymbolImageId(map, imageId) + if (symbolImageId) { + applySymbolHighlightLayer(map, `${base}-symbol`, sourceId, srcLayer, layerId, symbolImageId, filter) + } +} - if (geom === 'fill') { +const applySourceHighlight = (map, sourceId, featuresBySource, stylesMap, prefix, getSymbolImageId, isSelected) => { + const { ids, fillIds, idProperty, layerId, hasFillGeometry } = featuresBySource[sourceId] + const baseLayer = map.getLayer(layerId) + const srcLayer = baseLayer.sourceLayer + const geom = hasFillGeometry ? 'fill' : baseLayer.type + const base = `${prefix}-${sourceId}` + const { stroke, selectionStroke, strokeWidth, activeStrokeWidth, fill } = stylesMap[layerId] + const lineColor = isSelected ? selectionStroke : stroke + const lineWidth = isSelected ? strokeWidth : activeStrokeWidth + const idExpression = idProperty ? ['get', idProperty] : ['id'] + const filter = ['in', idExpression, ['literal', [...ids]]] + + if (geom === 'fill') { + if (isSelected) { const fillFilter = ['in', idExpression, ['literal', [...fillIds]]] // Only apply fill highlight to polygon features, not to any co-selected line features applyHighlightLayer(map, `${base}-fill`, 'fill', sourceId, srcLayer, { 'fill-color': fill }, fillFilter) - applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, { 'line-color': lineColor, 'line-width': strokeWidth }, filter) } + applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, { 'line-color': lineColor, 'line-width': lineWidth }, filter) + } - if (geom === 'line') { + if (geom === 'line') { + if (map.getLayer(`${base}-fill`)) { // Clear any fill highlight from a previous polygon on the same source - if (map.getLayer(`${base}-fill`)) { - map.setFilter(`${base}-fill`, ['==', 'id', '']) - } - applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, { 'line-color': lineColor, 'line-width': strokeWidth }, filter) + map.setFilter(`${base}-fill`, ['==', 'id', '']) } + applyHighlightLayer(map, `${base}-line`, 'line', sourceId, srcLayer, { 'line-color': lineColor, 'line-width': lineWidth }, filter) + } - if (geom === 'symbol') { - const imageId = map.getLayoutProperty(layerId, 'icon-image') - const symbolImageId = getSymbolImageId(map, imageId) - if (symbolImageId) { - applySymbolHighlightLayer(map, `${base}-symbol`, sourceId, srcLayer, layerId, symbolImageId, filter) - } - } - }) + if (geom === 'symbol') { + applySymbolGeomHighlight(map, base, sourceId, srcLayer, layerId, filter, getSymbolImageId) + } +} + +const applyFeatureHighlights = (map, features, stylesMap, prefix, getSymbolImageId) => { + const featuresBySource = groupFeaturesBySource(map, features) + const currentSources = new Set(Object.keys(featuresBySource)) + const storageKey = `_${prefix.replaceAll('-', '')}Sources` + const previousSources = map[storageKey] || new Set() + const isSelected = prefix === SELECTED_PREFIX + + cleanupStaleSources(map, previousSources, currentSources, prefix) + map[storageKey] = currentSources + currentSources.forEach(sourceId => applySourceHighlight(map, sourceId, featuresBySource, stylesMap, prefix, getSymbolImageId, isSelected)) return featuresBySource } diff --git a/src/config/symbolConfig.js b/src/config/symbolConfig.js index 97a3f83e..14ca8917 100644 --- a/src/config/symbolConfig.js +++ b/src/config/symbolConfig.js @@ -10,11 +10,14 @@ export const symbolDefaults = { foregroundColor: '#ffffff' } -/** Stroke width of the ring layer (selection + active rings) in SVG units */ -export const RING_WIDTH = 6 +/** Stroke width for the halo (background shape outline) in SVG units */ +export const HALO_STROKE_WIDTH = 2 -/** Stroke width of the halo (background shape outline) in SVG units */ -export const HALO_WIDTH = 2 +/** Stroke width for the selected state — symbol rings and feature highlight lines */ +export const SELECTED_STROKE_WIDTH = 3 + +/** Stroke width for the active (keyboard cursor) state — double selected, extends 1× each side */ +export const ACTIVE_STROKE_WIDTH = SELECTED_STROKE_WIDTH * 2 /** * Built-in graphic path data strings for use with the `graphic` token. From 0bb2c9aeb84e22bdada9e8d3e8ef448ee2c35c15 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Wed, 29 Apr 2026 16:20:28 +0100 Subject: [PATCH 04/10] Active stroke rendering amends --- demo/js/index.js | 6 +- .../maplibre/src/utils/highlightFeatures.js | 60 ++++++++++------- .../src/utils/highlightFeatures.test.js | 64 +++++++++++++------ 3 files changed, 85 insertions(+), 45 deletions(-) diff --git a/demo/js/index.js b/demo/js/index.js index 2019f621..1f77ab6c 100755 --- a/demo/js/index.js +++ b/demo/js/index.js @@ -35,23 +35,27 @@ const interactPlugin = createInteractPlugin({ // idProperty: 'gid' },{ layerId: 'land-covers-110', + labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-130-131', + labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-332', + labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-379', + labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-other', + labelProperty: 'gid' // idProperty: 'gid' }, { layerId: 'hedge-control', - labelProperty: 'id', idProperty: 'id' }, // { diff --git a/providers/maplibre/src/utils/highlightFeatures.js b/providers/maplibre/src/utils/highlightFeatures.js index bdda2fdf..4d066307 100755 --- a/providers/maplibre/src/utils/highlightFeatures.js +++ b/providers/maplibre/src/utils/highlightFeatures.js @@ -1,5 +1,15 @@ const ICON_IMAGE = 'icon-image' +const ACTIVE_PREFIX = 'active-highlight' +const ACTIVE_INNER_PREFIX = 'active-highlight-inner' +const SELECTED_PREFIX = 'selected-highlight' + +// Inner and selected both use the selected colour/width (black, thin) +const usesSelectedStyle = (prefix) => prefix === SELECTED_PREFIX || prefix === ACTIVE_INNER_PREFIX + +const getActiveImageId = (map, imageId) => map._activeSymbolImageMap?.[imageId] ?? null +const getSelectedImageId = (map, imageId) => map._selectedSymbolImageMap?.[imageId] ?? null + const groupFeaturesBySource = (map, selectedFeatures) => { const featuresBySource = {} @@ -22,7 +32,7 @@ const groupFeaturesBySource = (map, selectedFeatures) => { } } - // Track whether any selected feature on this source is a polygon + // Track whether any feature on this source is a polygon if (geometry && (geometry.type === 'Polygon' || geometry.type === 'MultiPolygon')) { featuresBySource[sourceId].hasFillGeometry = true featuresBySource[sourceId].fillIds.add(featureId) @@ -47,6 +57,12 @@ const cleanupStaleSources = (map, previousSources, currentSources, prefix) => { }) } +const clearPrefixSources = (map, prefix) => { + const key = `_${prefix.replaceAll('-', '')}Sources` + cleanupStaleSources(map, map[key] || new Set(), new Set(), prefix) + map[key] = new Set() +} + const applyHighlightLayer = (map, id, type, sourceId, srcLayer, paint, filter) => { if (!map.getLayer(id)) { map.addLayer({ @@ -98,11 +114,6 @@ const calculateBounds = (LngLatBounds, renderedFeatures) => { return [bounds.getWest(), bounds.getSouth(), bounds.getEast(), bounds.getNorth()] } -const getActiveImageId = (map, imageId) => map._activeSymbolImageMap?.[imageId] ?? null -const getSelectedImageId = (map, imageId) => map._selectedSymbolImageMap?.[imageId] ?? null - -const SELECTED_PREFIX = 'selected-highlight' - const applySymbolGeomHighlight = (map, base, sourceId, srcLayer, layerId, filter, getSymbolImageId) => { const imageId = map.getLayoutProperty(layerId, ICON_IMAGE) const symbolImageId = getSymbolImageId(map, imageId) @@ -111,15 +122,17 @@ const applySymbolGeomHighlight = (map, base, sourceId, srcLayer, layerId, filter } } -const applySourceHighlight = (map, sourceId, featuresBySource, stylesMap, prefix, getSymbolImageId, isSelected) => { +const applySourceHighlight = (map, sourceId, featuresBySource, stylesMap, prefix, getSymbolImageId) => { const { ids, fillIds, idProperty, layerId, hasFillGeometry } = featuresBySource[sourceId] const baseLayer = map.getLayer(layerId) const srcLayer = baseLayer.sourceLayer const geom = hasFillGeometry ? 'fill' : baseLayer.type const base = `${prefix}-${sourceId}` const { stroke, selectionStroke, strokeWidth, activeStrokeWidth, fill } = stylesMap[layerId] - const lineColor = isSelected ? selectionStroke : stroke - const lineWidth = isSelected ? strokeWidth : activeStrokeWidth + const isSelected = prefix === SELECTED_PREFIX + const selectedStyle = usesSelectedStyle(prefix) + const lineColor = selectedStyle ? selectionStroke : stroke + const lineWidth = selectedStyle ? strokeWidth : activeStrokeWidth const idExpression = idProperty ? ['get', idProperty] : ['id'] const filter = ['in', idExpression, ['literal', [...ids]]] @@ -150,18 +163,18 @@ const applyFeatureHighlights = (map, features, stylesMap, prefix, getSymbolImage const currentSources = new Set(Object.keys(featuresBySource)) const storageKey = `_${prefix.replaceAll('-', '')}Sources` const previousSources = map[storageKey] || new Set() - const isSelected = prefix === SELECTED_PREFIX cleanupStaleSources(map, previousSources, currentSources, prefix) map[storageKey] = currentSources - currentSources.forEach(sourceId => applySourceHighlight(map, sourceId, featuresBySource, stylesMap, prefix, getSymbolImageId, isSelected)) + currentSources.forEach(sourceId => applySourceHighlight(map, sourceId, featuresBySource, stylesMap, prefix, getSymbolImageId)) return featuresBySource } /** * Update highlighted features using pure filters. - * activeFeatures (keyboard cursor) render with the active ring (yellow); selectedFeatures with the selected ring (black). + * activeFeatures (keyboard cursor) render with the active ring (yellow) plus a selected ring inner (black). + * selectedFeatures render with the selected ring (black) only. * Supports fill, line and symbol geometry, multi-source, cleanup, and bounds. */ export function updateHighlightedFeatures ({ LngLatBounds, map, selectedFeatures, activeFeatures, stylesMap }) { @@ -171,20 +184,21 @@ export function updateHighlightedFeatures ({ LngLatBounds, map, selectedFeatures // Active cursor features — rendered first so selected layers appear on top if (activeFeatures?.length) { - applyFeatureHighlights(map, activeFeatures, stylesMap, 'highlight', getActiveImageId) + applyFeatureHighlights(map, activeFeatures, stylesMap, ACTIVE_PREFIX, getActiveImageId) + // Black selected stroke on top of yellow active (mirrors resolveActive for symbols) + applyFeatureHighlights(map, activeFeatures, stylesMap, ACTIVE_INNER_PREFIX, getSelectedImageId) } else { - cleanupStaleSources(map, map._highlightSources || new Set(), new Set(), 'highlight') - map._highlightSources = new Set() + clearPrefixSources(map, ACTIVE_PREFIX) + clearPrefixSources(map, ACTIVE_INNER_PREFIX) } - // Committed selection features - const featuresBySource = selectedFeatures?.length - ? applyFeatureHighlights(map, selectedFeatures, stylesMap, SELECTED_PREFIX, getSelectedImageId) - : (() => { - cleanupStaleSources(map, map._selectedhighlightSources || new Set(), new Set(), SELECTED_PREFIX) - map._selectedhighlightSources = new Set() - return {} - })() + // Selection features + let featuresBySource = {} + if (selectedFeatures?.length) { + featuresBySource = applyFeatureHighlights(map, selectedFeatures, stylesMap, SELECTED_PREFIX, getSelectedImageId) + } else { + clearPrefixSources(map, SELECTED_PREFIX) + } // Bounds only from selected features const renderedFeatures = [] diff --git a/providers/maplibre/src/utils/highlightFeatures.test.js b/providers/maplibre/src/utils/highlightFeatures.test.js index 4a788599..fd5357b5 100644 --- a/providers/maplibre/src/utils/highlightFeatures.test.js +++ b/providers/maplibre/src/utils/highlightFeatures.test.js @@ -11,11 +11,13 @@ class LngLatBounds { const SYMBOL_IMAGE = 'symbol-abc123' const EMPTY_FILTER = ['==', 'id', ''] -const STALE_SYMBOL_LAYER = 'highlight-stale-symbol' +const LINE_COLOR = 'line-color' +const STALE_SYMBOL_LAYER = 'active-highlight-stale-symbol' const SEL_STALE_SYMBOL_LAYER = 'selected-highlight-stale-symbol' +const EXPECTED_NEW_LAYER_COUNT = 3 const makeMap = (overrides = {}) => ({ - _highlightSources: new Set(), + _activehighlightSources: new Set(), _selectedhighlightSources: new Set(), getLayer: jest.fn(), addLayer: jest.fn(), @@ -28,7 +30,7 @@ const makeMap = (overrides = {}) => ({ ...overrides }) -// ─── Active (cursor) features — highlight-* layers ──────────────────────────── +// ─── Active (cursor) features — active-highlight-* layers ──────────────────── describe('Highlighting Utils — active (cursor) fill and line', () => { let map @@ -43,7 +45,7 @@ describe('Highlighting Utils — active (cursor) fill and line', () => { const ALL_BRANCHES_STYLES = { l1: { stroke: 'red', fill: 'blue' }, l2: { stroke: 'green' } } beforeEach(() => { - map = makeMap({ _highlightSources: new Set(['stale']) }) + map = makeMap({ _activehighlightSources: new Set(['stale']) }) }) test('All branches', () => { @@ -53,36 +55,55 @@ describe('Highlighting Utils — active (cursor) fill and line', () => { if (id.includes('stale')) { return {} } if (id === 'l1') { return { source: 's1', type: 'fill' } } if (id === 'l2') { return { source: 's2', type: 'line' } } - if (id === 'highlight-s2-fill') { return {} } + if (id === 'active-highlight-s2-fill') { return {} } return null }) updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: ALL_BRANCHES_FEATURES, stylesMap: ALL_BRANCHES_STYLES }) - expect(map.setFilter).toHaveBeenCalledWith('highlight-stale-fill', EMPTY_FILTER) + expect(map.setFilter).toHaveBeenCalledWith('active-highlight-stale-fill', EMPTY_FILTER) expect(map.setFilter).toHaveBeenCalledWith(STALE_SYMBOL_LAYER, EMPTY_FILTER) - expect(map.setFilter).toHaveBeenCalledWith('highlight-s2-fill', EMPTY_FILTER) - expect(map.setFilter).toHaveBeenCalledWith('highlight-s2-line', expect.arrayContaining([['get', 'customId']])) + expect(map.setFilter).toHaveBeenCalledWith('active-highlight-s2-fill', EMPTY_FILTER) + expect(map.setFilter).toHaveBeenCalledWith('active-highlight-s2-line', expect.arrayContaining([['get', 'customId']])) }) - test('null _highlightSources falls back to empty set; line geom skips absent fill layer', () => { - map._highlightSources = null + test('null _activehighlightSources falls back to empty set; line geom skips absent fill layer', () => { + map._activehighlightSources = null map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) - expect(map.setFilter).not.toHaveBeenCalledWith('highlight-s1-fill', expect.anything()) + expect(map.setFilter).not.toHaveBeenCalledWith('active-highlight-s1-fill', expect.anything()) }) test('persistent source skips cleanup; missing stale layers skip setFilter', () => { - map._highlightSources = new Set(['stale', 's1']) + map._activehighlightSources = new Set(['stale', 's1']) map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) expect(map.setFilter).not.toHaveBeenCalledWith(expect.stringContaining('stale'), expect.anything()) }) + + test('active features get selected-style overlay line on top', () => { + map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR + const stylesMap = { l1: { stroke: 'yellow', selectionStroke: 'black', strokeWidth: 3, activeStrokeWidth: 9 } } + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap }) + + const activeColorCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'active-highlight-s1-line' && c[1] === LINE_COLOR) + expect(activeColorCall?.[2]).toBe('yellow') + + const overlayColorCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'active-highlight-inner-s1-line' && c[1] === LINE_COLOR) + expect(overlayColorCall?.[2]).toBe('black') + }) + + test('overlay cleanup when active features cleared', () => { + map._activehighlightinnerSources = new Set(['stale']) + map.getLayer.mockImplementation(id => id === 'active-highlight-inner-stale-line' ? {} : null) // NOSONAR + updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [], stylesMap: {} }) + expect(map.setFilter).toHaveBeenCalledWith('active-highlight-inner-stale-line', EMPTY_FILTER) + }) }) -// ─── Selected (committed) features — selected-highlight-* layers ───────────── +// ─── Selected features — selected-highlight-* layers ───────────────────────── -describe('Highlighting Utils — selected (committed) fill and line', () => { +describe('Highlighting Utils — selected fill and line', () => { let map const SELECTED_FEATURES = [ @@ -104,7 +125,7 @@ describe('Highlighting Utils — selected (committed) fill and line', () => { updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: SELECTED_FEATURES, stylesMap: STYLES }) expect(map.setFilter).toHaveBeenCalledWith(SEL_STALE_SYMBOL_LAYER, EMPTY_FILTER) expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ id: 'selected-highlight-s1-line' })) - const linePaintCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'selected-highlight-s1-line' && c[1] === 'line-color') + const linePaintCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'selected-highlight-s1-line' && c[1] === LINE_COLOR) expect(linePaintCall).toBeTruthy() expect(linePaintCall[2]).toBe('#ffdd00') }) @@ -132,18 +153,19 @@ describe('Highlighting Utils — layer management', () => { let map beforeEach(() => { - map = makeMap({ _highlightSources: new Set(['stale']) }) + map = makeMap({ _activehighlightSources: new Set(['stale']) }) }) - test('reuses existing highlight layer; new layer spreads sourceLayer', () => { + test('reuses existing active-highlight layer; new layers spread sourceLayer', () => { map.getLayer.mockImplementation(id => { // NOSONAR if (id === 'l1') { return { source: 's1', type: 'line' } } if (id === 'l2') { return { source: 's2', type: 'line', sourceLayer: 'tiles' } } - if (id === 'highlight-s1-line') { return {} } + if (id === 'active-highlight-s1-line') { return {} } return null }) updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }, { featureId: 2, layerId: 'l2' }], stylesMap: { l1: { stroke: 'blue' }, l2: { stroke: 'green' } } }) - expect(map.addLayer).toHaveBeenCalledTimes(1) + // active-highlight-s1-line reused; active-highlight-s2-line, active-highlight-inner-s1-line, active-highlight-inner-s2-line are new + expect(map.addLayer).toHaveBeenCalledTimes(3) expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ 'source-layer': 'tiles' })) }) @@ -156,7 +178,7 @@ describe('Highlighting Utils — layer management', () => { describe('Highlighting Utils — symbol layers (active cursor)', () => { const ACTIVE_IMAGE = 'symbol-act-abc123' - const HIGHLIGHT_LAYER = 'highlight-s1-symbol' + const HIGHLIGHT_LAYER = 'active-highlight-s1-symbol' const ICON_IMAGE = 'icon-image' const ICON_ANCHOR = 'icon-anchor' const POINT_FEATURE = { featureId: 1, layerId: 'l1', geometry: { type: 'Point' } } @@ -228,7 +250,7 @@ describe('Highlighting Utils — symbol layers (active cursor)', () => { }) test('cleans up stale symbol highlight layer', () => { - map._highlightSources = new Set(['stale']) + map._activehighlightSources = new Set(['stale']) map.getLayer.mockImplementation(id => id === STALE_SYMBOL_LAYER ? { type: 'symbol' } : null) // NOSONAR run([]) expect(map.setFilter).toHaveBeenCalledWith(STALE_SYMBOL_LAYER, EMPTY_FILTER) From 459651566f3143bc0b85f3024d43cbc22592fa43 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Wed, 29 Apr 2026 20:17:17 +0100 Subject: [PATCH 05/10] Test fixes --- .../datasets/src/components/svgProperties.js | 2 +- plugins/interact/src/hooks/useMapItemList.js | 5 +- .../interact/src/hooks/useMapItemList.test.js | 84 +++++++++++++++++-- plugins/interact/src/reducer.test.js | 20 ++++- .../maplibre/src/maplibreProvider.test.js | 22 ++++- .../src/utils/highlightFeatures.test.js | 33 +++++--- .../maplibre/src/utils/symbolImages.test.js | 62 ++++++++------ src/App/components/Markers/Markers.test.jsx | 13 ++- src/services/symbolRegistry.test.js | 17 ++++ 9 files changed, 206 insertions(+), 52 deletions(-) diff --git a/plugins/beta/datasets/src/components/svgProperties.js b/plugins/beta/datasets/src/components/svgProperties.js index 54e4555b..c68dd3f1 100644 --- a/plugins/beta/datasets/src/components/svgProperties.js +++ b/plugins/beta/datasets/src/components/svgProperties.js @@ -1,6 +1,6 @@ export const SVG_SIZE = 20 // Width and height attributes of the svg element export const SVG_CENTER = SVG_SIZE / 2 -const SVG_SYMBOL_SIZE = 44 // Width and height attributes of the svg element if its a marker or point feature symbol +const SVG_SYMBOL_SIZE = 44 // Width and height attributes of the svg element if its a marker or point feature symbol export const svgProps = { xmlns: 'http://www.w3.org/2000/svg', diff --git a/plugins/interact/src/hooks/useMapItemList.js b/plugins/interact/src/hooks/useMapItemList.js index 6cca1a33..7f2fb113 100644 --- a/plugins/interact/src/hooks/useMapItemList.js +++ b/plugins/interact/src/hooks/useMapItemList.js @@ -40,10 +40,7 @@ const toFeatureItem = (feature, layerConfigMap, seenIds) => { return null } seenIds.add(stringId) - const layerLabel = config.label || config.layerId - const label = config.labelProperty - ? (feature.properties?.[config.labelProperty] ?? stringId) - : `${layerLabel} ${stringId}` + const label = feature.properties?.[config.labelProperty] ?? stringId return { id: stringId, label } } diff --git a/plugins/interact/src/hooks/useMapItemList.test.js b/plugins/interact/src/hooks/useMapItemList.test.js index 281faa37..01a3d27d 100644 --- a/plugins/interact/src/hooks/useMapItemList.test.js +++ b/plugins/interact/src/hooks/useMapItemList.test.js @@ -123,6 +123,19 @@ describe('useMapItemList — selectMarker mode', () => { container.remove() }) + it('excludes markers whose element has no .im-c-viewport__markers ancestor', () => { + const el = document.createElement('div') + document.body.appendChild(el) + const markers = makeMarkers([{ id: 'm1', label: 'Orphan', symbol: 'pin', isVisible: true }]) + markers.markerRefs.set('m1', el) + + const { eb } = setup({ interactionModes: ['selectMarker'], markers }) + act(() => eb.emit(MOVE_END, {})) + + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) + el.remove() + }) + it('excludes markers outside the viewport', () => { const { el, container } = makeMarkerEl({ inViewport: false }) const markers = makeMarkers([{ id: 'm1', label: 'Offscreen', symbol: 'pin', isVisible: true }]) @@ -205,16 +218,45 @@ describe('useMapItemList — selectFeature mode: label resolution', () => { }) }) - it('excludes features from layers with a label but no labelProperty', () => { - const features = [{ layer: { id: 'roads' }, properties: { road_id: '4' } }] + it('uses feature.id when idProperty is not present in feature properties', () => { + const features = [{ layer: { id: 'roads' }, id: 42, properties: { road_name: 'Oak Ave' } }] + const { eb } = setup({ interactionModes: ['selectFeature'], layers, mapProvider: makeMapProvider(features) }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: '42', label: 'Oak Ave' }], multiselectable: false + }) + }) + + it('skips feature when both idProperty and feature.id are absent', () => { + const features = [{ layer: { id: 'roads' }, properties: { road_name: 'Lost Lane' } }] + const { eb } = setup({ interactionModes: ['selectFeature'], layers, mapProvider: makeMapProvider(features) }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { items: [], multiselectable: false }) + }) + + it('falls back to stringId label when feature has no properties object', () => { + // Line 45: feature.properties is undefined → ?. short-circuits → ?? stringId used + const features = [{ layer: { id: 'roads' }, id: 99 }] + const { eb } = setup({ interactionModes: ['selectFeature'], layers, mapProvider: makeMapProvider(features) }) + act(() => eb.emit(MOVE_END, {})) + expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { + items: [{ id: '99', label: '99' }], multiselectable: false + }) + }) + + it('deduplicates features with the same id', () => { + const features = [ + { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'High Street' } }, + { layer: { id: 'roads' }, properties: { road_id: '1', road_name: 'Duplicate' } } + ] const { eb } = setup({ interactionModes: ['selectFeature'], - layers: [{ layerId: 'roads', label: 'Roads', idProperty: 'road_id' }], + layers: [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }], mapProvider: makeMapProvider(features) }) act(() => eb.emit(MOVE_END, {})) expect(eb.emit).toHaveBeenCalledWith(SET_FEATURES, { - items: [], multiselectable: false + items: [{ id: '1', label: 'High Street' }], multiselectable: false }) }) }) @@ -366,6 +408,38 @@ describe('useMapItemList — map:setactivefeature listener', () => { act(() => eb.emit(SET_ACTIVE, { id: 'missing' })) // NOSONAR expect(dp).not.toHaveBeenCalled() }) + + it('skips features from unknown layers when searching by id', () => { + // Line 69: getFeatureId returns null for unknown layer → rawId != null is false + const layers = [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }] + const features = [{ layer: { id: 'unknown' }, properties: { some_id: '1' }, geometry: { type: 'Point' } }] + const { eb, dp } = setup({ + interactionModes: ['selectFeature'], + layers, + mapProvider: makeMapProvider(features) + }) + act(() => eb.emit(SET_ACTIVE, { id: '1' })) + expect(dp).not.toHaveBeenCalled() + }) + + it('does not search features when interactionModes excludes selectFeature', () => { + // Line 114: interactionModes.includes('selectFeature') → false → block skipped + const features = [{ layer: { id: 'roads' }, properties: { road_id: '1' }, geometry: { type: 'Point' } }] + const { eb, dp } = setup({ + interactionModes: ['selectMarker'], + layers: [{ layerId: 'roads', idProperty: 'road_id', labelProperty: 'road_name' }], + mapProvider: makeMapProvider(features) + }) + act(() => eb.emit(SET_ACTIVE, { id: '1' })) + expect(dp).not.toHaveBeenCalled() + }) + + it('does not search features when layers is empty', () => { + // Line 114: layers.length > 0 → false → block skipped + const { eb, dp } = setup({ interactionModes: ['selectFeature'], layers: [] }) + act(() => eb.emit(SET_ACTIVE, { id: '1' })) + expect(dp).not.toHaveBeenCalled() + }) }) // ─── useMapItemList — confirm: lifecycle and guards ────────────────────── @@ -391,7 +465,7 @@ describe('useMapItemList — confirm: lifecycle and guards', () => { act(() => eb.emit(CONFIRM)) dp.mockClear() act(() => eb.emit(CONFIRM)) - expect(dp).toHaveBeenCalledWith({ type: 'SELECT_MARKER', payload: { markerId: 'm1', multiSelect: false } }) + expect(dp).toHaveBeenCalledWith({ type: 'TOGGLE_SELECTED_MARKERS', payload: { markerId: 'm1', multiSelect: false } }) }) }) diff --git a/plugins/interact/src/reducer.test.js b/plugins/interact/src/reducer.test.js index 147afc7b..00c547be 100644 --- a/plugins/interact/src/reducer.test.js +++ b/plugins/interact/src/reducer.test.js @@ -12,7 +12,8 @@ describe('initialState', () => { selectedFeatures: [], selectedMarkers: [], selectionBounds: null, - closeOnAction: true + closeOnAction: true, + listboxActiveItem: null }) }) }) @@ -188,6 +189,20 @@ describe('CLEAR_SELECTED_FEATURES action', () => { }) }) +describe('SET_LISTBOX_ACTIVE action', () => { + it('sets listboxActiveItem to the payload value', () => { + const payload = { featureId: 'f1', layerId: 'l1' } + const result = actions.SET_LISTBOX_ACTIVE(initialState, payload) + expect(result.listboxActiveItem).toEqual(payload) + }) + + it('sets listboxActiveItem to null when payload is null', () => { + const state = { ...initialState, listboxActiveItem: { featureId: 'f1' } } + const result = actions.SET_LISTBOX_ACTIVE(state, null) + expect(result.listboxActiveItem).toBeNull() + }) +}) + describe('SELECT_MARKER action', () => { it('adds a marker in single-select mode, clearing selectedFeatures', () => { const state = { ...initialState, selectedFeatures: [{ featureId: 'f1' }], selectedMarkers: [] } @@ -235,7 +250,8 @@ describe('actions object', () => { 'UPDATE_SELECTED_BOUNDS', 'CLEAR_SELECTED_FEATURES', 'SELECT_MARKER', - 'UNSELECT_MARKER' + 'UNSELECT_MARKER', + 'SET_LISTBOX_ACTIVE' ]) Object.values(actions).forEach(fn => expect(typeof fn).toBe('function')) }) diff --git a/providers/maplibre/src/maplibreProvider.test.js b/providers/maplibre/src/maplibreProvider.test.js index 6ec52f01..9e06f8ce 100644 --- a/providers/maplibre/src/maplibreProvider.test.js +++ b/providers/maplibre/src/maplibreProvider.test.js @@ -244,12 +244,30 @@ describe('MapLibreProvider', () => { await doInitMap(p) p.getFeaturesAtPoint({ x: 10, y: 20 }, { radius: 5 }) expect(queryFeatures).toHaveBeenCalledWith(map, { x: 10, y: 20 }, { radius: 5 }) - p.updateHighlightedFeatures(['feat'], { style: 1 }) + p.updateHighlightedFeatures(['feat'], null, { style: 1 }) expect(updateHighlightedFeatures).toHaveBeenCalledWith({ - LngLatBounds: maplibreModule.LngLatBounds, map, selectedFeatures: ['feat'], stylesMap: { style: 1 } + LngLatBounds: maplibreModule.LngLatBounds, map, selectedFeatures: ['feat'], activeFeatures: null, stylesMap: { style: 1 } }) }) + test('getVisibleFeatures returns empty array when no layers are present on the map', async () => { + const p = makeProvider() + await doInitMap(p) + map.getLayer.mockReturnValue(null) + expect(p.getVisibleFeatures(['l1', 'l2'])).toEqual([]) + expect(map.queryRenderedFeatures).not.toHaveBeenCalled() + }) + + test('getVisibleFeatures queries only layers present on the map', async () => { + const p = makeProvider() + await doInitMap(p) + map.getLayer.mockImplementation(id => id === 'l1' ? {} : null) // NOSONAR + const features = [{ id: 'f1' }] + map.queryRenderedFeatures.mockReturnValue(features) + expect(p.getVisibleFeatures(['l1', 'l2'])).toEqual(features) + expect(map.queryRenderedFeatures).toHaveBeenCalledWith(undefined, { layers: ['l1'] }) + }) + test('addSymbolsToMap delegates to utility with map instance', async () => { const p = makeProvider() await doInitMap(p) diff --git a/providers/maplibre/src/utils/highlightFeatures.test.js b/providers/maplibre/src/utils/highlightFeatures.test.js index fd5357b5..dbda1301 100644 --- a/providers/maplibre/src/utils/highlightFeatures.test.js +++ b/providers/maplibre/src/utils/highlightFeatures.test.js @@ -14,7 +14,6 @@ const EMPTY_FILTER = ['==', 'id', ''] const LINE_COLOR = 'line-color' const STALE_SYMBOL_LAYER = 'active-highlight-stale-symbol' const SEL_STALE_SYMBOL_LAYER = 'selected-highlight-stale-symbol' -const EXPECTED_NEW_LAYER_COUNT = 3 const makeMap = (overrides = {}) => ({ _activehighlightSources: new Set(), @@ -59,7 +58,7 @@ describe('Highlighting Utils — active (cursor) fill and line', () => { return null }) - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: ALL_BRANCHES_FEATURES, stylesMap: ALL_BRANCHES_STYLES }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures: ALL_BRANCHES_FEATURES, stylesMap: ALL_BRANCHES_STYLES }) expect(map.setFilter).toHaveBeenCalledWith('active-highlight-stale-fill', EMPTY_FILTER) expect(map.setFilter).toHaveBeenCalledWith(STALE_SYMBOL_LAYER, EMPTY_FILTER) @@ -70,21 +69,21 @@ describe('Highlighting Utils — active (cursor) fill and line', () => { test('null _activehighlightSources falls back to empty set; line geom skips absent fill layer', () => { map._activehighlightSources = null map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) expect(map.setFilter).not.toHaveBeenCalledWith('active-highlight-s1-fill', expect.anything()) }) test('persistent source skips cleanup; missing stale layers skip setFilter', () => { map._activehighlightSources = new Set(['stale', 's1']) map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap: { l1: { stroke: 'red' } } }) expect(map.setFilter).not.toHaveBeenCalledWith(expect.stringContaining('stale'), expect.anything()) }) test('active features get selected-style overlay line on top', () => { map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'line' } : null) // NOSONAR const stylesMap = { l1: { stroke: 'yellow', selectionStroke: 'black', strokeWidth: 3, activeStrokeWidth: 9 } } - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }], stylesMap }) const activeColorCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'active-highlight-s1-line' && c[1] === LINE_COLOR) expect(activeColorCall?.[2]).toBe('yellow') @@ -96,7 +95,7 @@ describe('Highlighting Utils — active (cursor) fill and line', () => { test('overlay cleanup when active features cleared', () => { map._activehighlightinnerSources = new Set(['stale']) map.getLayer.mockImplementation(id => id === 'active-highlight-inner-stale-line' ? {} : null) // NOSONAR - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [], stylesMap: {} }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures: [], stylesMap: {} }) expect(map.setFilter).toHaveBeenCalledWith('active-highlight-inner-stale-line', EMPTY_FILTER) }) }) @@ -122,7 +121,7 @@ describe('Highlighting Utils — selected fill and line', () => { if (id === 'l1') { return { source: 's1', type: 'fill' } } return null }) - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: SELECTED_FEATURES, stylesMap: STYLES }) + updateHighlightedFeatures({ LngLatBounds, map, selectedFeatures: SELECTED_FEATURES, stylesMap: STYLES }) expect(map.setFilter).toHaveBeenCalledWith(SEL_STALE_SYMBOL_LAYER, EMPTY_FILTER) expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ id: 'selected-highlight-s1-line' })) const linePaintCall = map.setPaintProperty.mock.calls.find(c => c[0] === 'selected-highlight-s1-line' && c[1] === LINE_COLOR) @@ -130,6 +129,16 @@ describe('Highlighting Utils — selected fill and line', () => { expect(linePaintCall[2]).toBe('#ffdd00') }) + test('bounds matched using idProperty when set', () => { + const features = [{ featureId: 'parcel-1', layerId: 'l1', idProperty: 'parcelId', geometry: { type: 'Polygon' } }] + map.getLayer.mockImplementation(id => id === 'l1' ? { source: 's1', type: 'fill' } : null) // NOSONAR + map.queryRenderedFeatures.mockReturnValue([ + { properties: { parcelId: 'parcel-1' }, geometry: { coordinates: [[1, 2]] } } + ]) + const bounds = updateHighlightedFeatures({ LngLatBounds, map, selectedFeatures: features, stylesMap: STYLES }) + expect(bounds).not.toBeNull() + }) + test('bounds are calculated from selected features only', () => { const features = [ { featureId: 1, layerId: 'l1', geometry: { type: 'Polygon' } }, @@ -142,7 +151,7 @@ describe('Highlighting Utils — selected fill and line', () => { { id: 1, geometry: { coordinates: [coordMax, coordMax] } }, { id: 2, geometry: { coordinates: [[0, 0], [coordMid, coordMid]] } } ]) - const bounds = updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: features, stylesMap: STYLES }) + const bounds = updateHighlightedFeatures({ LngLatBounds, map, selectedFeatures: features, stylesMap: STYLES }) expect(bounds).toEqual([0, 0, 10, 10]) }) }) @@ -163,14 +172,14 @@ describe('Highlighting Utils — layer management', () => { if (id === 'active-highlight-s1-line') { return {} } return null }) - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }, { featureId: 2, layerId: 'l2' }], stylesMap: { l1: { stroke: 'blue' }, l2: { stroke: 'green' } } }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures: [{ featureId: 1, layerId: 'l1' }, { featureId: 2, layerId: 'l2' }], stylesMap: { l1: { stroke: 'blue' }, l2: { stroke: 'green' } } }) // active-highlight-s1-line reused; active-highlight-s2-line, active-highlight-inner-s1-line, active-highlight-inner-s2-line are new expect(map.addLayer).toHaveBeenCalledTimes(3) expect(map.addLayer).toHaveBeenCalledWith(expect.objectContaining({ 'source-layer': 'tiles' })) }) test('returns null when no rendered features match', () => { - expect(updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures: [], stylesMap: {} })).toBeNull() + expect(updateHighlightedFeatures({ LngLatBounds, map, selectedFeatures: [], stylesMap: {} })).toBeNull() }) }) @@ -193,7 +202,7 @@ describe('Highlighting Utils — symbol layers (active cursor)', () => { }) const run = (activeFeatures = [POINT_FEATURE]) => - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, activeFeatures, stylesMap: { l1: {} } }) + updateHighlightedFeatures({ LngLatBounds, map, activeFeatures, stylesMap: { l1: {} } }) test('creates symbol highlight layer with cursor image variant', () => { run() @@ -273,7 +282,7 @@ describe('Highlighting Utils — symbol layers (committed selection)', () => { }) const run = (selectedFeatures = [POINT_FEATURE]) => - updateHighlightedFeatures({ LngLatBounds: LngLatBounds, map, selectedFeatures, stylesMap: { l1: {} } }) + updateHighlightedFeatures({ LngLatBounds, map, selectedFeatures, stylesMap: { l1: {} } }) test('creates selected-highlight symbol layer with selected image variant', () => { run() diff --git a/providers/maplibre/src/utils/symbolImages.test.js b/providers/maplibre/src/utils/symbolImages.test.js index 8e64eddc..c3da60e4 100644 --- a/providers/maplibre/src/utils/symbolImages.test.js +++ b/providers/maplibre/src/utils/symbolImages.test.js @@ -104,16 +104,16 @@ describe('getSymbolImageId', () => { expect(id).toMatch(/^symbol-[a-z0-9]+-\d+(\.\d+)?x$/) }) - it('returns a string prefixed symbol-sel- for selected state', () => { + it('returns a string prefixed symbol-act- for active state', () => { const id = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, true) expect(typeof id).toBe('string') - expect(id).toMatch(/^symbol-sel-[a-z0-9]+-\d+(\.\d+)?x$/) + expect(id).toMatch(/^symbol-act-[a-z0-9]+-\d+(\.\d+)?x$/) }) - it('normal and selected ids differ for the same dataset', () => { + it('normal and active ids differ for the same dataset', () => { const normalId = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, false) - const selectedId = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, true) - expect(normalId).not.toBe(selectedId) + const activeId = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, true) + expect(normalId).not.toBe(activeId) }) it('same dataset and style always produces the same id', () => { @@ -148,7 +148,8 @@ describe('getSymbolImageId', () => { // ─── addSymbolsToMap ────────────────────────────────────────────────────────── const makeMap = (existingIds = []) => ({ - _symbolImageMap: {}, + _activeSymbolImageMap: {}, + _selectedSymbolImageMap: {}, hasImage: jest.fn((id) => existingIds.includes(id)), addImage: jest.fn() }) @@ -161,33 +162,43 @@ describe('addSymbolsToMap — registration', () => { expect(map.addImage).not.toHaveBeenCalled() }) - it('resets _symbolImageMap before processing', async () => { + it('resets _activeSymbolImageMap and _selectedSymbolImageMap before processing', async () => { const map = makeMap() - map._symbolImageMap = { stale: 'entry' } + map._activeSymbolImageMap = { stale: 'entry' } + map._selectedSymbolImageMap = { stale: 'entry' } await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry) - expect(map._symbolImageMap).not.toHaveProperty('stale') + expect(map._activeSymbolImageMap).not.toHaveProperty('stale') + expect(map._selectedSymbolImageMap).not.toHaveProperty('stale') }) - it('calls addImage for normal and selected variants', async () => { + it('calls addImage for normal, active and selected variants', async () => { const map = makeMap() await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry) - expect(map.addImage).toHaveBeenCalledTimes(2) + expect(map.addImage).toHaveBeenCalledTimes(3) // NOSONAR S109 — normal, active, selected expect(map.addImage).toHaveBeenCalledWith(expect.stringMatching(/^symbol-[a-z0-9]+-\d+(\.\d+)?x$/), expect.any(Object), { pixelRatio: 2 }) + expect(map.addImage).toHaveBeenCalledWith(expect.stringMatching(/^symbol-act-[a-z0-9]+-\d+(\.\d+)?x$/), expect.any(Object), { pixelRatio: 2 }) expect(map.addImage).toHaveBeenCalledWith(expect.stringMatching(/^symbol-sel-[a-z0-9]+-\d+(\.\d+)?x$/), expect.any(Object), { pixelRatio: 2 }) }) - it('populates _symbolImageMap with normal → selected id pairs', async () => { + it('populates _activeSymbolImageMap and _selectedSymbolImageMap with normal → variant id pairs', async () => { const map = makeMap() await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry) const normalId = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, false) - const selectedId = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, true) - expect(map._symbolImageMap[normalId]).toBe(selectedId) + const activeId = getSymbolImageId({ symbol: 'pin' }, mapStyle, symbolRegistry, true) + const selectedId = map._selectedSymbolImageMap[normalId] + expect(map._activeSymbolImageMap[normalId]).toBe(activeId) + expect(selectedId).toMatch(/^symbol-sel-[a-z0-9]+-\d+(\.\d+)?x$/) }) - it('skips addImage when image is already registered', async () => { + it('skips addImage when all three variant images are already registered', async () => { + // Run once to discover the selected image ID (not derivable without rasterising) + const setupMap = makeMap() + await addSymbolsToMap(setupMap, [{ symbol: 'circle' }], mapStyle, symbolRegistry) const normalId = getSymbolImageId({ symbol: 'circle' }, mapStyle, symbolRegistry, false) - const selectedId = getSymbolImageId({ symbol: 'circle' }, mapStyle, symbolRegistry, true) - const map = makeMap([normalId, selectedId]) + const activeId = getSymbolImageId({ symbol: 'circle' }, mapStyle, symbolRegistry, true) + const selectedId = setupMap._selectedSymbolImageMap[normalId] + + const map = makeMap([normalId, activeId, selectedId]) await addSymbolsToMap(map, [{ symbol: 'circle' }], mapStyle, symbolRegistry) expect(map.addImage).not.toHaveBeenCalled() }) @@ -195,23 +206,25 @@ describe('addSymbolsToMap — registration', () => { it('processes multiple configs independently', async () => { const map = makeMap() await addSymbolsToMap(map, [{ symbol: 'pin' }, { symbol: 'circle' }], mapStyle, symbolRegistry) - expect(map.addImage).toHaveBeenCalledTimes(4) - expect(Object.keys(map._symbolImageMap)).toHaveLength(2) + expect(map.addImage).toHaveBeenCalledTimes(6) // NOSONAR S109 — 2 configs × 3 variants each + expect(Object.keys(map._activeSymbolImageMap)).toHaveLength(2) + expect(Object.keys(map._selectedSymbolImageMap)).toHaveLength(2) }) }) describe('addSymbolsToMap — null results and caching', () => { it('does not call addImage when rasteriseSymbolImage returns null', async () => { - // getSymbolImageId (called twice — normal + selected) needs a real symbolDef to produce imageIds, + // getSymbolImageId (called twice — normal + active) needs a real symbolDef to produce imageIds, // but rasteriseSymbolImage must get undefined from getSymbolDef so it returns null. - // The registry.get call order: [1] getSymbolImageId normal, [2] getSymbolImageId selected, - // [3] rasteriseSymbolImage normal, [4] rasteriseSymbolImage selected. + // The registry.get call order: [1] getSymbolImageId normal, [2] getSymbolImageId active, + // [3] rasteriseSymbolImage normal, [4] rasteriseSymbolImage active, [5] rasteriseSymbolImage selected. const pinDef = symbolRegistry.get('pin') const getSpy = jest.spyOn(symbolRegistry, 'get') .mockReturnValueOnce(pinDef) .mockReturnValueOnce(pinDef) .mockReturnValueOnce(undefined) .mockReturnValueOnce(undefined) + .mockReturnValueOnce(undefined) const map = makeMap() await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry) expect(map.addImage).not.toHaveBeenCalled() @@ -222,7 +235,8 @@ describe('addSymbolsToMap — null results and caching', () => { const map = makeMap() await addSymbolsToMap(map, [{ symbol: 'no-such-symbol' }], mapStyle, symbolRegistry) expect(map.addImage).not.toHaveBeenCalled() - expect(map._symbolImageMap).toEqual({}) + expect(map._activeSymbolImageMap).toEqual({}) + expect(map._selectedSymbolImageMap).toEqual({}) }) it('reuses cached imageData when called again with the same pixelRatio', async () => { @@ -243,6 +257,6 @@ describe('addSymbolsToMap — null results and caching', () => { // No new canvas — rasterisation was skipped via cache expect(getContextCallsAfterSecond).toBe(getContextCallsAfterFirst) // addImage still called because map2 has no pre-registered images - expect(map2.addImage).toHaveBeenCalledTimes(2) + expect(map2.addImage).toHaveBeenCalledTimes(3) // NOSONAR S109 — normal, active, selected }) }) diff --git a/src/App/components/Markers/Markers.test.jsx b/src/App/components/Markers/Markers.test.jsx index 244bd3a7..d36257a2 100644 --- a/src/App/components/Markers/Markers.test.jsx +++ b/src/App/components/Markers/Markers.test.jsx @@ -124,12 +124,12 @@ describe('Markers — symbol resolution', () => { expect(svg.getAttribute('height')).toBe('60') }) - it("falls back to '0 0 38 38' viewBox when none is provided", () => { + it("falls back to '0 0 44 44' viewBox when none is provided", () => { const sr = makeSymbolRegistry({ get: jest.fn(() => ({ svg: '' })), getDefaults: jest.fn(() => ({ symbol: 'pin' })) }) - expect(setup({ markers: [makeMarker()], symbolRegistry: sr }).result.container.querySelector(SVG_SEL).getAttribute('viewBox')).toBe('0 0 38 38') + expect(setup({ markers: [makeMarker()], symbolRegistry: sr }).result.container.querySelector(SVG_SEL).getAttribute('viewBox')).toBe('0 0 44 44') }) it.each([ @@ -169,6 +169,15 @@ describe('Markers — selection', () => { expect(sr.resolve).not.toHaveBeenCalledAfter?.(sr.resolveSelected) }) + it('calls resolveActive when the marker is the active listbox item', () => { + const SET_ACTIVE = 'map:setactivefeature' + const sr = makeSymbolRegistry({ resolveActive: jest.fn(() => '') }) + const { eb } = setup({ markers: [makeMarker()], symbolRegistry: sr }) + act(() => eb.emit(SET_ACTIVE, { id: MARKER_ID })) + expect(sr.resolveActive).toHaveBeenCalled() + expect(sr.resolveSelected).not.toHaveBeenCalled() + }) + it('uses resolve (not resolveSelected) for unselected markers', () => { const { sr } = setup({ markers: [makeMarker()] }) expect(sr.resolve).toHaveBeenCalled() diff --git a/src/services/symbolRegistry.test.js b/src/services/symbolRegistry.test.js index 5ee9a444..46d14579 100644 --- a/src/services/symbolRegistry.test.js +++ b/src/services/symbolRegistry.test.js @@ -211,6 +211,23 @@ describe('symbolRegistry — resolveActive (keyboard cursor state)', () => { }) }) +describe('symbolRegistry — resolveSelected (committed selection state)', () => { + const symbolDef = { + id: 'test-selected', + svg: '' + } + + it('returns empty string for null symbolDef', () => { + expect(symbolRegistry.resolveSelected(null, {}, mapStyle)).toBe('') + }) + + it('handles null styleColors — uses cascade defaults', () => { + const resolved = symbolRegistry.resolveSelected(symbolDef, null, mapStyle) + expect(typeof resolved).toBe('string') + expect(resolved.length).toBeGreaterThan(0) + }) +}) + describe('symbolRegistry — graphic token', () => { const graphicDef = { id: 'test-graphic', From e9531aebffaef9ba9e1b9cf5b7b3b5cf3dc834a8 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Wed, 29 Apr 2026 20:58:27 +0100 Subject: [PATCH 06/10] Edge case additions --- docs/api/symbol-registry.md | 3 ++- docs/plugins/interact.md | 2 ++ plugins/interact/src/InteractInit.jsx | 3 +-- plugins/interact/src/hooks/useHighlightSync.js | 2 +- plugins/interact/src/hooks/useHighlightSync.test.js | 8 -------- plugins/interact/src/reducer.js | 3 ++- src/types.js | 2 +- 7 files changed, 9 insertions(+), 14 deletions(-) diff --git a/docs/api/symbol-registry.md b/docs/api/symbol-registry.md index d7cbed3b..1818f2bc 100644 --- a/docs/api/symbol-registry.md +++ b/docs/api/symbol-registry.md @@ -6,12 +6,13 @@ The symbol registry is a service that manages reusable named symbols for map mar ## Built-in symbols -Two symbols are registered by default: +Three symbols are registered by default: | ID | Anchor | Description | |----|--------|-------------| | `'pin'` | `[0.5, 1]` | Teardrop pin — tip aligns with the coordinate | | `'circle'` | `[0.5, 0.5]` | Filled circle — centre aligns with the coordinate | +| `'square'` | `[0.5, 0.5]` | Rounded square — centre aligns with the coordinate | Both use the standard `{{token}}` placeholders and respect the resolution order described in [Symbol Config](./symbol-config.md#how-values-are-resolved). diff --git a/docs/plugins/interact.md b/docs/plugins/interact.md index 8e96921e..2a633c89 100644 --- a/docs/plugins/interact.md +++ b/docs/plugins/interact.md @@ -85,9 +85,11 @@ layers: [ |----------|------|-------------| | `layerId` | `string` | **Required.** The map layer identifier to enable selection on | | `idProperty` | `string` | Property name used as the feature's unique identifier. If omitted, features are matched by index | +| `labelProperty` | `string` | Property name used as the feature's display label in the keyboard-accessible listbox. **Required for features to appear in keyboard navigation** | | `selectedStroke` | `string` | Overrides the selection stroke colour for this layer. Defaults to `MapStyleConfig.selectedColor` | | `selectedFill` | `string` | Overrides the selection fill colour for this layer. Defaults to `transparent` | | `selectedStrokeWidth` | `number` | Overrides the selection stroke width for this layer. Defaults to `3` | +| `activeStroke` | `string` | Overrides the keyboard cursor ring colour for this layer. Defaults to `MapStyleConfig.activeColor` | #### Finding layer IDs diff --git a/plugins/interact/src/InteractInit.jsx b/plugins/interact/src/InteractInit.jsx index 304d5296..a9ef9bbc 100755 --- a/plugins/interact/src/InteractInit.jsx +++ b/plugins/interact/src/InteractInit.jsx @@ -16,7 +16,7 @@ export const InteractInit = ({ pluginState }) => { const { interfaceType } = appState - const { dispatch, enabled, selectedFeatures, selectionBounds, interactionModes, layers } = pluginState + const { dispatch, enabled, selectedFeatures, interactionModes, layers } = pluginState const { eventBus, closeApp } = services const { crossHair, mapStyle } = mapState @@ -61,7 +61,6 @@ export const InteractInit = ({ mapStyle, pluginState, selectedFeatures, - selectionBounds, dispatch, events: EVENTS, eventBus diff --git a/plugins/interact/src/hooks/useHighlightSync.js b/plugins/interact/src/hooks/useHighlightSync.js index a86ce0a4..f529cf24 100755 --- a/plugins/interact/src/hooks/useHighlightSync.js +++ b/plugins/interact/src/hooks/useHighlightSync.js @@ -34,7 +34,7 @@ export const useHighlightSync = ({ } useEffect(() => { - if (!mapProvider || !selectedFeatures || !stylesMap) { + if (!mapProvider || !stylesMap) { return undefined // Explicit return to match the cleanup function return below } diff --git a/plugins/interact/src/hooks/useHighlightSync.test.js b/plugins/interact/src/hooks/useHighlightSync.test.js index 90f2653e..9570030b 100644 --- a/plugins/interact/src/hooks/useHighlightSync.test.js +++ b/plugins/interact/src/hooks/useHighlightSync.test.js @@ -140,14 +140,6 @@ describe('useHighlightSync — guards', () => { expect(mockDeps.dispatch).not.toHaveBeenCalled() }) - it('does nothing when selectedFeatures is null', () => { - mockDeps.selectedFeatures = null - - render() - - expect(mockDeps.mapProvider.updateHighlightedFeatures).not.toHaveBeenCalled() - }) - it('does nothing when mapStyle is null', () => { mockDeps.mapStyle = null mockDeps.selectedFeatures = [{ featureId: 'F1' }] diff --git a/plugins/interact/src/reducer.js b/plugins/interact/src/reducer.js index ae921025..a17c515e 100755 --- a/plugins/interact/src/reducer.js +++ b/plugins/interact/src/reducer.js @@ -26,7 +26,8 @@ const disable = (state) => { enabled: false, selectedFeatures: [], selectedMarkers: [], - selectionBounds: null + selectionBounds: null, + listboxActiveItem: null } } diff --git a/src/types.js b/src/types.js index 7eff076e..a71ebbd1 100644 --- a/src/types.js +++ b/src/types.js @@ -284,7 +284,7 @@ * @property {(point: { x: number, y: number }) => [number, number]} screenToMap * Convert screen pixel position (x from left edge, y from top edge of viewport) to map coordinates ([lng, lat] or [easting, northing] depending on the crs of the map provider). * - * @property {(selectedFeatures: any[], stylesMap: any) => any} [updateHighlightedFeatures] + * @property {(selectedFeatures: any[], activeFeatures: any[], stylesMap: any) => any} [updateHighlightedFeatures] * @experimental Update highlighted features on the map. * * @property {(direction: string) => any} [highlightNextLabel] From 28bca7ebc862a67080eda6f20c52241965e386ab Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Thu, 30 Apr 2026 10:09:36 +0100 Subject: [PATCH 07/10] active item priority fix --- demo/js/index.js | 18 ++-- src/App/hooks/useFeatureFocus.js | 62 +++++++++++-- src/App/hooks/useFeatureFocus.test.js | 126 +++++++++++++++++++++++++- 3 files changed, 187 insertions(+), 19 deletions(-) diff --git a/demo/js/index.js b/demo/js/index.js index 8645a9d2..14445faa 100755 --- a/demo/js/index.js +++ b/demo/js/index.js @@ -31,32 +31,32 @@ const pointData = {type: 'FeatureCollection',features: [{type: 'Feature',propert const interactPlugin = createInteractPlugin({ layers: [{ layerId: 'historic-monuments-prehistoric-symbol', - labelProperty: 'name' + // labelProperty: 'name' // idProperty: 'gid' },{ layerId: 'land-covers-110', - labelProperty: 'gid' + // labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-130-131', - labelProperty: 'gid' + // labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-332', - labelProperty: 'gid' + // labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-379', - labelProperty: 'gid' + // labelProperty: 'gid' // idProperty: 'gid' },{ layerId: 'land-covers-other', - labelProperty: 'gid' + // labelProperty: 'gid' // idProperty: 'gid' }, { layerId: 'hedge-control', - idProperty: 'id' + // idProperty: 'id' }, // { // layerId: 'OS/TopographicArea_1/Agricultural Land', @@ -64,10 +64,10 @@ const interactPlugin = createInteractPlugin({ // }, { layerId: 'fill-inactive.cold', - idProperty: 'id' + // idProperty: 'id' },{ layerId: 'stroke-inactive.cold', - idProperty: 'id' + // idProperty: 'id' } ], debug: true, diff --git a/src/App/hooks/useFeatureFocus.js b/src/App/hooks/useFeatureFocus.js index 9daeb362..569b2970 100644 --- a/src/App/hooks/useFeatureFocus.js +++ b/src/App/hooks/useFeatureFocus.js @@ -12,20 +12,40 @@ const getNavigatedId = (id, key, items) => { return idx === -1 ? items[items.length - 1].id : items[Math.max(idx - 1, 0)].id } +// ARIA listbox entry priority: first selected → last active (if still in list) → first item +const resolveEntryId = (items, lastActiveId, selectedIds) => { + const firstSelected = items.find(item => selectedIds.includes(item.id)) + if (firstSelected) { + return firstSelected.id + } + if (lastActiveId && items.some(item => item.id === lastActiveId)) { + return lastActiveId + } + return items[0].id +} + export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBus }) { const [activeFeatureId, setActiveFeatureId] = useState(null) const [selectedIds, setSelectedIds] = useState([]) - const activeFeatureIdRef = useRef(null) - useEffect(() => { - activeFeatureIdRef.current = activeFeatureId - }, [activeFeatureId]) + const isFocusedRef = useRef(false) + const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus + const activeFeatureIdRef = useRef(null) // always-current for keydown closure + const selectedIdsRef = useRef([]) // always-current for items-change effect + + useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) + useEffect(() => { selectedIdsRef.current = selectedIds }, [selectedIds]) useEffect(() => { if (!eventBus) { return undefined } - const handleSetActive = ({ id }) => { setActiveFeatureId(id) } + const handleSetActive = ({ id }) => { + if (id !== null) { + lastActiveIdRef.current = id + } + setActiveFeatureId(id) + } const handleSelectionChange = ({ selectedFeatures = [], selectedMarkers = [] }) => { setSelectedIds([...selectedFeatures.map(f => String(f.featureId)), ...selectedMarkers]) } @@ -37,6 +57,25 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu } }, [eventBus]) + // When items change while focused, revalidate: keep current if still present, else re-pick + useEffect(() => { + if (!isFocusedRef.current) { + return + } + if (!items.length) { + setActiveFeatureId(null) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: null }) + return + } + if (items.some(item => item.id === activeFeatureIdRef.current)) { + return + } + const nextId = resolveEntryId(items, lastActiveIdRef.current, selectedIdsRef.current) + lastActiveIdRef.current = nextId + setActiveFeatureId(nextId) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: nextId }) + }, [items]) // NOSONAR — eventBus/selectedIds consumed via refs to avoid spurious re-runs on selection change + useEffect(() => { const listboxEl = featuresRef.current if (!listboxEl) { @@ -52,6 +91,7 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu event.preventDefault() event.stopPropagation() const newId = getNavigatedId(activeFeatureIdRef.current, event.key, items) + lastActiveIdRef.current = newId setActiveFeatureId(newId) eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: newId }) } else if (event.key === 'Enter' || event.key === ' ') { @@ -68,14 +108,18 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu }, [viewportRef, featuresRef, items, eventBus]) const onFocus = () => { - if (items.length) { - const firstId = items[0].id - setActiveFeatureId(firstId) - eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: firstId }) + isFocusedRef.current = true + if (!items.length) { + return } + const id = resolveEntryId(items, lastActiveIdRef.current, selectedIds) + lastActiveIdRef.current = id + setActiveFeatureId(id) + eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id }) } const onBlur = () => { + isFocusedRef.current = false setActiveFeatureId(null) eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: null }) } diff --git a/src/App/hooks/useFeatureFocus.test.js b/src/App/hooks/useFeatureFocus.test.js index 40bb1e84..95248d51 100644 --- a/src/App/hooks/useFeatureFocus.test.js +++ b/src/App/hooks/useFeatureFocus.test.js @@ -57,7 +57,7 @@ describe('useFeatureFocus — initial state', () => { // ─── useFeatureFocus — onFocus ──────────────────────────────────────────────── describe('useFeatureFocus — onFocus', () => { - it('sets activeFeatureId to first item when items are present', () => { + it('sets activeFeatureId to first item on first focus (no previous, no selected)', () => { const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS })) act(() => result.current.onFocus()) expect(result.current.activeFeatureId).toBe('a') @@ -68,6 +68,61 @@ describe('useFeatureFocus — onFocus', () => { act(() => result.current.onFocus()) expect(result.current.activeFeatureId).toBeNull() }) + + it('sets active to first selected item (highest priority)', () => { + const eb = makeEventBus() + const { result } = renderHook(() => useFeatureFocus({ ...makeRefs(), items: ITEMS, eventBus: eb })) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'b', layerId: 'roads' }] })) + act(() => result.current.onFocus()) + expect(result.current.activeFeatureId).toBe('b') + }) + + it('selected item takes priority over last active position', () => { + const eb = makeEventBus() + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS, eventBus: eb })) + act(() => result.current.onFocus()) + fireKey(el, 'ArrowDown') // last active = 'b' + act(() => result.current.onBlur()) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'c', layerId: 'roads' }] })) + act(() => result.current.onFocus()) + expect(result.current.activeFeatureId).toBe('c') // selected beats last-active + unmount(); el.remove() + }) + + it('restores last active position when nothing is selected', () => { + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) + act(() => result.current.onFocus()) + fireKey(el, 'ArrowDown') // last active = 'b' + act(() => result.current.onBlur()) + act(() => result.current.onFocus()) + expect(result.current.activeFeatureId).toBe('b') + unmount(); el.remove() + }) + + it('falls back to first item when previous active is no longer in the list', () => { + const eb = makeEventBus() + const refs = makeRefs() + const el = refs.featuresRef.current + document.body.appendChild(el) + const { result, rerender, unmount } = renderHook( + ({ items }) => useFeatureFocus({ ...refs, items, eventBus: eb }), + { initialProps: { items: ITEMS } } + ) + act(() => result.current.onFocus()) + fireKey(el, 'ArrowDown') // active = 'b' + act(() => result.current.onBlur()) + const NEW_ITEMS = [{ id: 'x', label: 'X' }, { id: 'y', label: 'Y' }] + rerender({ items: NEW_ITEMS }) + act(() => result.current.onFocus()) + expect(result.current.activeFeatureId).toBe('x') + unmount(); el.remove() + }) }) // ─── useFeatureFocus — onBlur ───────────────────────────────────────────────── @@ -435,3 +490,72 @@ describe('useFeatureFocus — Enter and Space keys', () => { unmount() }) }) + +// ─── useFeatureFocus — items change while focused ───────────────────────────── + +describe('useFeatureFocus — items change while focused', () => { + afterEach(() => { document.body.innerHTML = '' }) + + const setupRerender = (initialItems) => { + const eb = makeEventBus() + const refs = makeRefs() + document.body.appendChild(refs.featuresRef.current) + const { result, rerender, unmount } = renderHook( + ({ items }) => useFeatureFocus({ ...refs, items, eventBus: eb }), + { initialProps: { items: initialItems } } + ) + return { eb, result, rerender, unmount } + } + + it('keeps current active item when it is still in the updated list', () => { + const { result, rerender, unmount } = setupRerender(ITEMS) + act(() => result.current.onFocus()) // active = 'a' + act(() => { rerender({ items: [{ id: 'a', label: 'A updated' }, { id: 'd', label: 'D' }] }) }) + expect(result.current.activeFeatureId).toBe('a') + unmount() + }) + + it('re-picks by priority when current active item leaves the list — first selected wins', () => { + const { eb, result, rerender, unmount } = setupRerender(ITEMS) + act(() => eb.trigger(SELECTION_CHANGE, { selectedFeatures: [{ featureId: 'c', layerId: 'l' }] })) + act(() => result.current.onFocus()) // active = 'a' (first, no prev) + act(() => { rerender({ items: [{ id: 'c', label: 'C' }, { id: 'd', label: 'D' }] }) }) // 'a' gone + expect(result.current.activeFeatureId).toBe('c') // first selected + unmount() + }) + + it('re-picks first item when active leaves and no selected item is in the new list', () => { + const { result, rerender, unmount } = setupRerender(ITEMS) + act(() => result.current.onFocus()) // active = 'a' + act(() => { rerender({ items: [{ id: 'x', label: 'X' }, { id: 'y', label: 'Y' }] }) }) + expect(result.current.activeFeatureId).toBe('x') + unmount() + }) + + it('does not change active item when the listbox is not focused', () => { + const { result, rerender, unmount } = setupRerender(ITEMS) + // never called onFocus — isFocusedRef stays false + act(() => { rerender({ items: [{ id: 'x', label: 'X' }] }) }) + expect(result.current.activeFeatureId).toBeNull() + unmount() + }) + + it('emits map:setactivefeature with new id when re-pick occurs', () => { + const { eb, result, rerender, unmount } = setupRerender(ITEMS) + act(() => result.current.onFocus()) + eb.emit.mockClear() + act(() => { rerender({ items: [{ id: 'x', label: 'X' }] }) }) // 'a' gone + expect(eb.emit).toHaveBeenCalledWith('map:setactivefeature', { id: 'x' }) + unmount() + }) + + it('clears active and emits null when items list becomes empty while focused', () => { + const { eb, result, rerender, unmount } = setupRerender(ITEMS) + act(() => result.current.onFocus()) + eb.emit.mockClear() + act(() => { rerender({ items: [] }) }) + expect(result.current.activeFeatureId).toBeNull() + expect(eb.emit).toHaveBeenCalledWith('map:setactivefeature', { id: null }) + unmount() + }) +}) From 1d6ee54b227131c808cd22fcb9e9b68b09836ced Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Thu, 30 Apr 2026 10:34:21 +0100 Subject: [PATCH 08/10] Clear focus on non keyboard interaction --- demo/js/index.js | 10 ++-- src/App/hooks/useFeatureFocus.js | 56 +++++++++++++++++------ src/App/hooks/useFeatureFocus.test.js | 66 +++++++++++++++++++++++++-- 3 files changed, 107 insertions(+), 25 deletions(-) diff --git a/demo/js/index.js b/demo/js/index.js index 14445faa..846216a5 100755 --- a/demo/js/index.js +++ b/demo/js/index.js @@ -54,10 +54,10 @@ const interactPlugin = createInteractPlugin({ // labelProperty: 'gid' // idProperty: 'gid' }, - { - layerId: 'hedge-control', - // idProperty: 'id' - }, + // { + // layerId: 'hedge-control', + // idProperty: 'id' + // }, // { // layerId: 'OS/TopographicArea_1/Agricultural Land', // idProperty: 'TOID' @@ -71,7 +71,7 @@ const interactPlugin = createInteractPlugin({ } ], debug: true, - interactionModes: ['selectMarker', 'placeMarker', 'selectFeature'], // e.g. ['selectMarker'], ['selectFeature'], ['placeMarker'], or combinations + interactionModes: ['selectMarker', 'selectFeature'], // e.g. ['selectMarker'], ['selectFeature'], ['placeMarker'], or combinations multiSelect: true, deselectOnClickOutside: true }) diff --git a/src/App/hooks/useFeatureFocus.js b/src/App/hooks/useFeatureFocus.js index 569b2970..82e000cf 100644 --- a/src/App/hooks/useFeatureFocus.js +++ b/src/App/hooks/useFeatureFocus.js @@ -24,18 +24,7 @@ const resolveEntryId = (items, lastActiveId, selectedIds) => { return items[0].id } -export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBus }) { - const [activeFeatureId, setActiveFeatureId] = useState(null) - const [selectedIds, setSelectedIds] = useState([]) - - const isFocusedRef = useRef(false) - const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus - const activeFeatureIdRef = useRef(null) // always-current for keydown closure - const selectedIdsRef = useRef([]) // always-current for items-change effect - - useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) - useEffect(() => { selectedIdsRef.current = selectedIds }, [selectedIds]) - +function useEventBusListeners ({ eventBus, lastActiveIdRef, setActiveFeatureId, setSelectedIds }) { useEffect(() => { if (!eventBus) { return undefined @@ -56,8 +45,9 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu eventBus.off('interact:selectionchange', handleSelectionChange) } }, [eventBus]) +} - // When items change while focused, revalidate: keep current if still present, else re-pick +function useItemsRevalidation ({ items, eventBus, isFocusedRef, lastActiveIdRef, activeFeatureIdRef, selectedIdsRef, setActiveFeatureId }) { useEffect(() => { if (!isFocusedRef.current) { return @@ -75,13 +65,14 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu setActiveFeatureId(nextId) eventBus?.emit(EVENTS.MAP_SET_ACTIVE_FEATURE, { id: nextId }) }, [items]) // NOSONAR — eventBus/selectedIds consumed via refs to avoid spurious re-runs on selection change +} +function useKeyboardNavigation ({ featuresRef, viewportRef, items, eventBus, activeFeatureIdRef, lastActiveIdRef, setActiveFeatureId }) { useEffect(() => { const listboxEl = featuresRef.current if (!listboxEl) { return undefined } - const handleKeyDown = (event) => { if (event.key === 'Escape') { event.preventDefault() @@ -102,10 +93,45 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu // No action } } - listboxEl.addEventListener('keydown', handleKeyDown) return () => { listboxEl.removeEventListener('keydown', handleKeyDown) } }, [viewportRef, featuresRef, items, eventBus]) +} + +// Any pointer interaction on the map (pan, zoom, click on feature or empty space) should +// hand focus back to the viewport so the listbox focus ring is cleared. +function useMapInteractionBlur ({ viewportRef, featuresRef, isFocusedRef }) { + useEffect(() => { + const el = viewportRef.current + if (!el) { + return undefined + } + const handlePointerDown = (event) => { + if (isFocusedRef.current && !featuresRef.current?.contains(event.target)) { + viewportRef.current?.focus() + } + } + el.addEventListener('pointerdown', handlePointerDown) + return () => { el.removeEventListener('pointerdown', handlePointerDown) } + }, [viewportRef, featuresRef]) +} + +export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBus }) { + const [activeFeatureId, setActiveFeatureId] = useState(null) + const [selectedIds, setSelectedIds] = useState([]) + + const isFocusedRef = useRef(false) + const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus + const activeFeatureIdRef = useRef(null) // always-current for keydown closure + const selectedIdsRef = useRef([]) // always-current for items-change effect + + useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) + useEffect(() => { selectedIdsRef.current = selectedIds }, [selectedIds]) + + useEventBusListeners({ eventBus, lastActiveIdRef, setActiveFeatureId, setSelectedIds }) + useItemsRevalidation({ items, eventBus, isFocusedRef, lastActiveIdRef, activeFeatureIdRef, selectedIdsRef, setActiveFeatureId }) + useKeyboardNavigation({ featuresRef, viewportRef, items, eventBus, activeFeatureIdRef, lastActiveIdRef, setActiveFeatureId }) + useMapInteractionBlur({ viewportRef, featuresRef, isFocusedRef }) const onFocus = () => { isFocusedRef.current = true diff --git a/src/App/hooks/useFeatureFocus.test.js b/src/App/hooks/useFeatureFocus.test.js index 95248d51..285fdf53 100644 --- a/src/App/hooks/useFeatureFocus.test.js +++ b/src/App/hooks/useFeatureFocus.test.js @@ -21,10 +21,14 @@ const makeEventBus = () => { } } -const makeRefs = ({ viewportFocus } = {}) => ({ - viewportRef: { current: { focus: viewportFocus ?? jest.fn() } }, - featuresRef: { current: document.createElement('ul') } -}) +const makeRefs = ({ viewportFocus } = {}) => { + const viewportEl = document.createElement('div') + viewportEl.focus = viewportFocus ?? jest.fn() + return { + viewportRef: { current: viewportEl }, + featuresRef: { current: document.createElement('ul') } + } +} const fireKey = (el, key) => { let event @@ -155,7 +159,8 @@ describe('useFeatureFocus — onBlur', () => { describe('useFeatureFocus — keydown listener lifecycle', () => { it('does not attach listener when featuresRef.current is null', () => { - const refs = { viewportRef: { current: { focus: jest.fn() } }, featuresRef: { current: null } } + const refs = makeRefs() + refs.featuresRef = { current: null } const spy = jest.spyOn(document.body, 'addEventListener') renderHook(() => useFeatureFocus(refs)) expect(spy).not.toHaveBeenCalledWith('keydown', expect.any(Function)) @@ -447,6 +452,57 @@ describe('useFeatureFocus — interact:selectionchange listener', () => { }) }) +// ─── useFeatureFocus — map interaction returns focus to viewport ────────────── + +describe('useFeatureFocus — map pointerdown returns focus to viewport', () => { + const pointerDown = (target) => act(() => { + target.dispatchEvent(new Event('pointerdown', { bubbles: true, cancelable: true })) + }) + + it('focuses the viewport when pointer lands on the map while listbox is focused', () => { + const viewportFocus = jest.fn() + const refs = makeRefs({ viewportFocus }) + document.body.appendChild(refs.viewportRef.current) + const mapCanvas = document.createElement('canvas') + refs.viewportRef.current.appendChild(mapCanvas) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) + act(() => result.current.onFocus()) + viewportFocus.mockClear() + pointerDown(mapCanvas) + expect(viewportFocus).toHaveBeenCalled() + unmount() + }) + + it('does not move focus when pointer lands inside the listbox', () => { + const viewportFocus = jest.fn() + const refs = makeRefs({ viewportFocus }) + document.body.appendChild(refs.viewportRef.current) + refs.viewportRef.current.appendChild(refs.featuresRef.current) + const option = document.createElement('li') + refs.featuresRef.current.appendChild(option) + const { result, unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) + act(() => result.current.onFocus()) + viewportFocus.mockClear() + pointerDown(option) + expect(viewportFocus).not.toHaveBeenCalled() + unmount() + }) + + it('does not move focus when pointer fires while listbox is not focused', () => { + const viewportFocus = jest.fn() + const refs = makeRefs({ viewportFocus }) + document.body.appendChild(refs.viewportRef.current) + const mapCanvas = document.createElement('canvas') + refs.viewportRef.current.appendChild(mapCanvas) + const { unmount } = renderHook(() => useFeatureFocus({ ...refs, items: ITEMS })) + pointerDown(mapCanvas) + expect(viewportFocus).not.toHaveBeenCalled() + unmount() + }) + + afterEach(() => { document.body.innerHTML = '' }) +}) + // ─── useFeatureFocus — Enter and Space keys ─────────────────────────────────── describe('useFeatureFocus — Enter and Space keys', () => { From 9c3c44e34a1fa5a4a7b29bd8e5f327ac862a12d9 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Thu, 30 Apr 2026 11:00:24 +0100 Subject: [PATCH 09/10] JSDoc added to hooks --- .../interact/src/hooks/useHighlightSync.js | 12 ++++++ plugins/interact/src/hooks/useMapItemList.js | 30 +++++++++++-- src/App/hooks/useFeatureFocus.js | 42 +++++++++++++++++-- src/App/hooks/useFeatureItems.js | 8 ++++ 4 files changed, 84 insertions(+), 8 deletions(-) diff --git a/plugins/interact/src/hooks/useHighlightSync.js b/plugins/interact/src/hooks/useHighlightSync.js index f529cf24..29878f1c 100755 --- a/plugins/interact/src/hooks/useHighlightSync.js +++ b/plugins/interact/src/hooks/useHighlightSync.js @@ -1,6 +1,18 @@ import { useEffect, useMemo } from 'react' import { buildStylesMap } from '../utils/buildStylesMap.js' +/** + * Keeps map highlight rendering in sync with the current selection and keyboard-active item. + * + * Calls mapProvider.updateHighlightedFeatures whenever selectedFeatures or listboxActiveItem + * changes, passing selected features (shown with the selection ring) and the active item + * (shown with the keyboard cursor ring) as separate arguments so the provider can style them + * differently. Also re-applies highlights after a map style reload, since highlight layers + * are removed when the base style changes. + * + * Dispatches UPDATE_SELECTED_BOUNDS with the bounding box returned by the provider so + * downstream consumers (e.g. interact:done) receive up-to-date bounds. + */ export const useHighlightSync = ({ mapProvider, mapStyle, diff --git a/plugins/interact/src/hooks/useMapItemList.js b/plugins/interact/src/hooks/useMapItemList.js index 7f2fb113..0d11faa9 100644 --- a/plugins/interact/src/hooks/useMapItemList.js +++ b/plugins/interact/src/hooks/useMapItemList.js @@ -70,6 +70,11 @@ const findFeatureById = (features, layerConfigMap, targetId) => { return null } +/** + * Rebuilds the keyboard-navigable item list whenever the map moves or data changes. + * Collects visible markers (by DOM visibility) and visible features (by viewport query), + * then emits MAP_SET_FEATURES so the listbox stays in sync with what the user can see. + */ function useItemListSync ({ markers, interactionModes, layers, mapProvider, multiSelect, eventBus }) { useEffect(() => { const handleMoveEnd = () => { @@ -91,9 +96,12 @@ function useItemListSync ({ markers, interactionModes, layers, mapProvider, mult }, [markers, interactionModes, layers, mapProvider, multiSelect, eventBus]) } -// Shows the selection ring without firing interact:selectionchange. -// Marker rings are handled by Markers.jsx listening to map:setactivefeature directly. -// Feature rings are handled by useHighlightSync reading listboxActiveItem from plugin state. +/** + * Listens for MAP_SET_ACTIVE_FEATURE and resolves the active item to its full feature/marker data, + * storing it in both a ref (for synchronous access) and plugin state (for highlight rendering). + * Shows the keyboard cursor ring without firing interact:selectionchange — committing the item + * to the real selection only happens when the user presses Enter/Space. + */ function useActiveItemHandler ({ markers, interactionModes, layers, mapProvider, eventBus, dispatch, listboxActiveItemRef }) { useEffect(() => { const handle = ({ id }) => { @@ -130,7 +138,11 @@ function useActiveItemHandler ({ markers, interactionModes, layers, mapProvider, }, [markers, interactionModes, layers, mapProvider, eventBus, dispatch, listboxActiveItemRef]) } -// Promotes the listbox-active item to a real selection, firing interact:selectionchange. +/** + * Handles MAP_SELECT_FEATURE (Enter/Space keypress) by promoting the currently active + * listbox item to a confirmed selection, dispatching TOGGLE_SELECTED_FEATURES or + * TOGGLE_SELECTED_MARKERS and triggering interact:selectionchange downstream. + */ function useSelectItemHandler ({ eventBus, dispatch, listboxActiveItemRef, multiSelect }) { useEffect(() => { const handleConfirm = () => { @@ -153,6 +165,16 @@ function useSelectItemHandler ({ eventBus, dispatch, listboxActiveItemRef, multi }, [eventBus, dispatch, listboxActiveItemRef, multiSelect]) } +/** + * Orchestrates the keyboard-accessible listbox for the interact plugin. + * + * Composes three concerns: + * - Item list sync — keeps the listbox populated with currently visible markers and features + * - Active item resolution — translates a listbox cursor position into full feature/marker data + * - Selection confirmation — commits the active item to the selection on Enter/Space + * + * @param {{ mapState: object, pluginState: object, services: object, mapProvider: object }} params + */ export function useMapItemList ({ mapState, pluginState, services, mapProvider }) { const { markers } = mapState const { dispatch, interactionModes, layers, multiSelect } = pluginState diff --git a/src/App/hooks/useFeatureFocus.js b/src/App/hooks/useFeatureFocus.js index 82e000cf..0033b657 100644 --- a/src/App/hooks/useFeatureFocus.js +++ b/src/App/hooks/useFeatureFocus.js @@ -24,6 +24,11 @@ const resolveEntryId = (items, lastActiveId, selectedIds) => { return items[0].id } +/** + * Keeps local selectedIds in sync with interact:selectionchange, and mirrors + * MAP_SET_ACTIVE_FEATURE back into React state so aria-activedescendant stays current. + * Also tracks the last non-null active id so it can be restored on re-focus. + */ function useEventBusListeners ({ eventBus, lastActiveIdRef, setActiveFeatureId, setSelectedIds }) { useEffect(() => { if (!eventBus) { @@ -47,6 +52,12 @@ function useEventBusListeners ({ eventBus, lastActiveIdRef, setActiveFeatureId, }, [eventBus]) } +/** + * Revalidates aria-activedescendant when the item list changes while the listbox is focused. + * If the current active item is no longer in the list (e.g. panned off screen), picks a new one + * using ARIA priority order: first selected → last active position → first item. + * Reads eventBus and selectedIds via refs to avoid spurious re-runs on selection change. + */ function useItemsRevalidation ({ items, eventBus, isFocusedRef, lastActiveIdRef, activeFeatureIdRef, selectedIdsRef, setActiveFeatureId }) { useEffect(() => { if (!isFocusedRef.current) { @@ -67,6 +78,12 @@ function useItemsRevalidation ({ items, eventBus, isFocusedRef, lastActiveIdRef, }, [items]) // NOSONAR — eventBus/selectedIds consumed via refs to avoid spurious re-runs on selection change } +/** + * Attaches a keydown listener to the listbox element for ARIA keyboard navigation: + * - ArrowUp/ArrowDown — move the active item, emitting MAP_SET_ACTIVE_FEATURE + * - Enter/Space — confirm selection, emitting MAP_SELECT_FEATURE + * - Escape — return focus to the map viewport + */ function useKeyboardNavigation ({ featuresRef, viewportRef, items, eventBus, activeFeatureIdRef, lastActiveIdRef, setActiveFeatureId }) { useEffect(() => { const listboxEl = featuresRef.current @@ -98,8 +115,11 @@ function useKeyboardNavigation ({ featuresRef, viewportRef, items, eventBus, act }, [viewportRef, featuresRef, items, eventBus]) } -// Any pointer interaction on the map (pan, zoom, click on feature or empty space) should -// hand focus back to the viewport so the listbox focus ring is cleared. +/** + * Clears listbox focus whenever the user interacts with the map via pointer (mouse or touch). + * Any pointerdown outside the listbox element moves focus to the viewport, removing the + * visible focus ring from the listbox without dropping focus to an unknown location. + */ function useMapInteractionBlur ({ viewportRef, featuresRef, isFocusedRef }) { useEffect(() => { const el = viewportRef.current @@ -116,14 +136,28 @@ function useMapInteractionBlur ({ viewportRef, featuresRef, isFocusedRef }) { }, [viewportRef, featuresRef]) } +/** + * Manages ARIA listbox focus state for the keyboard-accessible feature list. + * + * On focus, sets aria-activedescendant following ARIA priority order: + * 1. First selected item (if any) + * 2. Last active position (if still in the list) + * 3. First item (fallback) + * + * On blur, clears the active descendant. Revalidates automatically when the item list + * changes (e.g. after a map pan) so the active id never points to a stale item. + * + * @param {{ viewportRef: React.RefObject, featuresRef: React.RefObject, items: Array, eventBus: object }} params + * @returns {{ activeFeatureId: string|null, selectedIds: string[], onFocus: Function, onBlur: Function }} + */ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBus }) { const [activeFeatureId, setActiveFeatureId] = useState(null) const [selectedIds, setSelectedIds] = useState([]) const isFocusedRef = useRef(false) - const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus + const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus const activeFeatureIdRef = useRef(null) // always-current for keydown closure - const selectedIdsRef = useRef([]) // always-current for items-change effect + const selectedIdsRef = useRef([]) // always-current for items-change effect useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) useEffect(() => { selectedIdsRef.current = selectedIds }, [selectedIds]) diff --git a/src/App/hooks/useFeatureItems.js b/src/App/hooks/useFeatureItems.js index e4971826..57b59ded 100644 --- a/src/App/hooks/useFeatureItems.js +++ b/src/App/hooks/useFeatureItems.js @@ -1,5 +1,13 @@ import { useState, useEffect } from 'react' +/** + * Subscribes to map:setfeatures and exposes the current listbox item list and multiselectable flag. + * The list is produced by useMapItemList in the interact plugin and pushed via the event bus + * whenever the visible set of markers or features changes. + * + * @param {object} eventBus + * @returns {{ items: Array<{ id: string, label: string }>, multiselectable: boolean }} + */ export function useFeatureItems (eventBus) { const [items, setItems] = useState([]) const [multiselectable, setMultiselectable] = useState(false) From 49abe9902f3ffa5886a6ba40c5b03ec6fd6ef055 Mon Sep 17 00:00:00 2001 From: Dan Leech Date: Thu, 30 Apr 2026 11:54:09 +0100 Subject: [PATCH 10/10] Lint fix --- src/App/hooks/useFeatureFocus.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/App/hooks/useFeatureFocus.js b/src/App/hooks/useFeatureFocus.js index 0033b657..b3c7b996 100644 --- a/src/App/hooks/useFeatureFocus.js +++ b/src/App/hooks/useFeatureFocus.js @@ -155,9 +155,9 @@ export function useFeatureFocus ({ viewportRef, featuresRef, items = [], eventBu const [selectedIds, setSelectedIds] = useState([]) const isFocusedRef = useRef(false) - const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus + const lastActiveIdRef = useRef(null) // preserved across blur; restores position on re-focus const activeFeatureIdRef = useRef(null) // always-current for keydown closure - const selectedIdsRef = useRef([]) // always-current for items-change effect + const selectedIdsRef = useRef([]) // always-current for items-change effect useEffect(() => { activeFeatureIdRef.current = activeFeatureId }, [activeFeatureId]) useEffect(() => { selectedIdsRef.current = selectedIds }, [selectedIds])