Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

fix(checkbox): change checkbox event type from change to click and add some logic for IE browser #5316

Merged
merged 12 commits into from
Dec 19, 2019
Merged
4 changes: 3 additions & 1 deletion packages/mdc-checkbox/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
`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.
Expand All @@ -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.
1 change: 1 addition & 0 deletions packages/mdc-checkbox/adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
9 changes: 5 additions & 4 deletions packages/mdc-checkbox/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,20 +86,20 @@ export class MDCCheckbox extends MDCComponent<MDCCheckboxFoundation> 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();
Expand All @@ -117,6 +117,7 @@ export class MDCCheckbox extends MDCComponent<MDCCheckboxFoundation> implements
isIndeterminate: () => this.indeterminate,
removeClass: (className) => this.root_.classList.remove(className),
removeNativeControlAttr: (attr) => this.nativeControl_.removeAttribute(attr),
setChecked: (checkStatus) => this.checked = checkStatus,
setNativeControlAttr: (attr, value) => this.nativeControl_.setAttribute(attr, value),
setNativeControlDisabled: (disabled) => this.nativeControl_.disabled = disabled,
};
Expand Down
17 changes: 16 additions & 1 deletion packages/mdc-checkbox/foundation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,14 @@ export class MDCCheckboxFoundation extends MDCFoundation<MDCCheckboxAdapter> {
isIndeterminate: () => false,
removeClass: () => undefined,
removeNativeControlAttr: () => undefined,
setChecked: () => undefined,
setNativeControlAttr: () => undefined,
setNativeControlDisabled: () => undefined,
};
}

private currentCheckState_ = strings.TRANSITION_STATE_INIT;
private previousIsChecked_ = false;
candysonya marked this conversation as resolved.
Show resolved Hide resolved
private currentAnimationClass_ = '';
private animEndLatchTimer_ = 0;
private enableAnimationEndHandler_ = false;
Expand All @@ -64,6 +66,7 @@ export class MDCCheckboxFoundation extends MDCFoundation<MDCCheckboxAdapter> {

init() {
this.currentCheckState_ = this.determineCheckState_();
this.previousIsChecked_ = this.adapter_.isChecked();
this.updateAriaChecked_();
this.adapter_.addClass(cssClasses.UPGRADED);
}
Expand Down Expand Up @@ -98,7 +101,19 @@ export class MDCCheckboxFoundation extends MDCFoundation<MDCCheckboxAdapter> {
}

/**
* Handles the change event for the checkbox
* Handles the click event for the checkbox
* added for IE browser to fix compatibility issue b/144588184
candysonya marked this conversation as resolved.
Show resolved Hide resolved
*/
handleClick() {
const {TRANSITION_STATE_INDETERMINATE} = strings;
candysonya marked this conversation as resolved.
Show resolved Hide resolved
if (this.currentCheckState_ === TRANSITION_STATE_INDETERMINATE) {
this.adapter_.setChecked(!this.previousIsChecked_);
}
this.handleChange();
candysonya marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Handles the actions after check status changes
*/
handleChange() {
this.transitionCheckState_();
Expand Down
59 changes: 30 additions & 29 deletions test/unit/mdc-checkbox/foundation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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() {
candysonya marked this conversation as resolved.
Show resolved Hide resolved
function setupClickHandlerTest() {
const {foundation, mockAdapter} = setupTest();
td.when(mockAdapter.isAttachedToDOM()).thenReturn(true);

Expand All @@ -82,16 +82,16 @@ 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};
}

function testChangeHandler(desc, changes, expectedClass, verificationOpts) {
function testClickHandler(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);
});
Expand All @@ -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',
'hasNativeControl', 'setNativeControlDisabled', 'setChecked',
]);
});

Expand Down Expand Up @@ -176,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,
Expand All @@ -197,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,
Expand All @@ -208,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,
Expand All @@ -219,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,
Expand All @@ -230,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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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;

Expand All @@ -326,8 +327,8 @@ 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', () => {
const {mockAdapter, change} = setupChangeHandlerTest();
test('click handler triggers layout for changes within the same frame to correctly restart anims', () => {
const {mockAdapter, change} = setupClickHandlerTest();

change({checked: true, indeterminate: false});
td.verify(mockAdapter.forceLayout(), {times: 0});
Expand All @@ -336,8 +337,8 @@ 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.', () => {
const {mockAdapter, change} = setupChangeHandlerTest();
test('click handler updates aria-checked attribute correctly.', () => {
const {mockAdapter, change} = setupClickHandlerTest();

change({checked: true, indeterminate: true});
td.verify(mockAdapter.setNativeControlAttr('aria-checked', 'mixed'));
Expand All @@ -346,27 +347,27 @@ 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', () => {
const {mockAdapter, change} = setupChangeHandlerTest();
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);

change({checked: true, indeterminate: false});
td.verify(mockAdapter.addClass(animClassArg), {times: 0});
});

test('change handler does not add animation classes for bogus changes (init -> unchecked)', () => {
const {mockAdapter, change} = setupChangeHandlerTest();
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);

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});
});
28 changes: 20 additions & 8 deletions test/unit/mdc-checkbox/mdc-checkbox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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);
Expand Down