From 22b8f24804e4d1a9e60678da59fd38785cc8298b Mon Sep 17 00:00:00 2001 From: ByoungYong Kim Date: Tue, 4 Feb 2025 11:08:57 +0100 Subject: [PATCH] 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 | 5 + .../src/LionCheckboxIndeterminate.js | 55 +++-- .../CheckboxIndeterminate.suite.js | 190 +++++++++++++++++- .../ui/components/form-core/src/FocusMixin.js | 8 +- .../ui/components/form-core/src/LionField.js | 2 +- 5 files changed, 238 insertions(+), 22 deletions(-) create mode 100644 .changeset/good-squids-love.md diff --git a/.changeset/good-squids-love.md b/.changeset/good-squids-love.md new file mode 100644 index 000000000..bf3911916 --- /dev/null +++ b/.changeset/good-squids-love.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[lion-checkbox-indeterminate] Fix bugs regarding disabled and pre-checked children diff --git a/packages/ui/components/checkbox-group/src/LionCheckboxIndeterminate.js b/packages/ui/components/checkbox-group/src/LionCheckboxIndeterminate.js index bc264fd8c..728b9d448 100644 --- a/packages/ui/components/checkbox-group/src/LionCheckboxIndeterminate.js +++ b/packages/ui/components/checkbox-group/src/LionCheckboxIndeterminate.js @@ -49,17 +49,7 @@ export class LionCheckboxIndeterminate extends LionCheckbox { * @protected */ get _subCheckboxes() { - let checkboxes = []; - 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); + return /** @type LionCheckbox[] */ (this.__subCheckboxes); } _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; } } diff --git a/packages/ui/components/checkbox-group/test-suites/CheckboxIndeterminate.suite.js b/packages/ui/components/checkbox-group/test-suites/CheckboxIndeterminate.suite.js index cf7a040a6..e847c8928 100644 --- a/packages/ui/components/checkbox-group/test-suites/CheckboxIndeterminate.suite.js +++ b/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} checked label="Francis Bacon"> + <${childTag} checked label="Marie Curie"> + + + `); + + 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"> + + + `); + + 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} label="Francis Bacon"> + + + `); + + 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} checked label="Francis Bacon"> + <${childTag} label="Marie Curie"> + + + `); + + 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} disabled label="Francis Bacon"> + <${childTag} checked label="Marie Curie"> + + + `) + ); + 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} disabled label="Francis Bacon"> + <${childTag} label="Marie Curie"> + + + `) + ); + 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} checked label="Francis Bacon" checked> + <${childTag} checked label="Marie Curie"> + + + `); + + 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} checked label="Francis Bacon" checked> + <${childTag} label="Marie Curie"> + + + `); + + 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} */ ( diff --git a/packages/ui/components/form-core/src/FocusMixin.js b/packages/ui/components/form-core/src/FocusMixin.js index 7a0a4b9e5..ec7fdbe8d 100644 --- a/packages/ui/components/form-core/src/FocusMixin.js +++ b/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), ); diff --git a/packages/ui/components/form-core/src/LionField.js b/packages/ui/components/form-core/src/LionField.js index f14702858..03fbc256b 100644 --- a/packages/ui/components/form-core/src/LionField.js +++ b/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() {