Fix indeterminate checkbox edge cases (#2458)

* Add failing tests

* Fix the issues with indeterminate checkbox

* Update the fix

* Clean up the code

* Add changeset

* Fix a type issue

* Refactor the code

* Fix a bug regarding disabled checked sub checkbox

* Add test case where subCheckboxes are added and removed dynamically

* Make a few node reference optional as it spits undefined error when the element is removed

.changeset/good-squids-love.md

@@ -0,0 +1,5 @@
+---
+'@lion/ui': patch
+---
+
+[lion-checkbox-indeterminate] Fix bugs regarding disabled and pre-checked children

packages/ui/components/checkbox-group/src/LionCheckboxIndeterminate.js

@@ -49,17 +49,7 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
    * @protected
    */
   get _subCheckboxes() {
-    let checkboxes = [];
+    return /** @type LionCheckbox[] */ (this.__subCheckboxes);
-    if (
-      this._checkboxGroupNode &&
-      this._checkboxGroupNode.formElements &&
-      this._checkboxGroupNode.formElements.length > 0
-    ) {
-      checkboxes = this._checkboxGroupNode.formElements.filter(
-        checkbox => checkbox !== this && this.contains(checkbox),
-      );
-    }
-    return /** @type LionCheckbox[] */ (checkboxes);
   }
 
   _storeIndeterminateState() {
@@ -97,9 +87,14 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
         this.indeterminate = false;
         this.checked = false;
         break;
-      default:
+      default: {
         this.indeterminate = true;
-        this.checked = false;
+        const disabledUncheckedElements = subCheckboxes.filter(
+          checkbox => checkbox.disabled && checkbox.checked === false,
+        );
+        this.checked =
+          subCheckboxes.length - checkedElements.length - disabledUncheckedElements.length === 0;
+      }
     }
     this.updateComplete.then(() => {
       this.__settingOwnChecked = false;
@@ -155,13 +150,12 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
         subCheckboxes.length > 0 && subCheckboxes.length === checkedElements.length;
       const allDisabled =
         subCheckboxes.length > 0 && subCheckboxes.length === disabledElements.length;
-      const hasDisabledElements = disabledElements.length > 0;
 
       if (allDisabled) {
         this.checked = allChecked;
       }
 
-      if (this.indeterminate && (this.mixedState || hasDisabledElements)) {
+      if (this.indeterminate && this.mixedState) {
         this._subCheckboxes.forEach((checkbox, i) => {
           // eslint-disable-next-line no-param-reassign
           checkbox.checked = this._indeterminateSubStates[i];
@@ -211,6 +205,7 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
     if (!(/** @type {HTMLElement} */ (ev.target).hasAttribute('role'))) {
       /** @type {HTMLElement} */ (ev.target)?.setAttribute('role', 'listitem');
     }
+    this.__addToSubCheckboxes(/** @type {CustomEvent} */ (ev).detail.element);
     this._setOwnCheckedState();
   }
 
@@ -223,6 +218,26 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
     if (/** @type {HTMLElement} */ (ev.target).getAttribute('role') === 'listitem') {
       /** @type {HTMLElement} */ (ev.target)?.removeAttribute('role');
     }
+    this.__removeFromSubCheckboxes(/** @type {CustomEvent} */ (ev).detail.element);
+  }
+
+  /**
+   * @param {HTMLElement} element
+   */
+  __addToSubCheckboxes(element) {
+    if (element !== this && this.contains(element)) {
+      this.__subCheckboxes.push(element);
+    }
+  }
+
+  /**
+   * @param {HTMLElement} element
+   */
+  __removeFromSubCheckboxes(element) {
+    const index = this.__subCheckboxes.indexOf(element);
+    if (index !== -1) {
+      this.__subCheckboxes.splice(index, 1);
+    }
   }
 
   constructor() {
@@ -230,6 +245,8 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
     this.indeterminate = false;
     this._onRequestToAddFormElement = this._onRequestToAddFormElement.bind(this);
     this.__onModelValueChanged = this.__onModelValueChanged.bind(this);
+    /** @type {HTMLElement[]} */
+    this.__subCheckboxes = [];
     /** @type {boolean[]} */
     this._indeterminateSubStates = [];
     this.mixedState = false;
@@ -259,7 +276,13 @@ export class LionCheckboxIndeterminate extends LionCheckbox {
   /** @param {import('lit-element').PropertyValues } changedProperties */
   updated(changedProperties) {
     super.updated(changedProperties);
-    if (changedProperties.has('indeterminate')) {
+
+    // When 1. sub checkboxes have disabled elements and 2. some elements are checked while the others are unchecked
+    // both this._inputNode.indeterminate and this.indeterminate are already true.
+    // If user clicks the input node, this._inputNode.indeterminate is turned to false by the browser
+    // while this.indeterminate is still true and the 'indeterminate' is not in the changedProperties
+    // because it hasn't been updated (true -> true) but checked would have been updated (false -> true).
+    if (changedProperties.has('indeterminate') || changedProperties.has('checked')) {
       this._inputNode.indeterminate = this.indeterminate;
     }
   }

packages/ui/components/checkbox-group/test-suites/CheckboxIndeterminate.suite.js

@@ -205,7 +205,84 @@ export function runCheckboxIndeterminateSuite(customConfig) {
 
       // Assert
       expect(elIndeterminate?.hasAttribute('indeterminate')).to.be.true;
-      expect(elIndeterminate?.checked).to.be.false;
+      expect(elIndeterminate?.checked).to.be.true;
+    });
+
+    it('should be checked when all children are prechecked', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ await fixture(html`
+      <${groupTag} name="scientists[]">
+        <${tag} label="Favorite scientists">
+          <${childTag} checked label="Archimedes"></${childTag}>
+          <${childTag} checked label="Francis Bacon"></${childTag}>
+          <${childTag} checked label="Marie Curie"></${childTag}>
+        </${tag}>
+      </${groupTag}>
+    `);
+
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+
+      // Assert
+      expect(elIndeterminate?.hasAttribute('checked')).to.be.true;
+    });
+
+    it('should be checked when it has one prechecked child', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ await fixture(html`
+      <${groupTag} name="scientists[]">
+        <${tag} label="Favorite scientists">
+          <${childTag} checked label="Archimedes"></${childTag}>
+        </${tag}>
+      </${groupTag}>
+    `);
+
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+
+      // Assert
+      expect(elIndeterminate?.hasAttribute('checked')).to.be.true;
+    });
+
+    it('should be unchecked when it has an disabled checked child and an unchecked child', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ await fixture(html`
+      <${groupTag} name="scientists[]">
+        <${tag} label="Favorite scientists">
+          <${childTag} disabled checked label="Archimedes"></${childTag}>
+          <${childTag} label="Francis Bacon"></${childTag}>
+        </${tag}>
+      </${groupTag}>
+    `);
+
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+
+      // Assert
+      expect(elIndeterminate?.hasAttribute('checked')).to.be.false;
+    });
+
+    it('should be indeterminated when some of the children are prechecked', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ await fixture(html`
+      <${groupTag} name="scientists[]">
+        <${tag} label="Favorite scientists">
+          <${childTag} checked label="Archimedes"></${childTag}>
+          <${childTag} checked label="Francis Bacon"></${childTag}>
+          <${childTag} label="Marie Curie"></${childTag}>
+        </${tag}>
+      </${groupTag}>
+    `);
+
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+
+      // Assert
+      expect(elIndeterminate.hasAttribute('indeterminate')).to.be.true;
     });
 
     it('should sync all children when parent is checked (from indeterminate to checked)', async () => {
@@ -384,6 +461,65 @@ export function runCheckboxIndeterminateSuite(customConfig) {
       expect(_subCheckboxes[2].hasAttribute('checked')).to.be.false;
     });
 
+    it('should sync all prechecked children when parent is indeterminate and some of the children are disabled (from checked to unchecked)', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ (
+        await fixture(html`
+        <${groupTag} name="scientists[]">
+          <${tag} label="Favorite scientists">
+            <${childTag} checked label="Archimedes"></${childTag}>
+            <${childTag} disabled label="Francis Bacon"></${childTag}>
+            <${childTag} checked label="Marie Curie"></${childTag}>
+          </${tag}>
+        </${groupTag}>
+      `)
+      );
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+      const { _subCheckboxes, _inputNode } = getCheckboxIndeterminateMembers(elIndeterminate);
+
+      // Act
+      _inputNode.click();
+      await elIndeterminate.updateComplete;
+
+      // Assert
+      expect(elIndeterminate?.hasAttribute('indeterminate')).to.be.false;
+      expect(_inputNode?.hasAttribute('indeterminate')).to.be.false;
+      expect(_subCheckboxes[0].hasAttribute('checked')).to.be.false;
+      expect(_subCheckboxes[1].hasAttribute('checked')).to.be.false;
+      expect(_subCheckboxes[2].hasAttribute('checked')).to.be.false;
+    });
+
+    it('should stay as indeterminated after it is clicked, when it is interminated already and some children are disabled', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ (
+        await fixture(html`
+        <${groupTag} name="scientists[]">
+          <${tag} label="Favorite scientists">
+            <${childTag} checked label="Archimedes"></${childTag}>
+            <${childTag} disabled label="Francis Bacon"></${childTag}>
+            <${childTag} label="Marie Curie"></${childTag}>
+          </${tag}>
+        </${groupTag}>
+      `)
+      );
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+      const { _subCheckboxes, _inputNode } = getCheckboxIndeterminateMembers(elIndeterminate);
+
+      // Act
+      _inputNode.click();
+      await elIndeterminate.updateComplete;
+
+      // Assert
+      expect(elIndeterminate.hasAttribute('indeterminate')).to.be.true;
+      expect(_inputNode.indeterminate).to.be.true;
+      expect(_subCheckboxes[0].hasAttribute('checked')).to.be.true;
+      expect(_subCheckboxes[2].hasAttribute('checked')).to.be.true;
+    });
+
     it('should work as expected with siblings checkbox-indeterminate', async () => {
       // Arrange
       const el = /**  @type {LionCheckboxGroup} */ (
@@ -450,6 +586,58 @@ export function runCheckboxIndeterminateSuite(customConfig) {
       expect(elSecondSubCheckboxes._subCheckboxes[1].hasAttribute('checked')).to.be.false;
     });
 
+    it('should work as expected when new checkbox was added', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ await fixture(html`
+      <${groupTag} name="scientists[]">
+        <${tag} label="Favorite scientists">
+          <${childTag} checked label="Archimedes"></${childTag}>
+          <${childTag} checked label="Francis Bacon" checked></${childTag}>
+          <${childTag} checked label="Marie Curie"></${childTag}>
+        </${tag}>
+      </${groupTag}>
+    `);
+
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+      expect(elIndeterminate.hasAttribute('indeterminate')).to.be.false;
+
+      // Act
+      const newChild = document.createElement(/** @type {string} */ (cfg.childTagString));
+      elIndeterminate.appendChild(newChild);
+      await elIndeterminate.updateComplete;
+
+      // Assert
+      expect(elIndeterminate.hasAttribute('indeterminate')).to.be.true;
+    });
+
+    it('should work as expected when an existing checkbox was removed', async () => {
+      // Arrange
+      const el = /**  @type {LionCheckboxGroup} */ await fixture(html`
+      <${groupTag} name="scientists[]">
+        <${tag} label="Favorite scientists">
+          <${childTag} checked label="Archimedes"></${childTag}>
+          <${childTag} checked label="Francis Bacon" checked></${childTag}>
+          <${childTag} label="Marie Curie"></${childTag}>
+        </${tag}>
+      </${groupTag}>
+    `);
+
+      const elIndeterminate = /**  @type {LionCheckboxIndeterminate} */ (
+        el.querySelector(`${cfg.tagString}`)
+      );
+      expect(elIndeterminate.hasAttribute('indeterminate')).to.be.true;
+      expect(elIndeterminate.checked).to.be.false;
+
+      // Act
+      elIndeterminate.removeChild(/** @type {ChildNode} */ (elIndeterminate.lastChild));
+      await elIndeterminate.updateComplete;
+
+      // Assert
+      expect(elIndeterminate?.hasAttribute('indeterminate')).to.be.true;
+    });
+
     it('should work as expected with nested indeterminate checkboxes', async () => {
       // Arrange
       const el = /**  @type {LionCheckboxGroup} */ (

packages/ui/components/form-core/src/FocusMixin.js

@@ -198,19 +198,19 @@ const FocusMixinImplementation = superclass =>
      * @private
      */
     __teardownEventsForFocusMixin() {
-      this._focusableNode.removeEventListener(
+      this._focusableNode?.removeEventListener(
         'focus',
         /** @type {EventListenerOrEventListenerObject} */ (this.__redispatchFocus),
       );
-      this._focusableNode.removeEventListener(
+      this._focusableNode?.removeEventListener(
         'blur',
         /** @type {EventListenerOrEventListenerObject} */ (this.__redispatchBlur),
       );
-      this._focusableNode.removeEventListener(
+      this._focusableNode?.removeEventListener(
         'focusin',
         /** @type {EventListenerOrEventListenerObject} */ (this.__redispatchFocusin),
       );
-      this._focusableNode.removeEventListener(
+      this._focusableNode?.removeEventListener(
         'focusout',
         /** @type {EventListenerOrEventListenerObject} */ (this.__redispatchFocusout),
       );

packages/ui/components/form-core/src/LionField.js

@@ -44,7 +44,7 @@ export class LionField extends FormControlMixin(
 
   disconnectedCallback() {
     super.disconnectedCallback();
-    this._inputNode.removeEventListener('change', this._onChange);
+    this._inputNode?.removeEventListener('change', this._onChange);
   }
 
   resetInteractionState() {