fix(overlays): enhanced and documented closeOnOutsideClick

tlouisse · Knorgias · tlouisse · commit 412270fa1a67 · 2021-03-18T15:38:38.000+01:00
Co-authored-by: Konstantinos Norgias &lt;Konstantinos.Norgias@ing.com&gt;
diff --git a/.changeset/forty-taxis-lay.md b/.changeset/forty-taxis-lay.md
@@ -0,0 +1,5 @@
+---
+'@lion/overlays': patch
+---
+
+fix: only close an overlay if both mousedown and mousep events are outside (for hidesOnOutsideClick)
diff --git a/docs/docs/systems/overlays/configuration.md b/docs/docs/systems/overlays/configuration.md
@@ -167,7 +167,8 @@ export const hidesOnOutsideClick = () => {
     <demo-overlay-system .config=${hidesOnOutsideClickConfig}>
       <button slot="invoker">Click me to open the overlay!</button>
       <div slot="content" class="demo-overlay">
-        Hello! You can close this notification here:
+        <label for="myInput">Clicking this label should not trigger close</label>
+        <input id="myInput" />
         <button
           class="close-button"
           @click=${e => e.target.dispatchEvent(new Event('close-overlay', { bubbles: true }))}
diff --git a/packages/input-datepicker/test/lion-input-datepicker.test.js b/packages/input-datepicker/test/lion-input-datepicker.test.js
@@ -2,6 +2,7 @@ import { LionCalendar, isSameDate } from '@lion/calendar';
 import { html, LitElement } from '@lion/core';
 import { IsDateDisabled, MaxDate, MinDate, MinMaxDate } from '@lion/form-core';
 import { aTimeout, defineCE, expect, fixture as _fixture, nextFrame } from '@open-wc/testing';
+import { mimicClick } from '@lion/overlays/test-helpers';
 import sinon from 'sinon';
 import { setViewport } from '@web/test-runner-commands';
 import '@lion/input-datepicker/define';
@@ -98,13 +99,13 @@ describe('<lion-input-datepicker>', () => {
       expect(elObj.overlayController.isShown).to.equal(false);
     });
 
-    it('closes the calendar via outside click', async () => {
+    it('closes the calendar via outside click event', async () => {
       const el = await fixture(html`<lion-input-datepicker></lion-input-datepicker>`);
       const elObj = new DatepickerInputObject(el);
       await elObj.openCalendar();
       expect(elObj.overlayController.isShown).to.equal(true);
 
-      document.body.click();
+      mimicClick(document.body);
       await aTimeout(0);
       expect(elObj.overlayController.isShown).to.be.false;
     });
diff --git a/packages/overlays/package.json b/packages/overlays/package.json
@@ -53,6 +53,7 @@
     ".": "./index.js",
     "./test-suites": "./test-suites/index.js",
     "./translations/*": "./translations/*",
+    "./test-helpers": "./test-helpers.js",
     "./docs/": "./docs/"
   }
 }
diff --git a/packages/overlays/src/OverlayController.js b/packages/overlays/src/OverlayController.js
@@ -1124,55 +1124,86 @@ export class OverlayController extends EventTargetShim {
     const addOrRemoveListener = phase === 'show' ? 'addEventListener' : 'removeEventListener';
 
     if (phase === 'show') {
-      let wasClickInside = false;
-      let wasIndirectSynchronousClick = false;
-      // Handle on capture phase and remember till the next task that there was an inside click
+      /**
+       * We listen to click (more specifically mouseup and mousedown) events
+       * in their capture phase (see our tests about 3rd parties stopping event propagation).
+       * We define an outside click as follows:
+       * - both mousedown and mouseup occur outside of content or invoker
+       *
+       * This means we have the following flow:
+       * [1]. (optional) mousedown is triggered on content/invoker
+       * [2]. mouseup is triggered on document (logic will be scheduled to step 4)
+       * [3]. (optional) mouseup is triggered on content/invoker
+       * [4]. mouseup logic is executed on document (its logic is inside a timeout and is thus
+       * executed after 3)
+       * [5]. Reset all helper variables that were considered in step [4]
+       *
+       */
+
+      /** @type {boolean} */
+      let wasMouseDownInside = false;
+      /** @type {boolean} */
+      let wasMouseUpInside = false;
+
       /** @type {EventListenerOrEventListenerObject} */
-      this.__preventCloseOutsideClick = () => {
-        if (wasClickInside) {
-          // This occurs when a synchronous new click is triggered from a previous click.
-          // For instance, when we have a label pointing to an input, the platform triggers
-          // a new click on the input. Not taking this click into account, will hide the overlay
-          // in `__onCaptureHtmlClick`
-          wasIndirectSynchronousClick = true;
-        }
-        wasClickInside = true;
-        setTimeout(() => {
-          wasClickInside = false;
-          setTimeout(() => {
-            wasIndirectSynchronousClick = false;
-          });
-        });
+      this.__onInsideMouseDown = () => {
+        // [1]. was mousedown inside content or invoker
+        wasMouseDownInside = true;
+      };
+
+      this.__onInsideMouseUp = () => {
+        // [3]. was mouseup inside content or invoker
+        wasMouseUpInside = true;
       };
-      // handle on capture phase and schedule the hide if needed
+
       /** @type {EventListenerOrEventListenerObject} */
-      this.__onCaptureHtmlClick = () => {
+      this.__onDocumentMouseUp = () => {
+        // [2]. The captured mouseup goes from top of the document to bottom. We add a timeout,
+        // so that [3] can be executed before [4]
         setTimeout(() => {
-          if (wasClickInside === false && !wasIndirectSynchronousClick) {
+          // [4]. Keep open if step 1 (mousedown) or 3 (mouseup) was inside
+          if (!wasMouseDownInside && !wasMouseUpInside) {
             this.hide();
           }
+          // [5]. Reset...
+          wasMouseDownInside = false;
+          wasMouseUpInside = false;
         });
       };
     }
 
     this.contentWrapperNode[addOrRemoveListener](
-      'click',
+      'mousedown',
+      /** @type {EventListenerOrEventListenerObject} */
+      (this.__onInsideMouseDown),
+      true,
+    );
+    this.contentWrapperNode[addOrRemoveListener](
+      'mouseup',
       /** @type {EventListenerOrEventListenerObject} */
-      (this.__preventCloseOutsideClick),
+      (this.__onInsideMouseUp),
       true,
     );
     if (this.invokerNode) {
+      // An invoker click (usually resulting in toggle) should be left to a different part of
+      // the code
+      this.invokerNode[addOrRemoveListener](
+        'mousedown',
+        /** @type {EventListenerOrEventListenerObject} */
+        (this.__onInsideMouseDown),
+        true,
+      );
       this.invokerNode[addOrRemoveListener](
-        'click',
+        'mouseup',
         /** @type {EventListenerOrEventListenerObject} */
-        (this.__preventCloseOutsideClick),
+        (this.__onInsideMouseUp),
         true,
       );
     }
     document.documentElement[addOrRemoveListener](
-      'click',
+      'mouseup',
       /** @type {EventListenerOrEventListenerObject} */
-      (this.__onCaptureHtmlClick),
+      (this.__onDocumentMouseUp),
       true,
     );
   }
diff --git a/packages/overlays/test-helpers.js b/packages/overlays/test-helpers.js
@@ -0,0 +1 @@
+export { mimicClick } from './test-helpers/mimicClick.js';
diff --git a/packages/overlays/test-helpers/mimicClick.js b/packages/overlays/test-helpers/mimicClick.js
@@ -0,0 +1,21 @@
+async function sleep(t = 0) {
+  return new Promise(resolve => {
+    setTimeout(() => {
+      resolve(true);
+    }, t);
+  });
+}
+
+/**
+ * @param {HTMLElement} el
+ * @param {{isAsync?:boolean, releaseElement?: HTMLElement}} [config]
+ */
+export async function mimicClick(el, { isAsync, releaseElement } = { isAsync: false }) {
+  const releaseEl = releaseElement || el;
+  el.dispatchEvent(new MouseEvent('mousedown'));
+  if (isAsync) {
+    await sleep();
+  }
+  releaseEl.dispatchEvent(new MouseEvent('click'));
+  releaseEl.dispatchEvent(new MouseEvent('mouseup'));
+}
diff --git a/packages/overlays/test/OverlayController.test.js b/packages/overlays/test/OverlayController.test.js
@@ -8,6 +8,7 @@ import { OverlayController } from '../src/OverlayController.js';
 import { overlays } from '../src/overlays.js';
 import { keyCodes } from '../src/utils/key-codes.js';
 import { simulateTab } from '../src/utils/simulate-tab.js';
+import { mimicClick } from '../test-helpers.js';
 
 /**
  * @typedef {import('../types/OverlayConfig').OverlayConfig} OverlayConfig
@@ -451,8 +452,12 @@ describe('OverlayController', () => {
           contentNode,
         });
         await ctrl.show();
+        mimicClick(document.body);
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.false;
 
-        document.body.click();
+        await ctrl.show();
+        await mimicClick(document.body, { isAsync: true });
         await aTimeout(0);
         expect(ctrl.isShown).to.be.false;
       });
@@ -479,13 +484,56 @@ describe('OverlayController', () => {
 
         expect(ctrl.isShown).to.be.true;
 
+        // Don't hide on inside mousedown & outside mouseup
+        ctrl.contentNode.dispatchEvent(new MouseEvent('mousedown'));
+        await aTimeout(0);
+        document.body.dispatchEvent(new MouseEvent('mouseup'));
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.true;
+
         // Important to check if it can be still shown after, because we do some hacks inside
         await ctrl.hide();
         expect(ctrl.isShown).to.be.false;
         await ctrl.show();
         expect(ctrl.isShown).to.be.true;
       });
 
+      it('only hides when both mousedown and mouseup events are outside', async () => {
+        const contentNode = /** @type {HTMLElement} */ (await fixture('<div>Content</div>'));
+        const ctrl = new OverlayController({
+          ...withGlobalTestConfig(),
+          hidesOnOutsideClick: true,
+          contentNode,
+          invokerNode: /** @type {HTMLElement} */ (fixtureSync(html`
+            <div role="button" style="width: 100px; height: 20px;">Invoker</div>
+          `)),
+        });
+        await ctrl.show();
+        mimicClick(document.body, { releaseElement: contentNode });
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.true;
+
+        mimicClick(contentNode, { releaseElement: document.body });
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.true;
+
+        mimicClick(document.body, {
+          releaseElement: /** @type {HTMLElement} */ (ctrl.invokerNode),
+        });
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.true;
+
+        mimicClick(/** @type {HTMLElement} */ (ctrl.invokerNode), {
+          releaseElement: document.body,
+        });
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.true;
+
+        mimicClick(document.body);
+        await aTimeout(0);
+        expect(ctrl.isShown).to.be.false;
+      });
+
       it('doesn\'t hide on "inside sub shadow dom" click', async () => {
         const invokerNode = /** @type {HTMLElement} */ (await fixture('<button>Invoker</button>'));
         const contentNode = /** @type {HTMLElement} */ (await fixture('<div>Content</div>'));
@@ -548,31 +596,24 @@ describe('OverlayController', () => {
           contentNode,
           invokerNode,
         });
+        const stopProp = (/** @type {Event} */ e) => e.stopPropagation();
         const dom = await fixture(
-          /**
-           * @param {{ stopPropagation: () => any; }} e
-           */
           `
           <div>
             <div id="popup">${invokerNode}${contentNode}</div>
-            <div
-              id="regular-sibling"
-              @click="${() => {
-                /* propagates */
-              }}"
-            ></div>
-            <third-party-noise @click="${(/** @type {Event} */ e) => e.stopPropagation()}">
+            <div id="third-party-noise" @click="${stopProp}" @mousedown="${stopProp}" @mouseup="${stopProp}">
               This element prevents our handlers from reaching the document click handler.
-            </third-party-noise>
+            </div>
           </div>
         `,
         );
 
         await ctrl.show();
         expect(ctrl.isShown).to.equal(true);
 
-        /** @type {HTMLElement} */
-        (dom.querySelector('third-party-noise')).click();
+        const noiseEl = /** @type {HTMLElement} */ (dom.querySelector('#third-party-noise'));
+
+        mimicClick(noiseEl);
         await aTimeout(0);
         expect(ctrl.isShown).to.equal(false);
 
@@ -592,35 +633,26 @@ describe('OverlayController', () => {
           contentNode,
           invokerNode,
         });
+        const stopProp = (/** @type {Event} */ e) => e.stopPropagation();
         const dom = /** @type {HTMLElement} */ (await fixture(`
           <div>
             <div id="popup">${invokerNode}${ctrl.content}</div>
-            <div
-              id="regular-sibling"
-              @click="${() => {
-                /* propagates */
-              }}"
-            ></div>
-            <third-party-noise>
+            <div id="third-party-noise">
               This element prevents our handlers from reaching the document click handler.
-            </third-party-noise>
+            </div>
           </div>
         `));
 
-        /** @type {HTMLElement} */
-        (dom.querySelector('third-party-noise')).addEventListener(
-          'click',
-          (/** @type {Event} */ event) => {
-            event.stopPropagation();
-          },
-          true,
-        );
+        const noiseEl = /** @type {HTMLElement} */ (dom.querySelector('#third-party-noise'));
+
+        noiseEl.addEventListener('click', stopProp, true);
+        noiseEl.addEventListener('mousedown', stopProp, true);
+        noiseEl.addEventListener('mouseup', stopProp, true);
 
         await ctrl.show();
         expect(ctrl.isShown).to.equal(true);
 
-        /** @type {HTMLElement} */
-        (dom.querySelector('third-party-noise')).click();
+        mimicClick(noiseEl);
         await aTimeout(0);
         expect(ctrl.isShown).to.equal(false);
 
diff --git a/packages/select-rich/test/lion-select-rich.test.js b/packages/select-rich/test/lion-select-rich.test.js
@@ -2,6 +2,7 @@ import { LitElement } from '@lion/core';
 import { renderLitAsNode } from '@lion/helpers';
 import { OverlayController } from '@lion/overlays';
 import { LionOption } from '@lion/listbox';
+import { mimicClick } from '@lion/overlays/test-helpers';
 import {
   aTimeout,
   defineCE,
@@ -253,9 +254,8 @@ describe('lion-select-rich', () => {
       ));
 
       expect(el.opened).to.be.true;
-      // a click on the button will trigger hide on outside click
-      // which we then need to sync back to "opened"
-      outerEl.click();
+
+      mimicClick(outerEl);
       await aTimeout(0);
       expect(el.opened).to.be.false;
     });

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'@lion/overlays': patch
 +---
++
 +fix: only close an overlay if both mousedown and mousep events are outside (for hidesOnOutsideClick)
Original file line number	Diff line number	Diff line change
`@@ -53,6 +53,7 @@`
`53`	`53`	`".": "./index.js",`
`54`	`54`	`"./test-suites": "./test-suites/index.js",`
`55`	`55`	`"./translations/": "./translations/",`
	`56`	`+ "./test-helpers": "./test-helpers.js",`
`56`	`57`	`"./docs/": "./docs/"`
`57`	`58`	`}`
`58`	`59`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+export { mimicClick } from './test-helpers/mimicClick.js';`