Handles firing closeOnClick before another listener (#10926)

* Initial fix of force firing of onClose to happen first if multiple event listeners

* removed unused added importand extra space

* changed == to ===

* Implemented preclick solution and adjusted map event tests to reflect preclick call on click events

* added sythetic preclick, WIP: closeonclick listener not activating

* Added documentation, fixed lint issue, as per unit test popup default behavior should be true for closeOnClick

* added unit test for preclick event

* added whitespace to unit test file

* fixed arrow expression error for lint test

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.
      *

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';
 
@@ -38,8 +39,15 @@ export class MapEventHandler {
         this._map.fire(new MapMouseEvent(e.type, this._map, e));
     }
 
+    preclick(e: MouseEvent) {
+        const synth = extend({}, e);
+        synth.type = 'preclick';
+        this._map.fire(new MapMouseEvent(synth.type, this._map, synth));
+    }
+
     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));
     }
 

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`

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) {

src/util/evented.js

@@ -124,6 +124,7 @@ 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() : [];
+
             for (const listener of listeners) {
                 listener.call(this, event);
             }

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', () => {
+        t.ok(clickSpy.notCalled);
+    });
+
+    simulate.click(map.getCanvas());
+
+    t.ok(preclickSpy.calledOnce);
+    t.ok(clickSpy.calledOnce);
+
+    map.remove();
+    t.end();
+});