From e808df7a99b2aea2199ffd7e2b71c21f6d4f7ad1 Mon Sep 17 00:00:00 2001
From: Mathieu Puech <mathieu.puech@ubisoft.com>
Date: Wed, 2 Dec 2020 00:43:12 -0500
Subject: [PATCH] fix(listbox): become active on click

---
 packages/listbox/README.md                    |  2 +-
 packages/listbox/src/LionOption.js            |  2 ++
 packages/listbox/src/ListboxMixin.js          |  4 ++-
 .../listbox/test-suites/ListboxMixin.suite.js | 26 +++++++++++++++++--
 packages/listbox/test/lion-option.test.js     |  4 ++-
 5 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/packages/listbox/README.md b/packages/listbox/README.md
index b1dbe0d8f..befcade63 100644
--- a/packages/listbox/README.md
+++ b/packages/listbox/README.md
@@ -131,7 +131,7 @@ See [wai aria spec](https://www.w3.org/TR/wai-aria-practices/#kbd_selection_foll
 export const selectionFollowsFocus = () => html`
   <lion-listbox name="combo" label="Selection follows focus" selection-follows-focus>
     <lion-option .choiceValue=${'Apple'}>Apple</lion-option>
-    <lion-option .choiceValue=${'Artichoke'}>Artichoke</lion-option>
+    <lion-option .choiceValue=${'Artichoke'} disabled>Artichoke</lion-option>
     <lion-option .choiceValue=${'Asparagus'}>Asparagus</lion-option>
     <lion-option .choiceValue=${'Banana'}>Banana</lion-option>
     <lion-option .choiceValue=${'Beets'}>Beets</lion-option>
diff --git a/packages/listbox/src/LionOption.js b/packages/listbox/src/LionOption.js
index 02b0e2c8e..27e676d81 100644
--- a/packages/listbox/src/LionOption.js
+++ b/packages/listbox/src/LionOption.js
@@ -125,8 +125,10 @@ export class LionOption extends DisabledMixin(ChoiceInputMixin(FormRegisteringMi
     const parentForm = /** @type {unknown} */ (this.__parentFormGroup);
     if (parentForm && /** @type {ChoiceGroupHost} */ (parentForm).multipleChoice) {
       this.checked = !this.checked;
+      this.active = !this.active;
     } else {
       this.checked = true;
+      this.active = true;
     }
   }
 }
diff --git a/packages/listbox/src/ListboxMixin.js b/packages/listbox/src/ListboxMixin.js
index 6b3e697a3..33fe22b76 100644
--- a/packages/listbox/src/ListboxMixin.js
+++ b/packages/listbox/src/ListboxMixin.js
@@ -362,7 +362,9 @@ const ListboxMixinImplementation = superclass =>
           this._uncheckChildren();
         }
         if (this.formElements[index]) {
-          if (this.multipleChoice) {
+          if (this.formElements[index].disabled) {
+            this._uncheckChildren();
+          } else if (this.multipleChoice) {
             this.formElements[index].checked = !this.formElements[index].checked;
           } else {
             this.formElements[index].checked = true;
diff --git a/packages/listbox/test-suites/ListboxMixin.suite.js b/packages/listbox/test-suites/ListboxMixin.suite.js
index 74148196c..f57bce38d 100644
--- a/packages/listbox/test-suites/ListboxMixin.suite.js
+++ b/packages/listbox/test-suites/ListboxMixin.suite.js
@@ -912,7 +912,7 @@ export function runListboxMixinSuite(customConfig = {}) {
             });
           }
           const el = /** @type {LionListbox} */ (await fixture(html`
-              <${tag} opened selection-follows-focus orientation="horizontal" autocomplete="none">
+              <${tag} opened selection-follows-focus autocomplete="none">
                   <${optionTag} .choiceValue=${10}>Item 1</${optionTag}>
                   <${optionTag} .choiceValue=${20}>Item 2</${optionTag}>
                   <${optionTag} .choiceValue=${30}>Item 3</${optionTag}>
@@ -951,7 +951,7 @@ export function runListboxMixinSuite(customConfig = {}) {
             });
           }
           const el = /** @type {LionListbox} */ (await fixture(html`
-              <${tag} opened selection-follows-focus autocomplete="none">
+              <${tag} opened selection-follows-focus orientation="horizontal" autocomplete="none">
                   <${optionTag} .choiceValue=${10}>Item 1</${optionTag}>
                   <${optionTag} .choiceValue=${20}>Item 2</${optionTag}>
                   <${optionTag} .choiceValue=${30}>Item 3</${optionTag}>
@@ -1075,6 +1075,28 @@ export function runListboxMixinSuite(customConfig = {}) {
           // Checked index stays where it was
           expect(el.checkedIndex).to.equal(0);
         });
+        it('does not check disabled options when selection-follow-focus is enabled', async () => {
+          const el = await fixture(html`
+            <${tag} opened autocomplete="inline" .selectionFollowsFocus="${true}">
+              <${optionTag} .choiceValue=${'Item 1'} checked>Item 1</${optionTag}>
+              <${optionTag} .choiceValue=${'Item 2'} disabled>Item 2</${optionTag}>
+              <${optionTag} .choiceValue=${'Item 3'}>Item 3</${optionTag}>
+            </${tag}>
+          `);
+
+          const { listbox } = getProtectedMembers(el);
+
+          // Normalize activeIndex across multiple implementers of ListboxMixinSuite
+          el.activeIndex = 0;
+
+          listbox.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown' }));
+          expect(el.activeIndex).to.equal(1);
+          expect(el.checkedIndex).to.equal(-1);
+
+          listbox.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown' }));
+          expect(el.activeIndex).to.equal(2);
+          expect(el.checkedIndex).to.equal(2);
+        });
       });
 
       describe('Programmatic interaction', () => {
diff --git a/packages/listbox/test/lion-option.test.js b/packages/listbox/test/lion-option.test.js
index f6c8edb2d..441482682 100644
--- a/packages/listbox/test/lion-option.test.js
+++ b/packages/listbox/test/lion-option.test.js
@@ -86,14 +86,16 @@ describe('lion-option', () => {
       expect(el.hasAttribute('active')).to.be.false;
     });
 
-    it('does become checked on [click]', async () => {
+    it('does become checked and active on [click]', async () => {
       const el = /** @type {LionOption} */ (await fixture(
         html`<lion-option .choiceValue=${10}></lion-option>`,
       ));
       expect(el.checked).to.be.false;
+      expect(el.active).to.be.false;
       el.click();
       await el.updateComplete;
       expect(el.checked).to.be.true;
+      expect(el.active).to.be.true;
     });
 
     it('fires active-changed event', async () => {