From a6ec31045f1a5f68c8b1320ce1917edb66d2e6d7 Mon Sep 17 00:00:00 2001 From: avpeery Date: Wed, 11 Aug 2021 09:46:04 -0700 Subject: [PATCH 1/9] Initial fix of force firing of onClose to happen first if multiple event listeners --- src/util/evented.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/util/evented.js b/src/util/evented.js index 745ebea16de..4f7c0e7e95e 100644 --- a/src/util/evented.js +++ b/src/util/evented.js @@ -1,5 +1,6 @@ // @flow +import { convertChangesToXML } from 'diff'; import {extend} from './util.js'; type Listener = (Object) => any; @@ -68,6 +69,7 @@ export class Evented { * @returns {Object} Returns itself to allow for method chaining. */ on(type: *, listener: Listener): this { + this._listeners = this._listeners || {}; _addEventListener(type, listener, this._listeners); @@ -124,6 +126,12 @@ export class Evented { // make sure adding or removing listeners inside other listeners won't cause an infinite loop const listeners = this._listeners && this._listeners[type] ? this._listeners[type].slice() : []; + + // Check if onClose is an event listener and move to front of list to fire first + if (listeners.length > 1 && listeners[1].name == "bound _onClose") { + listeners.unshift(listeners.splice(1, 1)[0]); + } + for (const listener of listeners) { listener.call(this, event); } From 3f877d483e216965a2bba759a0c2cdd68dabce9d Mon Sep 17 00:00:00 2001 From: avpeery Date: Wed, 11 Aug 2021 09:53:56 -0700 Subject: [PATCH 2/9] removed unused added importand extra space --- src/util/evented.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/evented.js b/src/util/evented.js index 4f7c0e7e95e..37bf4ad4b4c 100644 --- a/src/util/evented.js +++ b/src/util/evented.js @@ -1,6 +1,5 @@ // @flow -import { convertChangesToXML } from 'diff'; import {extend} from './util.js'; type Listener = (Object) => any; @@ -69,7 +68,6 @@ export class Evented { * @returns {Object} Returns itself to allow for method chaining. */ on(type: *, listener: Listener): this { - this._listeners = this._listeners || {}; _addEventListener(type, listener, this._listeners); From 9a10afdfa68015e8cf397781668e0cba4e27c157 Mon Sep 17 00:00:00 2001 From: avpeery Date: Wed, 11 Aug 2021 11:31:57 -0700 Subject: [PATCH 3/9] changed == to === --- src/util/evented.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/evented.js b/src/util/evented.js index 37bf4ad4b4c..750034d48d1 100644 --- a/src/util/evented.js +++ b/src/util/evented.js @@ -126,7 +126,7 @@ export class Evented { const listeners = this._listeners && this._listeners[type] ? this._listeners[type].slice() : []; // Check if onClose is an event listener and move to front of list to fire first - if (listeners.length > 1 && listeners[1].name == "bound _onClose") { + if (listeners.length > 1 && listeners[1].name === "bound _onClose") { listeners.unshift(listeners.splice(1, 1)[0]); } From 7513961ce0e8fac8acb6ca0ca7c05fdefe8e936d Mon Sep 17 00:00:00 2001 From: avpeery Date: Wed, 11 Aug 2021 16:41:59 -0700 Subject: [PATCH 4/9] Implemented preclick solution and adjusted map event tests to reflect preclick call on click events --- src/ui/events.js | 12 ++++++++++++ src/ui/handler/map_event.js | 5 +++++ src/ui/popup.js | 2 +- src/util/evented.js | 5 ----- test/unit/ui/map_events.test.js | 16 ++++++++-------- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/ui/events.js b/src/ui/events.js index e570e769c43..50a378c7674 100644 --- a/src/ui/events.js +++ b/src/ui/events.js @@ -43,6 +43,7 @@ export class MapMouseEvent extends Event { */ type: 'mousedown' | 'mouseup' + | 'preclick' | 'click' | 'dblclick' | 'mousemove' @@ -575,6 +576,17 @@ export type MapEvent = */ | 'mousemove' + /** + * Triggered when a click event occurs and is fired before the click event. + * Primarily implemented to ensure closeOnClick for pop-ups is fired before any other listeners. + * + * @event preclick + * @memberof Map + * @instance + * @type {MapMouseEvent} + */ + | 'preclick' + /** * Fired when a pointing device (usually a mouse) is pressed and released at the same point on the map. * diff --git a/src/ui/handler/map_event.js b/src/ui/handler/map_event.js index ca9e4085044..273a6589ab3 100644 --- a/src/ui/handler/map_event.js +++ b/src/ui/handler/map_event.js @@ -38,8 +38,13 @@ export class MapEventHandler { this._map.fire(new MapMouseEvent(e.type, this._map, e)); } + preclick(e: MouseEvent) { + this._map.fire(new MapMouseEvent(e.type, this._map, e)); + } + click(e: MouseEvent, point: Point) { if (this._mousedownPos && this._mousedownPos.dist(point) >= this._clickTolerance) return; + this.preclick(e); this._map.fire(new MapMouseEvent(e.type, this._map, e)); } diff --git a/src/ui/popup.js b/src/ui/popup.js index 04f96e007a2..2ad74604437 100644 --- a/src/ui/popup.js +++ b/src/ui/popup.js @@ -140,7 +140,7 @@ export default class Popup extends Evented { this._map = map; if (this.options.closeOnClick) { - this._map.on('click', this._onClose); + this._map.on('preclick', this._onClose); } if (this.options.closeOnMove) { diff --git a/src/util/evented.js b/src/util/evented.js index 750034d48d1..ecceedd00e9 100644 --- a/src/util/evented.js +++ b/src/util/evented.js @@ -125,11 +125,6 @@ export class Evented { // make sure adding or removing listeners inside other listeners won't cause an infinite loop const listeners = this._listeners && this._listeners[type] ? this._listeners[type].slice() : []; - // Check if onClose is an event listener and move to front of list to fire first - if (listeners.length > 1 && listeners[1].name === "bound _onClose") { - listeners.unshift(listeners.splice(1, 1)[0]); - } - for (const listener of listeners) { listener.call(this, event); } diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index ece29ad28d4..ce2888b6289 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -12,7 +12,7 @@ test('Map#on adds a non-delegated event listener', (t) => { map.on('click', spy); simulate.click(map.getCanvas()); - t.ok(spy.calledOnce); + t.ok(spy.calledTwice); t.end(); }); @@ -47,7 +47,7 @@ test('Map#on adds a listener for an event on a given layer', (t) => { map.on('click', 'layer', spy); simulate.click(map.getCanvas()); - t.ok(spy.calledOnce); + t.ok(spy.calledTwice); t.end(); }); @@ -129,8 +129,8 @@ test('Map#on distinguishes distinct layers', (t) => { map.on('click', 'B', spyB); simulate.click(map.getCanvas()); - t.ok(spyA.calledOnce); - t.ok(spyB.calledOnce); + t.ok(spyA.calledTwice); + t.ok(spyB.calledTwice); t.end(); }); @@ -147,8 +147,8 @@ test('Map#on distinguishes distinct listeners', (t) => { map.on('click', 'layer', spyB); simulate.click(map.getCanvas()); - t.ok(spyA.calledOnce); - t.ok(spyB.calledOnce); + t.ok(spyA.calledTwice); + t.ok(spyB.calledTwice); t.end(); }); @@ -206,7 +206,7 @@ test('Map#off distinguishes distinct layers', (t) => { map.off('click', 'B', spy); simulate.click(map.getCanvas()); - t.ok(spy.calledOnce); + t.ok(spy.calledTwice); t.end(); }); @@ -224,7 +224,7 @@ test('Map#off distinguishes distinct listeners', (t) => { map.off('click', 'layer', spyB); simulate.click(map.getCanvas()); - t.ok(spyA.calledOnce); + t.ok(spyA.calledTwice); t.ok(spyB.notCalled); t.end(); }); From 5dc231c3a5bb09e1283a3d8fa87f0faf82dbfad4 Mon Sep 17 00:00:00 2001 From: avpeery Date: Thu, 12 Aug 2021 10:21:34 -0700 Subject: [PATCH 5/9] added sythetic preclick, WIP: closeonclick listener not activating --- src/ui/handler/map_event.js | 5 ++++- test/unit/ui/map_events.test.js | 16 ++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/ui/handler/map_event.js b/src/ui/handler/map_event.js index 273a6589ab3..555c6704ddf 100644 --- a/src/ui/handler/map_event.js +++ b/src/ui/handler/map_event.js @@ -1,5 +1,6 @@ // @flow +import { extend } from '../../util/util.js'; import {MapMouseEvent, MapTouchEvent, MapWheelEvent} from '../events.js'; import type Map from '../map.js'; @@ -39,7 +40,9 @@ export class MapEventHandler { } preclick(e: MouseEvent) { - this._map.fire(new MapMouseEvent(e.type, this._map, e)); + const synth = extend({}, e); + synth.type = 'preclick'; + this._map.fire(new MapMouseEvent(synth.type, this._map, synth)); } click(e: MouseEvent, point: Point) { diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index ce2888b6289..ece29ad28d4 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -12,7 +12,7 @@ test('Map#on adds a non-delegated event listener', (t) => { map.on('click', spy); simulate.click(map.getCanvas()); - t.ok(spy.calledTwice); + t.ok(spy.calledOnce); t.end(); }); @@ -47,7 +47,7 @@ test('Map#on adds a listener for an event on a given layer', (t) => { map.on('click', 'layer', spy); simulate.click(map.getCanvas()); - t.ok(spy.calledTwice); + t.ok(spy.calledOnce); t.end(); }); @@ -129,8 +129,8 @@ test('Map#on distinguishes distinct layers', (t) => { map.on('click', 'B', spyB); simulate.click(map.getCanvas()); - t.ok(spyA.calledTwice); - t.ok(spyB.calledTwice); + t.ok(spyA.calledOnce); + t.ok(spyB.calledOnce); t.end(); }); @@ -147,8 +147,8 @@ test('Map#on distinguishes distinct listeners', (t) => { map.on('click', 'layer', spyB); simulate.click(map.getCanvas()); - t.ok(spyA.calledTwice); - t.ok(spyB.calledTwice); + t.ok(spyA.calledOnce); + t.ok(spyB.calledOnce); t.end(); }); @@ -206,7 +206,7 @@ test('Map#off distinguishes distinct layers', (t) => { map.off('click', 'B', spy); simulate.click(map.getCanvas()); - t.ok(spy.calledTwice); + t.ok(spy.calledOnce); t.end(); }); @@ -224,7 +224,7 @@ test('Map#off distinguishes distinct listeners', (t) => { map.off('click', 'layer', spyB); simulate.click(map.getCanvas()); - t.ok(spyA.calledTwice); + t.ok(spyA.calledOnce); t.ok(spyB.notCalled); t.end(); }); From f30e3ac749f7741e9ea9c4bffee2eba7cebeabed Mon Sep 17 00:00:00 2001 From: avpeery Date: Thu, 12 Aug 2021 11:58:45 -0700 Subject: [PATCH 6/9] Added documentation, fixed lint issue, as per unit test popup default behavior should be true for closeOnClick --- src/ui/handler/map_event.js | 2 +- src/ui/map.js | 3 ++- src/ui/popup.js | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ui/handler/map_event.js b/src/ui/handler/map_event.js index 555c6704ddf..fd3c8e8731c 100644 --- a/src/ui/handler/map_event.js +++ b/src/ui/handler/map_event.js @@ -1,6 +1,6 @@ // @flow -import { extend } from '../../util/util.js'; +import {extend} from '../../util/util.js'; import {MapMouseEvent, MapTouchEvent, MapWheelEvent} from '../events.js'; import type Map from '../map.js'; diff --git a/src/ui/map.js b/src/ui/map.js index 2486230b47d..e7a43881692 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -1088,6 +1088,7 @@ class Map extends Camera { * | [`mousemove`](#map.event:mousemove) | yes | * | [`mouseenter`](#map.event:mouseenter) | yes (required) | * | [`mouseleave`](#map.event:mouseleave) | yes (required) | + * | [`preclick`](#map.event:preclick) | | * | [`click`](#map.event:click) | yes | * | [`dblclick`](#map.event:dblclick) | yes | * | [`contextmenu`](#map.event:contextmenu) | yes | @@ -1196,7 +1197,7 @@ class Map extends Camera { * Adds a listener that will be called only once to a specified event type, * optionally limited to events occurring on features in a specified style layer. * - * @param {string} type The event type to listen for; one of `'mousedown'`, `'mouseup'`, `'click'`, `'dblclick'`, + * @param {string} type The event type to listen for; one of `'mousedown'`, `'mouseup'`, `'preclick'`, `'click'`, `'dblclick'`, * `'mousemove'`, `'mouseenter'`, `'mouseleave'`, `'mouseover'`, `'mouseout'`, `'contextmenu'`, `'touchstart'`, * `'touchend'`, or `'touchcancel'`. `mouseenter` and `mouseover` events are triggered when the cursor enters * a visible portion of the specified layer from outside that layer or outside the map canvas. `mouseleave` diff --git a/src/ui/popup.js b/src/ui/popup.js index 2ad74604437..5af467cfc42 100644 --- a/src/ui/popup.js +++ b/src/ui/popup.js @@ -215,7 +215,7 @@ export default class Popup extends Evented { if (this._map) { this._map.off('move', this._update); this._map.off('move', this._onClose); - this._map.off('click', this._onClose); + this._map.off('preclick', this._onClose); this._map.off('remove', this.remove); this._map.off('mousemove', this._onMouseMove); this._map.off('mouseup', this._onMouseUp); From a929463cbbe34af957627bee77cd0db7c172cfe5 Mon Sep 17 00:00:00 2001 From: avpeery Date: Thu, 12 Aug 2021 12:48:17 -0700 Subject: [PATCH 7/9] added unit test for preclick event --- src/ui/popup.js | 2 +- test/unit/ui/map_events.test.js | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/ui/popup.js b/src/ui/popup.js index 5af467cfc42..2ad74604437 100644 --- a/src/ui/popup.js +++ b/src/ui/popup.js @@ -215,7 +215,7 @@ export default class Popup extends Evented { if (this._map) { this._map.off('move', this._update); this._map.off('move', this._onClose); - this._map.off('preclick', this._onClose); + this._map.off('click', this._onClose); this._map.off('remove', this.remove); this._map.off('mousemove', this._onMouseMove); this._map.off('mouseup', this._onMouseUp); diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index ece29ad28d4..0ad7efc61c6 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -630,3 +630,30 @@ test("Map#isMoving() returns false in mousedown/mouseup/click with no movement", map.remove(); t.end(); }); + +test("Map#on click should fire preclick before click", (t) => { + const map = createMap(t); + const preclickSpy = t.spy(function (e) { + t.equal(this, map); + t.equal(e.type, 'preclick'); + }); + + const clickSpy = t.spy(function (e) { + t.equal(this, map); + t.equal(e.type, 'click'); + }); + + map.on('click', clickSpy); + map.on('preclick', preclickSpy); + map.once('preclick', function () { + t.ok(clickSpy.notCalled); + }); + + simulate.click(map.getCanvas()); + + t.ok(preclickSpy.calledOnce); + t.ok(clickSpy.calledOnce); + + map.remove(); + t.end(); +}); \ No newline at end of file From ad509763f0ea5570e80ca437ea491cf378ce7bb3 Mon Sep 17 00:00:00 2001 From: avpeery Date: Thu, 12 Aug 2021 12:53:40 -0700 Subject: [PATCH 8/9] added whitespace to unit test file --- test/unit/ui/map_events.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index 0ad7efc61c6..e55b0c857c5 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -656,4 +656,4 @@ test("Map#on click should fire preclick before click", (t) => { map.remove(); t.end(); -}); \ No newline at end of file +}); From a8465241c262bac6ad00ecc0751dadf13a4f8127 Mon Sep 17 00:00:00 2001 From: avpeery Date: Thu, 12 Aug 2021 12:59:56 -0700 Subject: [PATCH 9/9] fixed arrow expression error for lint test --- test/unit/ui/map_events.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index e55b0c857c5..d81fa777731 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -645,7 +645,7 @@ test("Map#on click should fire preclick before click", (t) => { map.on('click', clickSpy); map.on('preclick', preclickSpy); - map.once('preclick', function () { + map.once('preclick', () => { t.ok(clickSpy.notCalled); });