From ea4dfb748eb994f754db2a29b496192a81910ec7 Mon Sep 17 00:00:00 2001
From: Xiaoji Chen <Pessimistress@users.noreply.github.com>
Date: Tue, 7 Sep 2021 09:45:48 -0700
Subject: [PATCH] Fix Popup closeOnClick behavior (#1565)

---
 docs/api-reference/popup.md |  2 +-
 src/components/popup.js     | 55 ++++++++++++++++++++++---------------
 2 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/docs/api-reference/popup.md b/docs/api-reference/popup.md
index fbc912b94..29479e748 100644
--- a/docs/api-reference/popup.md
+++ b/docs/api-reference/popup.md
@@ -71,7 +71,7 @@ If `true`, a close button will appear in the top right corner of the popup.
 
 - default: `true`
 
-If `true`, the popup will closed when the map is clicked.
+If `true`, the popup will be closed when the map is clicked.
 
 ##### `tipSize` (Number)
 
diff --git a/src/components/popup.js b/src/components/popup.js
index cf87ec54a..9d99ba075 100644
--- a/src/components/popup.js
+++ b/src/components/popup.js
@@ -121,21 +121,6 @@ function getContainerStyle(props, viewport, el, [x, y, z], positionType) {
   return style;
 }
 
-function onClick(evt, {props, context}) {
-  if (props.closeOnClick || evt.target.className === 'mapboxgl-popup-close-button') {
-    props.onClose();
-
-    if (context.eventManager) {
-      // Using with InteractiveMap
-      // After we call `onClose` on `anyclick`, this component will be unmounted
-      // at which point we unregister the event listeners and stop blocking propagation.
-      // Then after a short delay a `click` event will fire
-      // Attach a one-time event listener here to prevent it from triggering `onClick` of the base map
-      context.eventManager.once('click', e => e.stopPropagation(), evt.target);
-    }
-  }
-}
-
 /*
  * PureComponent doesn't update when context changes.
  * The only way is to implement our own shouldComponentUpdate here. Considering
@@ -145,7 +130,7 @@ function onClick(evt, {props, context}) {
  */
 function Popup(props) {
   const contentRef = useRef(null);
-  const thisRef = useMapControl({...props, onClick});
+  const thisRef = useMapControl(props);
   const {context, containerRef} = thisRef;
   const [, setLoaded] = useState(false);
 
@@ -154,6 +139,18 @@ function Popup(props) {
     setLoaded(true);
   }, [contentRef.current]);
 
+  useEffect(() => {
+    if (context.eventManager && props.closeOnClick) {
+      const clickCallback = () => thisRef.props.onClose();
+      context.eventManager.on('anyclick', clickCallback);
+
+      return () => {
+        context.eventManager.off('anyclick', clickCallback);
+      };
+    }
+    return undefined;
+  }, [context.eventManager, props.closeOnClick]);
+
   const {viewport, map} = context;
   const {className, longitude, latitude, tipSize, closeButton, children} = props;
 
@@ -173,10 +170,19 @@ function Popup(props) {
     positionType
   );
 
-  // If eventManager does not exist (using with static map), listen to React event
-  const onReactClick = useCallback(e => !context.eventManager && onClick(e, thisRef), [
-    context.eventManager
-  ]);
+  const onClickCloseButton = useCallback(evt => {
+    thisRef.props.onClose();
+
+    const {eventManager} = thisRef.context;
+    if (eventManager) {
+      // Using with InteractiveMap
+      // After we call `onClose` on `anyclick`, this component will be unmounted
+      // at which point we unregister the event listeners and stop blocking propagation.
+      // Then after a short delay a `click` event will fire
+      // Attach a one-time event listener here to prevent it from triggering `onClick` of the base map
+      eventManager.once('click', e => e.stopPropagation(), evt.target);
+    }
+  }, []);
 
   return (
     <div
@@ -186,9 +192,14 @@ function Popup(props) {
       ref={containerRef}
     >
       <div key="tip" className="mapboxgl-popup-tip" style={{borderWidth: tipSize}} />
-      <div key="content" ref={contentRef} className="mapboxgl-popup-content" onClick={onReactClick}>
+      <div key="content" ref={contentRef} className="mapboxgl-popup-content">
         {closeButton && (
-          <button key="close-button" className="mapboxgl-popup-close-button" type="button">
+          <button
+            key="close-button"
+            className="mapboxgl-popup-close-button"
+            type="button"
+            onClick={onClickCloseButton}
+          >
             ×
           </button>
         )}