From a9c1a43e49c95fb3dc31fea832e18a2c662c4eba Mon Sep 17 00:00:00 2001 From: Nicholas Kundu Date: Thu, 25 Jul 2024 20:10:49 +0000 Subject: [PATCH 1/5] #4268 fix async race when adding/removing geolocate rapidly --- src/ui/control/geolocate_control.ts | 16 ++++++++++--- test/examples/add-remove-geolocate.html | 30 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 test/examples/add-remove-geolocate.html diff --git a/src/ui/control/geolocate_control.ts b/src/ui/control/geolocate_control.ts index a62efad521..77bed43f58 100644 --- a/src/ui/control/geolocate_control.ts +++ b/src/ui/control/geolocate_control.ts @@ -281,7 +281,8 @@ export class GeolocateControl extends Evented implements IControl { onAdd(map: Map) { this._map = map; this._container = DOM.create('div', 'maplibregl-ctrl maplibregl-ctrl-group'); - checkGeolocationSupport().then((supported) => this._setupUI(supported)); + this._setupUI(); + checkGeolocationSupport().then((supported) => this._finishSetupUI(supported)); return this._container; } @@ -523,8 +524,7 @@ export class GeolocateControl extends Evented implements IControl { this._timeoutId = undefined; }; - _setupUI = (supported: boolean) => { - // this method is called asynchronously during onAdd + _setupUI = () => { // the control could have been removed before reaching here if (!this._map) { return; @@ -534,6 +534,15 @@ export class GeolocateControl extends Evented implements IControl { this._geolocateButton = DOM.create('button', 'maplibregl-ctrl-geolocate', this._container); DOM.create('span', 'maplibregl-ctrl-icon', this._geolocateButton).setAttribute('aria-hidden', 'true'); this._geolocateButton.type = 'button'; + this._geolocateButton.disabled = true; + }; + + _finishSetupUI = (supported: boolean) => { + // this method is called asynchronously during onAdd + if (!this._map) { + // control has since been removed + return; + } if (supported === false) { warnOnce('Geolocation support is not available so the GeolocateControl will be disabled.'); @@ -543,6 +552,7 @@ export class GeolocateControl extends Evented implements IControl { this._geolocateButton.setAttribute('aria-label', title); } else { const title = this._map._getUIString('GeolocateControl.FindMyLocation'); + this._geolocateButton.disabled = false; this._geolocateButton.title = title; this._geolocateButton.setAttribute('aria-label', title); } diff --git a/test/examples/add-remove-geolocate.html b/test/examples/add-remove-geolocate.html new file mode 100644 index 0000000000..ff56d63465 --- /dev/null +++ b/test/examples/add-remove-geolocate.html @@ -0,0 +1,30 @@ + + + + Geolocation Control + + + + + + + + +
+ + + \ No newline at end of file From 57296ed69db39f455e48e4c1b739fa8cd64f46cc Mon Sep 17 00:00:00 2001 From: Nicholas Kundu Date: Mon, 29 Jul 2024 13:18:54 +0000 Subject: [PATCH 2/5] add test for bug fix, remove example, update changelog --- CHANGELOG.md | 1 + test/examples/add-remove-geolocate.html | 30 ------------------------ test/integration/browser/browser.test.ts | 15 ++++++++++++ 3 files changed, 16 insertions(+), 30 deletions(-) delete mode 100644 test/examples/add-remove-geolocate.html diff --git a/CHANGELOG.md b/CHANGELOG.md index 760decd15c..0b6eb21540 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - Fix lag on fast map zoom ([#4366](https://github.com/maplibre/maplibre-gl-js/pull/4366)) - Fix unguarded read access to possibly undefined object ([#4431](https://github.com/maplibre/maplibre-gl-js/pull/4431)) - Fix remove hash string when map is removed ([#4427](https://github.com/maplibre/maplibre-gl-js/pull/4427)) +- Fix GeolocateControl may be added twice when calling addControl/removeControl/addControl rapidly ([#4454](https://github.com/maplibre/maplibre-gl-js/pull/4454)) - _...Add new stuff here..._ ## 4.5.0 diff --git a/test/examples/add-remove-geolocate.html b/test/examples/add-remove-geolocate.html deleted file mode 100644 index ff56d63465..0000000000 --- a/test/examples/add-remove-geolocate.html +++ /dev/null @@ -1,30 +0,0 @@ - - - - Geolocation Control - - - - - - - - -
- - - \ No newline at end of file diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index 5efa3b2508..5a5d9c834c 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -447,4 +447,19 @@ describe('Browser tests', () => { expect(center.lng).toBeCloseTo(11.39770); expect(center.lat).toBeCloseTo(47.29960); }, 20000); + + test('Geolocate control should appear only once', async () => { + await page.evaluate(async () => { + const geolocateControl = new maplibregl.GeolocateControl({}); + + map.addControl(geolocateControl); + map.removeControl(geolocateControl); + map.addControl(geolocateControl); + + await map.once('idle'); + }); + + const geolocateUIelem = await page.$$('.maplibregl-ctrl-geolocate'); + expect(geolocateUIelem.length).toBe(1); + }, 20000); }); From c24f34cb8dfe6c56bc5a68fa1f569728d5849958 Mon Sep 17 00:00:00 2001 From: Nicholas Kundu Date: Mon, 29 Jul 2024 13:24:34 +0000 Subject: [PATCH 3/5] fix linter errors/warnings --- test/integration/browser/browser.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index 5a5d9c834c..16ed022a45 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -455,11 +455,11 @@ describe('Browser tests', () => { map.addControl(geolocateControl); map.removeControl(geolocateControl); map.addControl(geolocateControl); - + await map.once('idle'); }); const geolocateUIelem = await page.$$('.maplibregl-ctrl-geolocate'); - expect(geolocateUIelem.length).toBe(1); + expect(geolocateUIelem).toHaveLength(1); }, 20000); }); From 6ef98c15d9bb914ea251c761f17a01fc25cf5799 Mon Sep 17 00:00:00 2001 From: Nicholas Kundu Date: Mon, 29 Jul 2024 15:49:00 +0000 Subject: [PATCH 4/5] convert to unit test --- src/ui/control/geolocate_control.test.ts | 37 ++++++++++++++++++++++++ test/integration/browser/browser.test.ts | 14 --------- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/ui/control/geolocate_control.test.ts b/src/ui/control/geolocate_control.test.ts index 0ececfd88a..37ac41f921 100644 --- a/src/ui/control/geolocate_control.test.ts +++ b/src/ui/control/geolocate_control.test.ts @@ -583,4 +583,41 @@ describe('GeolocateControl with no options', () => { await zoomendPromise; expect(geolocate._circleElement.style.width).toBeTruthy(); }); + + test('shown even if trackUserLocation = false', async () => { + const geolocate = new GeolocateControl({ + trackUserLocation: false, + showUserLocation: true, + showAccuracyCircle: true, + }); + map.addControl(geolocate); + await sleep(0); + const click = new window.Event('click'); + + const geolocatePromise = geolocate.once('geolocate'); + geolocate._geolocateButton.dispatchEvent(click); + geolocation.send({latitude: 10, longitude: 20, accuracy: 700}); + await geolocatePromise; + map.jumpTo({ + center: [10, 20] + }); + const zoomendPromise = map.once('zoomend'); + map.zoomTo(10, {duration: 0}); + await zoomendPromise; + expect(geolocate._circleElement.style.width).toBeTruthy(); + }); + + test('Geolocate control should appear only once', async () => { + const geolocateControl = new GeolocateControl({}); + + map.addControl(geolocateControl); + // adding and removing to verify there is no race condition, and it is just added once + map.removeControl(geolocateControl); + map.addControl(geolocateControl); + + await map.once('idle'); + + const geolocateUIelem = await geolocateControl._container.getElementsByClassName('maplibregl-ctrl-geolocate'); + expect(geolocateUIelem).toHaveLength(1); + }); }); diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index 16ed022a45..88f146486e 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -448,18 +448,4 @@ describe('Browser tests', () => { expect(center.lat).toBeCloseTo(47.29960); }, 20000); - test('Geolocate control should appear only once', async () => { - await page.evaluate(async () => { - const geolocateControl = new maplibregl.GeolocateControl({}); - - map.addControl(geolocateControl); - map.removeControl(geolocateControl); - map.addControl(geolocateControl); - - await map.once('idle'); - }); - - const geolocateUIelem = await page.$$('.maplibregl-ctrl-geolocate'); - expect(geolocateUIelem).toHaveLength(1); - }, 20000); }); From 4265456f9de97a06e1a12ac02de702db75447cc6 Mon Sep 17 00:00:00 2001 From: Harel M Date: Mon, 29 Jul 2024 23:07:18 +0300 Subject: [PATCH 5/5] Update test/integration/browser/browser.test.ts --- test/integration/browser/browser.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index 88f146486e..5efa3b2508 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -447,5 +447,4 @@ describe('Browser tests', () => { expect(center.lng).toBeCloseTo(11.39770); expect(center.lat).toBeCloseTo(47.29960); }, 20000); - });