From d1c6b18c0ee177227ba0c471c79cbb6e036265bf Mon Sep 17 00:00:00 2001
From: Thijs Louisse <Thijs.Louisse@ing.com>
Date: Fri, 2 Oct 2020 11:25:35 +0200
Subject: [PATCH] fix(listbox): no cancellation multi mouse click

---
 .changeset/eighty-planes-build.md             |  5 ++
 packages/listbox/src/ListboxMixin.js          | 20 ++++---
 .../listbox/test-suites/ListboxMixin.suite.js | 52 +++++++++++++++++++
 3 files changed, 71 insertions(+), 6 deletions(-)
 create mode 100644 .changeset/eighty-planes-build.md

diff --git a/.changeset/eighty-planes-build.md b/.changeset/eighty-planes-build.md
new file mode 100644
index 000000000..030a1df7c
--- /dev/null
+++ b/.changeset/eighty-planes-build.md
@@ -0,0 +1,5 @@
+---
+'@lion/listbox': patch
+---
+
+no cancellation multi mouse click
diff --git a/packages/listbox/src/ListboxMixin.js b/packages/listbox/src/ListboxMixin.js
index ca6d5f222..79e3020e7 100644
--- a/packages/listbox/src/ListboxMixin.js
+++ b/packages/listbox/src/ListboxMixin.js
@@ -537,12 +537,20 @@ const ListboxMixinImplementation = superclass =>
      */
     // eslint-disable-next-line class-methods-use-this, no-unused-vars
     _listboxOnClick(ev) {
-      const option = /** @type {HTMLElement} */ (ev.target).closest('[role=option]');
-      const foundIndex = this.formElements.indexOf(option);
-      if (foundIndex > -1) {
-        this.activeIndex = foundIndex;
-        this.setCheckedIndex(foundIndex);
-      }
+      /**
+      For now we disable, handling click interactions via delegated click handlers, since
+      LionOption detects clicks itself and could now potentially be checked as offline dom.
+      When we enable both methods in multiple choice mode, we would 'cancel out' each other.
+      TODO: If we want to allow 'less smart' options (think of role=menu), we should enable this
+      again and disable click handling inside LionOption (since there would be no use case for
+      'orphan' options)
+       */
+      // const option = /** @type {HTMLElement} */ (ev.target).closest('[role=option]');
+      // const foundIndex = this.formElements.indexOf(option);
+      // if (foundIndex > -1) {
+      //   this.activeIndex = foundIndex;
+      //   this.setCheckedIndex(foundIndex, 'toggle', );
+      // }
     }
 
     /**
diff --git a/packages/listbox/test-suites/ListboxMixin.suite.js b/packages/listbox/test-suites/ListboxMixin.suite.js
index 43e60f258..02536d192 100644
--- a/packages/listbox/test-suites/ListboxMixin.suite.js
+++ b/packages/listbox/test-suites/ListboxMixin.suite.js
@@ -476,6 +476,7 @@ export function runListboxMixinSuite(customConfig = {}) {
             expect(options[1].checked).to.be.true;
           });
         });
+
         describe('Space', () => {
           it('selects active option when "_listboxReceivesNoFocus" is true', async () => {
             // When listbox is not focusable (in case of a combobox), the user should be allowed
@@ -667,6 +668,57 @@ export function runListboxMixinSuite(customConfig = {}) {
           expect(el.modelValue).to.eql(['Artichoke', 'Chard']);
         });
 
+        it('works via different interaction mechanisms (click, enter, spaces)', async () => {
+          const el = /** @type {Listbox} */ (await fixture(html`
+            <${tag} name="foo" multiple-choice>
+              <${optionTag} .choiceValue="${'Artichoke'}">Artichoke</${optionTag}>
+              <${optionTag} .choiceValue="${'Chard'}">Chard</${optionTag}>
+              <${optionTag} .choiceValue="${'Chicory'}">Chicory</${optionTag}>
+              <${optionTag} .choiceValue="${'Victoria Plum'}">Victoria Plum</${optionTag}>
+            </${tag}>
+          `));
+          const options = el.formElements;
+
+          // feature detection select-rich
+          if (el.navigateWithinInvoker !== undefined) {
+            // Note we don't have multipleChoice in the select-rich yet.
+            // TODO: implement in future when requested
+            return;
+          }
+
+          // click
+          options[0].click();
+          options[1].click();
+          expect(options[0].checked).to.equal(true);
+          expect(el.modelValue).to.eql(['Artichoke', 'Chard']);
+
+          // Reset
+          el._uncheckChildren();
+
+          // Enter
+          el.activeIndex = 0;
+          el._listboxNode.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));
+          el.activeIndex = 1;
+          el._listboxNode.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));
+          expect(options[0].checked).to.equal(true);
+          expect(el.modelValue).to.eql(['Artichoke', 'Chard']);
+
+          if (el._listboxReceivesNoFocus) {
+            return; // if suite is run for combobox, we don't respond to [Space]
+          }
+
+          // Reset
+          el._uncheckChildren();
+
+          // Space
+          el.activeIndex = 0;
+          el._listboxNode.dispatchEvent(new KeyboardEvent('keydown', { key: ' ' }));
+          el.activeIndex = 1;
+          el._listboxNode.dispatchEvent(new KeyboardEvent('keydown', { key: ' ' }));
+          expect(options[0].checked).to.equal(true);
+          expect(el.modelValue).to.eql(['Artichoke', 'Chard']);
+        });
+
         describe('Accessibility', () => {
           it('adds aria-multiselectable="true" to listbox node', async () => {
             const el = /** @type {Listbox} */ (await fixture(html`