From c319b90c502adeeb07545843529273b98704e8c0 Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Mon, 9 Dec 2019 17:27:46 +0100 Subject: [PATCH 1/9] Resolves #4893 --- packages/mdc-checkbox/adapter.ts | 1 + packages/mdc-checkbox/component.ts | 9 +++++---- packages/mdc-checkbox/foundation.ts | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/mdc-checkbox/adapter.ts b/packages/mdc-checkbox/adapter.ts index e68f7f3183b..e3f039d988b 100644 --- a/packages/mdc-checkbox/adapter.ts +++ b/packages/mdc-checkbox/adapter.ts @@ -37,6 +37,7 @@ export interface MDCCheckboxAdapter { isIndeterminate(): boolean; removeClass(className: string): void; removeNativeControlAttr(attr: string): void; + setChecked(checkStatus: boolean): void; setNativeControlAttr(attr: string, value: string): void; setNativeControlDisabled(disabled: boolean): void; } diff --git a/packages/mdc-checkbox/component.ts b/packages/mdc-checkbox/component.ts index be005ecee65..99fd3dafcbd 100644 --- a/packages/mdc-checkbox/component.ts +++ b/packages/mdc-checkbox/component.ts @@ -86,20 +86,20 @@ export class MDCCheckbox extends MDCComponent implements root_!: Element; // assigned in MDCComponent constructor private readonly ripple_: MDCRipple = this.createRipple_(); - private handleChange_!: EventListener; // assigned in initialSyncWithDOM() + private handleClick_!: EventListener; // assigned in initialSyncWithDOM() private handleAnimationEnd_!: EventListener; // assigned in initialSyncWithDOM() initialSyncWithDOM() { - this.handleChange_ = () => this.foundation_.handleChange(); + this.handleClick_ = () => this.foundation_.handleClick(); this.handleAnimationEnd_ = () => this.foundation_.handleAnimationEnd(); - this.nativeControl_.addEventListener('change', this.handleChange_); + this.nativeControl_.addEventListener('click', this.handleClick_); this.listen(getCorrectEventName(window, 'animationend'), this.handleAnimationEnd_); this.installPropertyChangeHooks_(); } destroy() { this.ripple_.destroy(); - this.nativeControl_.removeEventListener('change', this.handleChange_); + this.nativeControl_.removeEventListener('click', this.handleClick_); this.unlisten(getCorrectEventName(window, 'animationend'), this.handleAnimationEnd_); this.uninstallPropertyChangeHooks_(); super.destroy(); @@ -117,6 +117,7 @@ export class MDCCheckbox extends MDCComponent implements isIndeterminate: () => this.indeterminate, removeClass: (className) => this.root_.classList.remove(className), removeNativeControlAttr: (attr) => this.nativeControl_.removeAttribute(attr), + setChecked: (checkStatus) => this.nativeControl_.checked = checkStatus, setNativeControlAttr: (attr, value) => this.nativeControl_.setAttribute(attr, value), setNativeControlDisabled: (disabled) => this.nativeControl_.disabled = disabled, }; diff --git a/packages/mdc-checkbox/foundation.ts b/packages/mdc-checkbox/foundation.ts index bd73b69cf94..48695011d03 100644 --- a/packages/mdc-checkbox/foundation.ts +++ b/packages/mdc-checkbox/foundation.ts @@ -48,12 +48,14 @@ export class MDCCheckboxFoundation extends MDCFoundation { isIndeterminate: () => false, removeClass: () => undefined, removeNativeControlAttr: () => undefined, + setChecked: () => undefined, setNativeControlAttr: () => undefined, setNativeControlDisabled: () => undefined, }; } private currentCheckState_ = strings.TRANSITION_STATE_INIT; + private previousIsChecked_ = false; private currentAnimationClass_ = ''; private animEndLatchTimer_ = 0; private enableAnimationEndHandler_ = false; @@ -64,6 +66,7 @@ export class MDCCheckboxFoundation extends MDCFoundation { init() { this.currentCheckState_ = this.determineCheckState_(); + this.previousIsChecked_ = this.adapter_.isChecked(); this.updateAriaChecked_(); this.adapter_.addClass(cssClasses.UPGRADED); } @@ -97,6 +100,18 @@ export class MDCCheckboxFoundation extends MDCFoundation { }, numbers.ANIM_END_LATCH_MS); } + /** + * Handles the click event for the checkbox + * added for IE browser to fix compatibility issue b/144588184 + */ + handleClick() { + const {TRANSITION_STATE_INDETERMINATE} = strings; + if (this.currentCheckState_ === TRANSITION_STATE_INDETERMINATE) { + this.adapter_.setChecked(!this.previousIsChecked_); + this.handleChange(); + } + } + /** * Handles the change event for the checkbox */ From cc96ce7d09fc76347c3e95b3f79d87b46d74b73e Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Tue, 10 Dec 2019 11:54:13 +0100 Subject: [PATCH 2/9] refactor(checkbox): fix check failures for pull request update readme and unit test for newly added methods in adapter and foundation pr #5316 --- packages/mdc-checkbox/README.md | 4 +++- packages/mdc-checkbox/component.ts | 2 +- packages/mdc-checkbox/foundation.ts | 2 +- test/unit/mdc-checkbox/mdc-checkbox.test.js | 12 ++++++++++++ 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/mdc-checkbox/README.md b/packages/mdc-checkbox/README.md index 05488a9e9df..7fd49e1ef18 100644 --- a/packages/mdc-checkbox/README.md +++ b/packages/mdc-checkbox/README.md @@ -182,6 +182,7 @@ Method Signature | Description `isIndeterminate() => boolean` | Returns true if the component is in the indeterminate state. `isChecked() => boolean` | Returns true if the component is checked. `hasNativeControl() => boolean` | Returns true if the input is present in the component. +`setCheck(checkStatus: boolean) => void` | Sets the check status of the component. `setNativeControlDisabled(disabled: boolean) => void` | Sets the input to disabled. `setNativeControlAttr(attr: string, value: string) => void` | Sets an HTML attribute to the given value on the native input element. `removeNativeControlAttr(attr: string) => void` | Removes an attribute from the native input element. @@ -192,4 +193,5 @@ Method Signature | Description --- | --- `setDisabled(disabled: boolean) => void` | Updates the `disabled` property on the underlying input. Does nothing when the underlying input is not present. `handleAnimationEnd() => void` | `animationend` event handler that should be applied to the root element. -`handleChange() => void` | `change` event handler that should be applied to the checkbox element. +`handleClick() => void` | `click` event handler that should be applied to the checkbox element. +`handleChange() => void` | Actions taken after check status changes. diff --git a/packages/mdc-checkbox/component.ts b/packages/mdc-checkbox/component.ts index 99fd3dafcbd..756f7789d93 100644 --- a/packages/mdc-checkbox/component.ts +++ b/packages/mdc-checkbox/component.ts @@ -117,7 +117,7 @@ export class MDCCheckbox extends MDCComponent implements isIndeterminate: () => this.indeterminate, removeClass: (className) => this.root_.classList.remove(className), removeNativeControlAttr: (attr) => this.nativeControl_.removeAttribute(attr), - setChecked: (checkStatus) => this.nativeControl_.checked = checkStatus, + setChecked: (checkStatus) => this.checked = checkStatus, setNativeControlAttr: (attr, value) => this.nativeControl_.setAttribute(attr, value), setNativeControlDisabled: (disabled) => this.nativeControl_.disabled = disabled, }; diff --git a/packages/mdc-checkbox/foundation.ts b/packages/mdc-checkbox/foundation.ts index 48695011d03..70343ceabe2 100644 --- a/packages/mdc-checkbox/foundation.ts +++ b/packages/mdc-checkbox/foundation.ts @@ -113,7 +113,7 @@ export class MDCCheckboxFoundation extends MDCFoundation { } /** - * Handles the change event for the checkbox + * Handles the actions after check status changes */ handleChange() { this.transitionCheckState_(); diff --git a/test/unit/mdc-checkbox/mdc-checkbox.test.js b/test/unit/mdc-checkbox/mdc-checkbox.test.js index 9cbe25fd4c8..b964d6149b1 100644 --- a/test/unit/mdc-checkbox/mdc-checkbox.test.js +++ b/test/unit/mdc-checkbox/mdc-checkbox.test.js @@ -280,6 +280,18 @@ test('#adapter.hasNativeControl returns true when checkbox exists', () => { assert.isTrue(component.getDefaultFoundation().adapter_.hasNativeControl()); }); +test('#adapter.setChecked returns true when checkbox is set checked', () => { + const {cb, component} = setupTest(); + component.getDefaultFoundation().adapter_.setChecked(true); + assert.isTrue(cb.checked); +}); + +test('#adapter.setChecked returns false when checkbox is set not checked', () => { + const {cb, component} = setupTest(); + component.getDefaultFoundation().adapter_.setChecked(false); + assert.isFalse(cb.checked); +}); + test('#adapter.setNativeControlDisabled returns true when checkbox is disabled', () => { const {cb, component} = setupTest(); component.getDefaultFoundation().adapter_.setNativeControlDisabled(true); From df6d0051aa7c71d28bf31b5f8f3a7e83e10e0aad Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Tue, 10 Dec 2019 11:58:57 +0100 Subject: [PATCH 3/9] refactor(checkbox): fix a typo in readme.md --- packages/mdc-checkbox/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mdc-checkbox/README.md b/packages/mdc-checkbox/README.md index 7fd49e1ef18..fbcf626dd8a 100644 --- a/packages/mdc-checkbox/README.md +++ b/packages/mdc-checkbox/README.md @@ -182,7 +182,7 @@ Method Signature | Description `isIndeterminate() => boolean` | Returns true if the component is in the indeterminate state. `isChecked() => boolean` | Returns true if the component is checked. `hasNativeControl() => boolean` | Returns true if the input is present in the component. -`setCheck(checkStatus: boolean) => void` | Sets the check status of the component. +`setChecked(checkStatus: boolean) => void` | Sets the check status of the component. `setNativeControlDisabled(disabled: boolean) => void` | Sets the input to disabled. `setNativeControlAttr(attr: string, value: string) => void` | Sets an HTML attribute to the given value on the native input element. `removeNativeControlAttr(attr: string) => void` | Removes an attribute from the native input element. From 6564d7f818a6e9b74c693340175ed380c61fa271 Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Tue, 10 Dec 2019 12:50:09 +0100 Subject: [PATCH 4/9] update unit test for latest change --- test/unit/mdc-checkbox/foundation.test.js | 35 +++++++++++---------- test/unit/mdc-checkbox/mdc-checkbox.test.js | 16 +++++----- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/test/unit/mdc-checkbox/foundation.test.js b/test/unit/mdc-checkbox/foundation.test.js index de3c1f9cae8..8c016857b9b 100644 --- a/test/unit/mdc-checkbox/foundation.test.js +++ b/test/unit/mdc-checkbox/foundation.test.js @@ -62,7 +62,7 @@ function withMockCheckboxDescriptorReturning(descriptor, runTests) { Object.defineProperty(Object, 'getOwnPropertyDescriptor', originalDesc); } -// Sets up tests which execute change events through the change handler which the foundation +// Sets up tests which execute change events through the click handler which the foundation // registers. Returns an object containing the following properties: // - foundation - The MDCCheckboxFoundation instance // - mockAdapter - The adapter given to the foundation. The adapter is pre-configured to capture @@ -72,7 +72,7 @@ function withMockCheckboxDescriptorReturning(descriptor, runTests) { // representing the state of the native control after it was changed. E.g. // `change({checked: true, indeterminate: false})` simulates a change event as the result of a checkbox // being checked. -function setupChangeHandlerTest() { +function setupClickHandlerTest() { const {foundation, mockAdapter} = setupTest(); td.when(mockAdapter.isAttachedToDOM()).thenReturn(true); @@ -82,7 +82,7 @@ function setupChangeHandlerTest() { td.when(mockAdapter.hasNativeControl()).thenReturn(true); td.when(mockAdapter.isChecked()).thenReturn(newState.checked); td.when(mockAdapter.isIndeterminate()).thenReturn(newState.indeterminate); - foundation.handleChange(); + foundation.handleClick(); }; return {foundation, mockAdapter, change}; @@ -90,8 +90,8 @@ function setupChangeHandlerTest() { function testChangeHandler(desc, changes, expectedClass, verificationOpts) { changes = Array.isArray(changes) ? changes : [changes]; - test(`changeHandler: ${desc}`, () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + test(`clickHandler: ${desc}`, () => { + const {mockAdapter, change} = setupClickHandlerTest(); changes.forEach(change); td.verify(mockAdapter.addClass(expectedClass), verificationOpts); }); @@ -114,7 +114,8 @@ test('exports numbers', () => { test('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCCheckboxFoundation, [ 'addClass', 'removeClass', 'setNativeControlAttr', 'removeNativeControlAttr', - 'forceLayout', 'isAttachedToDOM', 'isIndeterminate', 'isChecked', 'hasNativeControl', 'setNativeControlDisabled', + 'forceLayout', 'isAttachedToDOM', 'isIndeterminate', 'isChecked', 'setChecked', + 'hasNativeControl', 'setNativeControlDisabled', ]); }); @@ -242,7 +243,7 @@ testChangeHandler('no transition classes applied when no state change', [ ], cssClasses.ANIM_UNCHECKED_CHECKED, {times: 1}); test('changing from unchecked to checked adds selected class', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); change({ checked: false, indeterminate: false, @@ -255,7 +256,7 @@ test('changing from unchecked to checked adds selected class', () => { }); test('changing from unchecked to indeterminate adds selected class', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); change({ checked: false, indeterminate: false, @@ -268,7 +269,7 @@ test('changing from unchecked to indeterminate adds selected class', () => { }); test('changing from checked to unchecked removes selected class', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); change({ checked: true, indeterminate: false, @@ -281,7 +282,7 @@ test('changing from checked to unchecked removes selected class', () => { }); test('changing from indeterminate to unchecked removes selected class', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); change({ checked: false, indeterminate: true, @@ -312,7 +313,7 @@ test('animation end handler removes animation class after short delay', () => { test('animation end is debounced if event is called twice', () => { const clock = installClock(); const {ANIM_UNCHECKED_CHECKED} = cssClasses; - const {mockAdapter, foundation} = setupChangeHandlerTest(); + const {mockAdapter, foundation} = setupClickHandlerTest(); foundation.enableAnimationEndHandler_ = true; foundation.currentAnimationClass_ = ANIM_UNCHECKED_CHECKED; @@ -327,7 +328,7 @@ test('animation end is debounced if event is called twice', () => { }); test('change handler triggers layout for changes within the same frame to correctly restart anims', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); change({checked: true, indeterminate: false}); td.verify(mockAdapter.forceLayout(), {times: 0}); @@ -337,7 +338,7 @@ test('change handler triggers layout for changes within the same frame to correc }); test('change handler updates aria-checked attribute correctly.', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); change({checked: true, indeterminate: true}); td.verify(mockAdapter.setNativeControlAttr('aria-checked', 'mixed')); @@ -347,7 +348,7 @@ test('change handler updates aria-checked attribute correctly.', () => { }); test('change handler does not add animation classes when isAttachedToDOM() is falsy', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); const animClassArg = td.matchers.argThat((cls) => cls.indexOf('mdc-checkbox--anim') >= 0); td.when(mockAdapter.isAttachedToDOM()).thenReturn(false); @@ -356,17 +357,17 @@ test('change handler does not add animation classes when isAttachedToDOM() is fa }); test('change handler does not add animation classes for bogus changes (init -> unchecked)', () => { - const {mockAdapter, change} = setupChangeHandlerTest(); + const {mockAdapter, change} = setupClickHandlerTest(); const animClassArg = td.matchers.argThat((cls) => cls.indexOf('mdc-checkbox--anim') >= 0); change({checked: false, indeterminate: false}); td.verify(mockAdapter.addClass(animClassArg), {times: 0}); }); -test('change handler does not do anything if checkbox element is not found', () => { +test('click handler does not do anything if checkbox element is not found', () => { const {foundation, mockAdapter} = setupTest(); td.when(mockAdapter.hasNativeControl()).thenReturn(false); - assert.doesNotThrow(() => foundation.handleChange()); + assert.doesNotThrow(() => foundation.handleClick()); td.verify(mockAdapter.setNativeControlAttr(td.matchers.isA(String), td.matchers.isA(String)), {times: 0}); td.verify(mockAdapter.removeNativeControlAttr(td.matchers.isA(String)), {times: 0}); }); diff --git a/test/unit/mdc-checkbox/mdc-checkbox.test.js b/test/unit/mdc-checkbox/mdc-checkbox.test.js index b964d6149b1..31a0bedf315 100644 --- a/test/unit/mdc-checkbox/mdc-checkbox.test.js +++ b/test/unit/mdc-checkbox/mdc-checkbox.test.js @@ -143,11 +143,11 @@ test('get ripple returns a MDCRipple instance', () => { assert.isOk(component.ripple instanceof MDCRipple); }); -test('checkbox change event calls #foundation.handleChange', () => { +test('checkbox click event calls #foundation.handleClick', () => { const {cb, component} = setupTest(); - component.foundation_.handleChange = td.func(); - domEvents.emit(cb, 'change'); - td.verify(component.foundation_.handleChange(), {times: 1}); + component.foundation_.handleClick = td.func(); + domEvents.emit(cb, 'click'); + td.verify(component.foundation_.handleClick(), {times: 1}); }); test('root animationend event calls #foundation.handleAnimationEnd', () => { @@ -169,12 +169,12 @@ test('"indeterminate" property change hook calls foundation#handleChange', () => td.verify(mockFoundation.handleChange(), {times: 1}); }); -test('checkbox change event handler is destroyed on #destroy', () => { +test('checkbox click event handler is destroyed on #destroy', () => { const {cb, component} = setupTest(); - component.foundation_.handleChange = td.func(); + component.foundation_.handleClick = td.func(); component.destroy(); - domEvents.emit(cb, 'change'); - td.verify(component.foundation_.handleChange(), {times: 0}); + domEvents.emit(cb, 'click'); + td.verify(component.foundation_.handleClick(), {times: 0}); }); test('root animationend event handler is destroyed on #destroy', () => { From 5ac3b0b4e1b1a4af4b15ff6faf015b0c55fdfd5b Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Wed, 11 Dec 2019 11:40:04 +0100 Subject: [PATCH 5/9] refactor(checkbox): fix a logic bug in previous commit and update test method names --- packages/mdc-checkbox/foundation.ts | 2 +- test/unit/mdc-checkbox/foundation.test.js | 28 +++++++++++------------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/mdc-checkbox/foundation.ts b/packages/mdc-checkbox/foundation.ts index 70343ceabe2..5bbd835a25d 100644 --- a/packages/mdc-checkbox/foundation.ts +++ b/packages/mdc-checkbox/foundation.ts @@ -108,8 +108,8 @@ export class MDCCheckboxFoundation extends MDCFoundation { const {TRANSITION_STATE_INDETERMINATE} = strings; if (this.currentCheckState_ === TRANSITION_STATE_INDETERMINATE) { this.adapter_.setChecked(!this.previousIsChecked_); - this.handleChange(); } + this.handleChange(); } /** diff --git a/test/unit/mdc-checkbox/foundation.test.js b/test/unit/mdc-checkbox/foundation.test.js index 8c016857b9b..1fb63e72041 100644 --- a/test/unit/mdc-checkbox/foundation.test.js +++ b/test/unit/mdc-checkbox/foundation.test.js @@ -88,7 +88,7 @@ function setupClickHandlerTest() { return {foundation, mockAdapter, change}; } -function testChangeHandler(desc, changes, expectedClass, verificationOpts) { +function testClickHandler(desc, changes, expectedClass, verificationOpts) { changes = Array.isArray(changes) ? changes : [changes]; test(`clickHandler: ${desc}`, () => { const {mockAdapter, change} = setupClickHandlerTest(); @@ -114,8 +114,8 @@ test('exports numbers', () => { test('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCCheckboxFoundation, [ 'addClass', 'removeClass', 'setNativeControlAttr', 'removeNativeControlAttr', - 'forceLayout', 'isAttachedToDOM', 'isIndeterminate', 'isChecked', 'setChecked', - 'hasNativeControl', 'setNativeControlDisabled', + 'forceLayout', 'isAttachedToDOM', 'isIndeterminate', 'isChecked', + 'hasNativeControl', 'setNativeControlDisabled', 'setChecked', ]); }); @@ -177,17 +177,17 @@ test('#setDisabled removes mdc-checkbox--disabled class from the root element wh td.verify(mockAdapter.removeClass(cssClasses.DISABLED)); }); -testChangeHandler('unchecked -> checked animation class', { +testClickHandler('unchecked -> checked animation class', { checked: true, indeterminate: false, }, cssClasses.ANIM_UNCHECKED_CHECKED); -testChangeHandler('unchecked -> indeterminate animation class', { +testClickHandler('unchecked -> indeterminate animation class', { checked: false, indeterminate: true, }, cssClasses.ANIM_UNCHECKED_INDETERMINATE); -testChangeHandler('checked -> unchecked animation class', [ +testClickHandler('checked -> unchecked animation class', [ { checked: true, indeterminate: false, @@ -198,7 +198,7 @@ testChangeHandler('checked -> unchecked animation class', [ }, ], cssClasses.ANIM_CHECKED_UNCHECKED); -testChangeHandler('checked -> indeterminate animation class', [ +testClickHandler('checked -> indeterminate animation class', [ { checked: true, indeterminate: false, @@ -209,7 +209,7 @@ testChangeHandler('checked -> indeterminate animation class', [ }, ], cssClasses.ANIM_CHECKED_INDETERMINATE); -testChangeHandler('indeterminate -> checked animation class', [ +testClickHandler('indeterminate -> checked animation class', [ { checked: false, indeterminate: true, @@ -220,7 +220,7 @@ testChangeHandler('indeterminate -> checked animation class', [ }, ], cssClasses.ANIM_INDETERMINATE_CHECKED); -testChangeHandler('indeterminate -> unchecked animation class', [ +testClickHandler('indeterminate -> unchecked animation class', [ { checked: true, indeterminate: true, @@ -231,7 +231,7 @@ testChangeHandler('indeterminate -> unchecked animation class', [ }, ], cssClasses.ANIM_INDETERMINATE_UNCHECKED); -testChangeHandler('no transition classes applied when no state change', [ +testClickHandler('no transition classes applied when no state change', [ { checked: true, indeterminate: false, @@ -327,7 +327,7 @@ test('animation end is debounced if event is called twice', () => { td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED), {times: 1}); }); -test('change handler triggers layout for changes within the same frame to correctly restart anims', () => { +test('click handler triggers layout for changes within the same frame to correctly restart anims', () => { const {mockAdapter, change} = setupClickHandlerTest(); change({checked: true, indeterminate: false}); @@ -337,7 +337,7 @@ test('change handler triggers layout for changes within the same frame to correc td.verify(mockAdapter.forceLayout()); }); -test('change handler updates aria-checked attribute correctly.', () => { +test('click handler updates aria-checked attribute correctly.', () => { const {mockAdapter, change} = setupClickHandlerTest(); change({checked: true, indeterminate: true}); @@ -347,7 +347,7 @@ test('change handler updates aria-checked attribute correctly.', () => { td.verify(mockAdapter.removeNativeControlAttr('aria-checked')); }); -test('change handler does not add animation classes when isAttachedToDOM() is falsy', () => { +test('click handler does not add animation classes when isAttachedToDOM() is falsy', () => { const {mockAdapter, change} = setupClickHandlerTest(); const animClassArg = td.matchers.argThat((cls) => cls.indexOf('mdc-checkbox--anim') >= 0); td.when(mockAdapter.isAttachedToDOM()).thenReturn(false); @@ -356,7 +356,7 @@ test('change handler does not add animation classes when isAttachedToDOM() is fa td.verify(mockAdapter.addClass(animClassArg), {times: 0}); }); -test('change handler does not add animation classes for bogus changes (init -> unchecked)', () => { +test('click handler does not add animation classes for bogus changes (init -> unchecked)', () => { const {mockAdapter, change} = setupClickHandlerTest(); const animClassArg = td.matchers.argThat((cls) => cls.indexOf('mdc-checkbox--anim') >= 0); From aa5a279c9680eba62d9da34320505dab8472f305 Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Thu, 12 Dec 2019 16:20:15 +0100 Subject: [PATCH 6/9] update for the changes requested in pr #5316 --- packages/mdc-checkbox/foundation.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/mdc-checkbox/foundation.ts b/packages/mdc-checkbox/foundation.ts index 5bbd835a25d..8e022e2185a 100644 --- a/packages/mdc-checkbox/foundation.ts +++ b/packages/mdc-checkbox/foundation.ts @@ -55,7 +55,7 @@ export class MDCCheckboxFoundation extends MDCFoundation { } private currentCheckState_ = strings.TRANSITION_STATE_INIT; - private previousIsChecked_ = false; + private realCheckState_ = false; private currentAnimationClass_ = ''; private animEndLatchTimer_ = 0; private enableAnimationEndHandler_ = false; @@ -66,7 +66,7 @@ export class MDCCheckboxFoundation extends MDCFoundation { init() { this.currentCheckState_ = this.determineCheckState_(); - this.previousIsChecked_ = this.adapter_.isChecked(); + this.realCheckState_ = this.adapter_.isChecked(); this.updateAriaChecked_(); this.adapter_.addClass(cssClasses.UPGRADED); } @@ -102,14 +102,18 @@ export class MDCCheckboxFoundation extends MDCFoundation { /** * Handles the click event for the checkbox - * added for IE browser to fix compatibility issue b/144588184 */ handleClick() { + /** + * added for IE browser to fix compatibility issue + * buganizer issue: b/144588184 + * github issue: https://github.com/material-components/material-components-web/issues/4893 + */ const {TRANSITION_STATE_INDETERMINATE} = strings; if (this.currentCheckState_ === TRANSITION_STATE_INDETERMINATE) { - this.adapter_.setChecked(!this.previousIsChecked_); + this.adapter_.setChecked(!this.realCheckState_); } - this.handleChange(); + this.transitionCheckState_(); } /** @@ -133,11 +137,16 @@ export class MDCCheckboxFoundation extends MDCFoundation { this.updateAriaChecked_(); const {TRANSITION_STATE_UNCHECKED} = strings; + const {TRANSITION_STATE_CHECKED} = strings; const {SELECTED} = cssClasses; if (newState === TRANSITION_STATE_UNCHECKED) { this.adapter_.removeClass(SELECTED); + this.realCheckState_ = false; } else { this.adapter_.addClass(SELECTED); + if (newState === TRANSITION_STATE_CHECKED) { + this.realCheckState_ = true; + } } // Check to ensure that there isn't a previously existing animation class, in case for example From 35f81b6272ecdc2d67982b23818a8824afe48032 Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Fri, 13 Dec 2019 10:28:07 +0100 Subject: [PATCH 7/9] update as per actions required from pr #5316 --- packages/mdc-checkbox/foundation.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/mdc-checkbox/foundation.ts b/packages/mdc-checkbox/foundation.ts index 8e022e2185a..32406cf52a5 100644 --- a/packages/mdc-checkbox/foundation.ts +++ b/packages/mdc-checkbox/foundation.ts @@ -104,11 +104,8 @@ export class MDCCheckboxFoundation extends MDCFoundation { * Handles the click event for the checkbox */ handleClick() { - /** - * added for IE browser to fix compatibility issue - * buganizer issue: b/144588184 - * github issue: https://github.com/material-components/material-components-web/issues/4893 - */ + // added for IE browser to fix compatibility issue: + // https://github.com/material-components/material-components-web/issues/4893 const {TRANSITION_STATE_INDETERMINATE} = strings; if (this.currentCheckState_ === TRANSITION_STATE_INDETERMINATE) { this.adapter_.setChecked(!this.realCheckState_); From d0bf6649c33cbea991e2f7ebc7ab3404744abb69 Mon Sep 17 00:00:00 2001 From: Chan Wang Date: Tue, 17 Dec 2019 13:51:52 +0100 Subject: [PATCH 8/9] add test code to new jasmine test file and modify as per pr requested change --- packages/mdc-checkbox/foundation.ts | 3 + packages/mdc-checkbox/test/foundation.test.ts | 254 +++++++++++++++++- 2 files changed, 255 insertions(+), 2 deletions(-) diff --git a/packages/mdc-checkbox/foundation.ts b/packages/mdc-checkbox/foundation.ts index 32406cf52a5..fdfefb3bb8a 100644 --- a/packages/mdc-checkbox/foundation.ts +++ b/packages/mdc-checkbox/foundation.ts @@ -55,6 +55,9 @@ export class MDCCheckboxFoundation extends MDCFoundation { } private currentCheckState_ = strings.TRANSITION_STATE_INIT; + // Native checkboxes can only have two real states: checked/true or unchecked/false + // The indeterminate state is visual only. + // See https://stackoverflow.com/a/33529024 for more details. private realCheckState_ = false; private currentAnimationClass_ = ''; private animEndLatchTimer_ = 0; diff --git a/packages/mdc-checkbox/test/foundation.test.ts b/packages/mdc-checkbox/test/foundation.test.ts index f5ef21170af..de5503998d6 100644 --- a/packages/mdc-checkbox/test/foundation.test.ts +++ b/packages/mdc-checkbox/test/foundation.test.ts @@ -82,7 +82,7 @@ function setupChangeHandlerTest() { function testChangeHandler( desc: string, changes: CheckboxState|CheckboxState[], expectedClass: string) { - changes = Array.isArray(changes) ? changes : [changes] + changes = Array.isArray(changes) ? changes : [changes]; it(`changeHandler: ${desc}`, () => { const {mockAdapter, change} = setupChangeHandlerTest(); @@ -91,6 +91,50 @@ function testChangeHandler( }); } +/** + * Sets up tests which execute click events through the click handler which + * the foundation registers. Returns an object containing the following + * properties: + * - foundation - The MDCCheckboxFoundation instance + * - mockAdapter - The adapter given to the foundation. The adapter is + * pre-configured to capture the changeHandler registered as well as respond + * with different mock objects for native controls based on the state given + * to the change() function. + * - change - A function that's passed an object containing two "checked" and + * "boolean" properties, representing the state of the native control after + * it was changed. E.g. `change({checked: true, indeterminate: false})` + * simulates a change event as the result of a checkbox being checked. + */ +function setupClickHandlerTest() { + const {foundation, mockAdapter} = setupTest(); + mockAdapter.isAttachedToDOM.and.returnValue(true); + mockAdapter.isIndeterminate.and.returnValue(false); + mockAdapter.isChecked.and.returnValue(false); + + foundation.init(); + + const change = (newState: CheckboxState) => { + mockAdapter.hasNativeControl.and.returnValue(true); + mockAdapter.isChecked.and.returnValue(newState.checked); + mockAdapter.isIndeterminate.and.returnValue(newState.indeterminate); + foundation.handleClick(); + }; + + return {foundation, mockAdapter, change}; +} + +function testClickHandler( + desc: string, changes: CheckboxState|CheckboxState[], + expectedClass: string) { + changes = Array.isArray(changes) ? changes : [changes]; + it(`clickHandler: ${desc}`, () => { + const {mockAdapter, change} = setupClickHandlerTest(); + + (changes as any).forEach(change); + expect(mockAdapter.addClass).toHaveBeenCalledWith(expectedClass); + }); +} + describe('MDCCheckboxFoundation', () => { beforeAll(() => { jasmine.clock().install(); @@ -114,6 +158,7 @@ describe('MDCCheckboxFoundation', () => { 'isChecked', 'hasNativeControl', 'setNativeControlDisabled', + 'setChecked', ]); }); @@ -365,7 +410,7 @@ describe('MDCCheckboxFoundation', () => { expect(foundation.enableAnimationEndHandler_).toBe(false); }); - it('animation end is debounced if event is called twice', () => { + it('animation end is debounced if change event is called twice', () => { const {ANIM_UNCHECKED_CHECKED} = cssClasses; const {mockAdapter, foundation} = setupChangeHandlerTest(); foundation.enableAnimationEndHandler_ = true; @@ -434,4 +479,209 @@ describe('MDCCheckboxFoundation', () => { expect(mockAdapter.setNativeControlAttr).not.toHaveBeenCalled(); expect(mockAdapter.removeNativeControlAttr).not.toHaveBeenCalled(); }); + testClickHandler( + 'unchecked -> checked animation class', { + checked: true, + indeterminate: false, + }, + cssClasses.ANIM_UNCHECKED_CHECKED); + + testClickHandler( + 'unchecked -> indeterminate animation class', { + checked: false, + indeterminate: true, + }, + cssClasses.ANIM_UNCHECKED_INDETERMINATE); + + testClickHandler( + 'checked -> unchecked animation class', + [ + { + checked: true, + indeterminate: false, + }, + { + checked: false, + indeterminate: false, + }, + ], + cssClasses.ANIM_CHECKED_UNCHECKED); + + testClickHandler( + 'checked -> indeterminate animation class', + [ + { + checked: true, + indeterminate: false, + }, + { + checked: true, + indeterminate: true, + }, + ], + cssClasses.ANIM_CHECKED_INDETERMINATE); + + testClickHandler( + 'indeterminate -> checked animation class', + [ + { + checked: false, + indeterminate: true, + }, + { + checked: true, + indeterminate: false, + }, + ], + cssClasses.ANIM_INDETERMINATE_CHECKED); + + testClickHandler( + 'indeterminate -> unchecked animation class', + [ + { + checked: true, + indeterminate: true, + }, + { + checked: false, + indeterminate: false, + }, + ], + cssClasses.ANIM_INDETERMINATE_UNCHECKED); + + testClickHandler( + 'no transition classes applied when no state change', + [ + { + checked: true, + indeterminate: false, + }, + { + checked: true, + indeterminate: false, + }, + ], + cssClasses.ANIM_UNCHECKED_CHECKED); + + it('changing from unchecked to checked adds selected class', () => { + const {mockAdapter, change} = setupClickHandlerTest(); + change({ + checked: false, + indeterminate: false, + }); + change({ + checked: true, + indeterminate: false, + }); + expect(mockAdapter.addClass).toHaveBeenCalledWith(cssClasses.SELECTED); + }); + + it('changing from unchecked to indeterminate adds selected class', () => { + const {mockAdapter, change} = setupClickHandlerTest(); + change({ + checked: false, + indeterminate: false, + }); + change({ + checked: false, + indeterminate: true, + }); + expect(mockAdapter.addClass).toHaveBeenCalledWith(cssClasses.SELECTED); + }); + + it('changing from checked to unchecked removes selected class', () => { + const {mockAdapter, change} = setupClickHandlerTest(); + change({ + checked: true, + indeterminate: false, + }); + change({ + checked: false, + indeterminate: false, + }); + expect(mockAdapter.removeClass).toHaveBeenCalledWith(cssClasses.SELECTED); + }); + + it('changing from indeterminate to unchecked removes selected class', () => { + const {mockAdapter, change} = setupClickHandlerTest(); + change({ + checked: false, + indeterminate: true, + }); + change({ + checked: false, + indeterminate: false, + }); + expect(mockAdapter.removeClass).toHaveBeenCalledWith(cssClasses.SELECTED); + }); + + it('animation end is debounced if click event is called twice', () => { + const {ANIM_UNCHECKED_CHECKED} = cssClasses; + const {mockAdapter, foundation} = setupClickHandlerTest(); + foundation.enableAnimationEndHandler_ = true; + foundation.currentAnimationClass_ = ANIM_UNCHECKED_CHECKED; + + foundation.handleAnimationEnd(); + + expect(mockAdapter.removeClass).not.toHaveBeenCalled(); + + foundation.handleAnimationEnd(); + + jasmine.clock().tick(numbers.ANIM_END_LATCH_MS); + expect(mockAdapter.removeClass) + .toHaveBeenCalledWith(ANIM_UNCHECKED_CHECKED); + }); + + it('click handler triggers layout for changes within the same frame to correctly restart anims', + () => { + const {mockAdapter, change} = setupClickHandlerTest(); + + change({checked: true, indeterminate: false}); + expect(mockAdapter.forceLayout).not.toHaveBeenCalled(); + + change({checked: true, indeterminate: true}); + expect(mockAdapter.forceLayout).toHaveBeenCalled(); + }); + + it('click handler updates aria-checked attribute correctly.', () => { + const {mockAdapter, change} = setupClickHandlerTest(); + + change({checked: true, indeterminate: true}); + expect(mockAdapter.setNativeControlAttr) + .toHaveBeenCalledWith('aria-checked', 'mixed'); + + change({checked: true, indeterminate: false}); + expect(mockAdapter.removeNativeControlAttr) + .toHaveBeenCalledWith('aria-checked'); + }); + + it('click handler does not add animation classes when isAttachedToDOM() is falsy', + () => { + const {mockAdapter, change} = setupClickHandlerTest(); + mockAdapter.isAttachedToDOM.and.returnValue(false); + + change({checked: true, indeterminate: false}); + expect(mockAdapter.addClass) + .not.toHaveBeenCalledWith( + jasmine.stringMatching('mdc-checkbox--anim')); + }); + + it('click handler does not add animation classes for bogus changes (init -> unchecked)', + () => { + const {mockAdapter, change} = setupClickHandlerTest(); + + change({checked: false, indeterminate: false}); + expect(mockAdapter.addClass) + .not.toHaveBeenCalledWith( + jasmine.stringMatching('mdc-checkbox--anim')); + }); + + it('click handler does not do anything if checkbox element is not found', + () => { + const {foundation, mockAdapter} = setupTest(); + mockAdapter.hasNativeControl.and.returnValue(false); + expect(() => foundation.handleClick).not.toThrow(); + expect(mockAdapter.setNativeControlAttr).not.toHaveBeenCalled(); + expect(mockAdapter.removeNativeControlAttr).not.toHaveBeenCalled(); + }); }); From b849317573a3f3f2e1efd8dceff8c80dbbf3909d Mon Sep 17 00:00:00 2001 From: Bonnie Zhou Date: Wed, 18 Dec 2019 15:13:15 +0100 Subject: [PATCH 9/9] doc change really just to retrigger travis ci --- packages/mdc-checkbox/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mdc-checkbox/README.md b/packages/mdc-checkbox/README.md index fbcf626dd8a..745bc1e84a0 100644 --- a/packages/mdc-checkbox/README.md +++ b/packages/mdc-checkbox/README.md @@ -194,4 +194,4 @@ Method Signature | Description `setDisabled(disabled: boolean) => void` | Updates the `disabled` property on the underlying input. Does nothing when the underlying input is not present. `handleAnimationEnd() => void` | `animationend` event handler that should be applied to the root element. `handleClick() => void` | `click` event handler that should be applied to the checkbox element. -`handleChange() => void` | Actions taken after check status changes. +`handleChange() => void` | Handles check status changes.