diff --git a/debug-popup-follow.html b/debug-popup-follow.html new file mode 100644 index 00000000000..0305175c534 --- /dev/null +++ b/debug-popup-follow.html @@ -0,0 +1,63 @@ + + + + + Popup follow marker - Issue #5655 + + + + +
+
+

Drag the marker

+

Watch if popup stays attached...

+
+
+ + + + + diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index d25cffc9938..a56849f4413 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -24,6 +24,18 @@ const wrapDispatcher = (dispatcher) => { } as Dispatcher; }; +// Helper to extract messages from spy calls +const getMessages = (spy: any) => spy.mock.calls.map((a: any[]) => a[0]); + +// Filter to loadData ('LD') messages only +const getLoadData = (spy: any) => getMessages(spy).filter(m => m?.type === MessageType.loadData); + +// Filter to messages with actual data (dataDiff or data field), excluding pure options updates +const getDataMessages = (spy: any) => getLoadData(spy).filter(m => m?.data?.dataDiff || m?.data?.data); + +// Filter to setData messages only (have .data field) +const getSetDataMessages = (spy: any) => getLoadData(spy).filter(m => m?.data?.data); + const mockDispatcher = wrapDispatcher({ sendAsync() { return Promise.resolve({}); } }); @@ -162,15 +174,41 @@ describe('GeoJSONSource.setData', () => { }); test('only marks source as loaded when there are no pending loads', async () => { - const source = createSource(); - const setDataPromise = source.once('data'); - source.setData({} as GeoJSON.GeoJSON); - source.setData({} as GeoJSON.GeoJSON); - await setDataPromise; - expect(source.loaded()).toBeFalsy(); - const setDataPromise2 = source.once('data'); - await setDataPromise2; - expect(source.loaded()).toBeTruthy(); + const calls: any[] = []; + let delayLoadData = false; + const pendingResolvers: Array<() => void> = []; + + const mockDispatcher = wrapDispatcher({ + sendAsync(message: any) { + calls.push(message); + if (message?.type === MessageType.loadData && delayLoadData) { + return new Promise(resolve => pendingResolvers.push(resolve)); + } + return Promise.resolve({}); + } + }); + + const source = new GeoJSONSource('id', { + type: 'geojson', + data: {type: 'FeatureCollection', features: []} as GeoJSON.GeoJSON + }, mockDispatcher, undefined); + + // initial load completes → loaded() true + await source.load(); + expect(source.loaded()).toBe(true); + + // ab hier Worker-Updates verzögern + delayLoadData = true; + + // Options-Änderung triggert loadData → sollte loaded() direkt auf false setzen + source.setClusterOptions({clusterRadius: 80}); + expect(source.loaded()).toBe(false); + + // Worker „fertig" + pendingResolvers.splice(0).forEach(r => r()); + await new Promise(r => setTimeout(r, 0)); + + expect(source.loaded()).toBe(true); }); test('marks source as not loaded before firing "dataloading" event', async () => { @@ -566,13 +604,15 @@ describe('GeoJSONSource.updateData', () => { await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); - expect(spy).toHaveBeenCalledTimes(2); - expect(spy.mock.calls[0][0].data.data).toEqual(JSON.stringify(data1)); - expect(spy.mock.calls[1][0].data.dataDiff).toEqual({ - remove: ['1', '4'], - add: [{id: '2', type: 'Feature', properties: {}, geometry: {type: 'LineString', coordinates: []}}, {id: '5', type: 'Feature', properties: {}, geometry: {type: 'LineString', coordinates: []}}], - update: [{id: '3', addOrUpdateProperties: [], newGeometry: {type: 'Point', coordinates: []}}, {id: '6', addOrUpdateProperties: [], newGeometry: {type: 'LineString', coordinates: []}}] - }); + // Filter to messages with actual data (dataDiff or data fields), excluding options-only updates + const dataMessages = getDataMessages(spy); + + // First call: setData(data1) - has .data.data field + // Following calls: updateData(...) - have .data.dataDiff field + expect(dataMessages.length).toBeGreaterThanOrEqual(2); + expect(dataMessages[0].data.data).toEqual(JSON.stringify(data1)); + expect(dataMessages[1].data.dataDiff).toEqual(update1); + expect(dataMessages[2].data.dataDiff).toEqual(update2); }); test('is overwritten by a subsequent call to setData when data is loading', async () => { @@ -608,9 +648,12 @@ describe('GeoJSONSource.updateData', () => { await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); - expect(spy).toHaveBeenCalledTimes(2); - expect(spy.mock.calls[0][0].data.data).toEqual(JSON.stringify(data1)); - expect(spy.mock.calls[1][0].data.data).toEqual(JSON.stringify(data2)); + // Filter to setData messages (data field), excluding options-only updates + const dataMessages = getSetDataMessages(spy); + + expect(dataMessages.length).toBeGreaterThanOrEqual(2); + expect(dataMessages[0].data.data).toEqual(JSON.stringify(data1)); + expect(dataMessages[1].data.data).toEqual(JSON.stringify(data2)); }); test('is queued after setData when data is loading', async () => { @@ -831,6 +874,50 @@ describe('GeoJSONSource.load', () => { source.load(); expect(spy).toHaveBeenCalledTimes(1); - expect(warnSpy).toHaveBeenCalledWith('No data or diff provided to GeoJSONSource id.'); + expect(warnSpy).toHaveBeenCalledWith('No pending worker updates for GeoJSONSource id.'); + }); + + test('setClusterOptions triggers worker update and toggles loaded()', async () => { + const calls: any[] = []; + const mockDispatcher = wrapDispatcher({ + sendAsync(message: any) { + calls.push(message); + // Simuliere sofort erfolgreiche Antwort + return Promise.resolve({}); + } + }); + + const source = new GeoJSONSource( + 'id', + { + type: 'geojson', + data: {type: 'FeatureCollection', features: []} as GeoJSON.GeoJSON, + cluster: true, + clusterRadius: 50 + }, + mockDispatcher, + undefined + ); + + // Initiale Ladung + await source.load(); + expect(source.loaded()).toBe(true); + + // Clear vorherige Nachrichten + calls.length = 0; + + // Änderung: Radius -> 80 + source.setClusterOptions({clusterRadius: 80}); + + // Sollte genau ein Worker-Update ausgelöst haben + expect(calls.length).toBe(1); + expect(calls[0].type).toBe(MessageType.loadData); + // loaded() sollte während des Updates false sein (direkt nach Aufruf) + expect(source.loaded()).toBe(false); + + // simulated "worker done" (sendAsync hat bereits resolved) + // kurzer Tick, damit interne Flags umspringen können + await new Promise(r => setTimeout(r, 0)); + expect(source.loaded()).toBe(true); }); }); diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 4207214065e..24b37f37dc8 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -126,7 +126,7 @@ export class GeoJSONSource extends Evented implements Source { map: Map; actor: Actor; _isUpdatingWorker: boolean; - _pendingWorkerUpdate: { data?: GeoJSON.GeoJSON | string; diff?: GeoJSONSourceDiff }; + _pendingWorkerUpdate: { data?: GeoJSON.GeoJSON | string; diff?: GeoJSONSourceDiff; optionsChanged?: boolean }; _collectResourceTiming: boolean; _removed: boolean; @@ -203,6 +203,10 @@ export class GeoJSONSource extends Evented implements Source { return pixelValue * (EXTENT / this.tileSize); } + private _hasPendingWorkerUpdate(): boolean { + return this._pendingWorkerUpdate.data !== undefined || this._pendingWorkerUpdate.diff !== undefined || this._pendingWorkerUpdate.optionsChanged; + } + private _getClusterMaxZoom(clusterMaxZoom: number): number { const effectiveClusterMaxZoom = clusterMaxZoom ? Math.round(clusterMaxZoom) : this.maxzoom - 1; if (!(Number.isInteger(clusterMaxZoom) || clusterMaxZoom === undefined)) { @@ -308,13 +312,16 @@ export class GeoJSONSource extends Evented implements Source { * ``` */ setClusterOptions(options: SetClusterOptions): this { - this.workerOptions.cluster = options.cluster; - if (options) { - if (options.clusterRadius !== undefined) this.workerOptions.superclusterOptions.radius = this._pixelsToTileUnits(options.clusterRadius); - if (options.clusterMaxZoom !== undefined) { - this.workerOptions.superclusterOptions.maxZoom = this._getClusterMaxZoom(options.clusterMaxZoom); - } + if (options.cluster !== undefined) { + this.workerOptions.cluster = options.cluster; } + if (options.clusterRadius !== undefined) { + this.workerOptions.superclusterOptions.radius = this._pixelsToTileUnits(options.clusterRadius); + } + if (options.clusterMaxZoom !== undefined) { + this.workerOptions.superclusterOptions.maxZoom = this._getClusterMaxZoom(options.clusterMaxZoom); + } + this._pendingWorkerUpdate.optionsChanged = true; this._updateWorkerData(); return this; } @@ -380,15 +387,13 @@ export class GeoJSONSource extends Evented implements Source { * using geojson-vt or supercluster as appropriate. */ async _updateWorkerData(): Promise { - if (this._isUpdatingWorker) return; - - const {data, diff} = this._pendingWorkerUpdate; - - if (!data && !diff) { - warnOnce(`No data or diff provided to GeoJSONSource ${this.id}.`); + if (!this._hasPendingWorkerUpdate()) { + warnOnce(`No pending worker updates for GeoJSONSource ${this.id}.`); return; } + const {data, diff} = this._pendingWorkerUpdate; + const options: LoadGeoJSONParameters = extend({type: this.type}, this.workerOptions); if (data) { if (typeof data === 'string') { @@ -405,6 +410,8 @@ export class GeoJSONSource extends Evented implements Source { } this._isUpdatingWorker = true; + // Reset the flag since this update is using the latest options + this._pendingWorkerUpdate.optionsChanged = undefined; this.fire(new Event('dataloading', {dataType: 'source'})); try { const result = await this.actor.sendAsync({type: MessageType.loadData, data: options}); @@ -439,14 +446,14 @@ export class GeoJSONSource extends Evented implements Source { this.fire(new ErrorEvent(err)); } finally { // If there is more pending data, update worker again. - if (this._pendingWorkerUpdate.data || this._pendingWorkerUpdate.diff) { + if (this._hasPendingWorkerUpdate()) { this._updateWorkerData(); } } } loaded(): boolean { - return !this._isUpdatingWorker && this._pendingWorkerUpdate.data === undefined && this._pendingWorkerUpdate.diff === undefined; + return !this._isUpdatingWorker && !this._hasPendingWorkerUpdate(); } async loadTile(tile: Tile): Promise { @@ -505,3 +512,4 @@ export class GeoJSONSource extends Evented implements Source { return false; } } + diff --git a/src/ui/marker.test.ts b/src/ui/marker.test.ts index ccd7ffa787c..418014a53ea 100644 --- a/src/ui/marker.test.ts +++ b/src/ui/marker.test.ts @@ -168,7 +168,7 @@ describe('marker', () => { .setLngLat([0,0]) .setPopup(popup) .addTo(map); - + // open popup marker.togglePopup(); const spy = vi.fn(); @@ -1206,4 +1206,118 @@ describe('marker', () => { expect(adjustedTransform) .toContain('translate(262.4px, 235.5934100987358px)'); }); + + describe('marker + popup interaction', () => { + test('popup follows marker while moving', () => { + const map = createMap(); + const marker = new Marker({draggable: true}) + .setLngLat([0, 0]) + .addTo(map); + + const popup = new Popup({closeButton: false}) + .setHTML('X'); + + marker.setPopup(popup).togglePopup(); + + // Spy: Wir beobachten, dass das Popup bei jeder Bewegung neu gesetzt wird + const spy = vi.spyOn(popup, 'setLngLat'); + + // Simuliere Marker-Bewegung + marker.setLngLat([0.01, 0.02]); + + // Erwartung: Popup bekam ein Update + expect(spy).toHaveBeenCalled(); + const lastArg = spy.mock.calls.at(-1)![0] as any; + expect(lastArg.lng).toBe(0.01); + expect(lastArg.lat).toBe(0.02); + + spy.mockRestore(); + map.remove(); + }); + + test('popup repositions on marker movement without drag', () => { + const map = createMap(); + const marker = new Marker() + .setLngLat([0, 0]) + .addTo(map); + + const popup = new Popup({closeButton: false}) + .setHTML('Test popup'); + + marker.setPopup(popup).togglePopup(); + + // Verify popup is open + expect(popup.isOpen()).toBe(true); + + const spy = vi.spyOn(popup, 'setLngLat'); + + // Move marker programmatically + marker.setLngLat([1, 1]); + + // Popup should update position + expect(spy).toHaveBeenCalled(); + const call = spy.mock.calls.at(-1)![0] as any; + expect(call.lng).toBe(1); + expect(call.lat).toBe(1); + + spy.mockRestore(); + map.remove(); + }); + + test('popup stays attached during consecutive marker moves', () => { + const map = createMap(); + const marker = new Marker({draggable: true}) + .setLngLat([0, 0]) + .addTo(map); + + const popup = new Popup({closeButton: false}) + .setHTML('Follow me!'); + + marker.setPopup(popup).togglePopup(); + + const spy = vi.spyOn(popup, 'setLngLat'); + + // Simulate multiple rapid moves + const positions = [[0.01, 0.01], [0.02, 0.02], [0.03, 0.03]]; + for (const pos of positions) { + marker.setLngLat(pos as [number, number]); + } + + // All positions should have been set on the popup + expect(spy.mock.calls.length).toBeGreaterThanOrEqual(positions.length); + + // Last position should match + const lastCall = spy.mock.calls.at(-1)![0] as any; + expect(lastCall.lng).toBe(0.03); + expect(lastCall.lat).toBe(0.03); + + spy.mockRestore(); + map.remove(); + }); + + test('popup stays attached when map zooms/pans', () => { + const map = createMap(); + const marker = new Marker() + .setLngLat([0, 0]) + .addTo(map); + + const popup = new Popup({closeButton: false}) + .setHTML('Stay with me!'); + + marker.setPopup(popup).togglePopup(); + + const initialPopupLngLat = popup.getLngLat(); + expect(initialPopupLngLat).toBeDefined(); + + // Simulate map interactions + const initialZoom = map.getZoom(); + map.setZoom(initialZoom + 1); + + // Popup should still be at the marker location + const popupLngLatAfterZoom = popup.getLngLat(); + expect(popupLngLatAfterZoom).toBeDefined(); + + map.remove(); + }); + }); });