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

fix(checkbox): remove register/deregisterEventlisteners from foundation #3402

Merged
merged 8 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/mdc-checkbox/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,6 @@ If you are using a JavaScript framework, such as React or Angular, you can creat
| --- | --- |
| `addClass(className: string) => void` | Adds a class to the root element. |
| `removeClass(className: string) => void` | Removes a class from the root element. |
| `registerAnimationEndHandler(handler: EventListener) => void` | Registers an event handler to be called when an `animationend` event is triggered on the root element. Note that you must account for vendor prefixes in order for this to work correctly. |
| `deregisterAnimationEndHandler(handler: EventListener) => void` | Deregisters an event handler from an `animationend` event listener. This will only be called with handlers that have previously been passed to `registerAnimationEndHandler` calls. |
| `registerChangeHandler(handler: EventListener) => void` | Registers an event handler to be called when a `change` event is triggered on the native control (_not_ the root element). |
| `deregisterChangeHandler(handler: EventListener) => void` | Deregisters an event handler that was previously passed to `registerChangeHandler`. |
| `getNativeControl() => HTMLInputElement?` | Returns the native checkbox control, if available. Note that if this control is not available, the methods that rely on it will exit gracefully.|
| `forceLayout() => void` | Force-trigger a layout on the root element. This is needed to restart animations correctly. If you find that you do not need to do this, you can simply make it a no-op. |
| `isAttachedToDOM() => boolean` | Returns true if the component is currently attached to the DOM, false otherwise. |
Expand All @@ -179,3 +175,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.
`getValue() => string` | Returns the value of `MDCCheckboxAdapter.getNativeControl().value`. Returns `null` if `getNativeControl()` does not return an object.
`setValue(value: string) => void` | Sets the value of `adapter.getNativeControl().value`. Does nothing if `getNativeControl()` does not return an object.
`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.
12 changes: 0 additions & 12 deletions packages/mdc-checkbox/adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,6 @@ class MDCCheckboxAdapter {
*/
removeNativeControlAttr(attr) {}

/** @param {!EventListener} handler */
registerAnimationEndHandler(handler) {}

/** @param {!EventListener} handler */
deregisterAnimationEndHandler(handler) {}

/** @param {!EventListener} handler */
registerChangeHandler(handler) {}

/** @param {!EventListener} handler */
deregisterChangeHandler(handler) {}

/** @return {!MDCSelectionControlState} */
getNativeControl() {}

Expand Down
22 changes: 9 additions & 13 deletions packages/mdc-checkbox/foundation.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ class MDCCheckboxFoundation extends MDCFoundation {
removeClass: (/* className: string */) => {},
setNativeControlAttr: (/* attr: string, value: string */) => {},
removeNativeControlAttr: (/* attr: string */) => {},
registerAnimationEndHandler: (/* handler: EventListener */) => {},
deregisterAnimationEndHandler: (/* handler: EventListener */) => {},
registerChangeHandler: (/* handler: EventListener */) => {},
deregisterChangeHandler: (/* handler: EventListener */) => {},
getNativeControl: () => /* !MDCSelectionControlState */ {},
forceLayout: () => {},
isAttachedToDOM: () => /* boolean */ {},
Expand All @@ -79,25 +75,20 @@ class MDCCheckboxFoundation extends MDCFoundation {
/** @private {number} */
this.animEndLatchTimer_ = 0;

this.animEndHandler_ = /** @private {!EventListener} */ (
() => this.handleAnimationEnd());

this.changeHandler_ = /** @private {!EventListener} */ (
() => this.handleChange());
/** @private {boolean} */
this.enableAnimationEndHandler_ = false;
}

/** @override */
init() {
this.currentCheckState_ = this.determineCheckState_(this.getNativeControl_());
this.updateAriaChecked_();
this.adapter_.addClass(cssClasses.UPGRADED);
this.adapter_.registerChangeHandler(this.changeHandler_);
this.installPropertyChangeHooks_();
}

/** @override */
destroy() {
this.adapter_.deregisterChangeHandler(this.changeHandler_);
this.uninstallPropertyChangeHooks_();
}

Expand Down Expand Up @@ -150,10 +141,13 @@ class MDCCheckboxFoundation extends MDCFoundation {
* Handles the animationend event for the checkbox
*/
handleAnimationEnd() {
if (!this.enableAnimationEndHandler_) return;

clearTimeout(this.animEndLatchTimer_);

this.animEndLatchTimer_ = setTimeout(() => {
this.adapter_.removeClass(this.currentAnimationClass_);
this.adapter_.deregisterAnimationEndHandler(this.animEndHandler_);
this.enableAnimationEndHandler_ = false;
}, numbers.ANIM_END_LATCH_MS);
}

Expand Down Expand Up @@ -231,7 +225,7 @@ class MDCCheckboxFoundation extends MDCFoundation {
// to the DOM.
if (this.adapter_.isAttachedToDOM() && this.currentAnimationClass_.length > 0) {
this.adapter_.addClass(this.currentAnimationClass_);
this.adapter_.registerAnimationEndHandler(this.animEndHandler_);
this.enableAnimationEndHandler_ = true;
}
}

Expand Down Expand Up @@ -297,6 +291,8 @@ class MDCCheckboxFoundation extends MDCFoundation {
this.adapter_.setNativeControlAttr(
strings.ARIA_CHECKED_ATTR, strings.ARIA_CHECKED_INDETERMINATE_VALUE);
} else {
// the on/off state does not need to keep track of aria-checked, since
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Capitalize The

// the screenreader uses the checked property on the checkbox element.
this.adapter_.removeNativeControlAttr(strings.ARIA_CHECKED_ATTR);
}
}
Expand Down
18 changes: 13 additions & 5 deletions packages/mdc-checkbox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ class MDCCheckbox extends MDCComponent {

/** @private {!MDCRipple} */
this.ripple_ = this.initRipple_();
/** @private {!Function} */
this.handleChange_;
/** @private {!Function} */
this.handleAnimationEnd_;
}

initialSyncWithDOM() {
this.handleChange_ = () => this.foundation_.handleChange();
this.handleAnimationEnd_= () => this.foundation_.handleAnimationEnd();
this.nativeCb_.addEventListener('change', this.handleChange_);
this.root_.addEventListener(getCorrectEventName(window, 'animationend'), this.handleAnimationEnd_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to this.listen (since you're using unlisten in destroy anyway)

}

/**
Expand All @@ -81,11 +92,6 @@ class MDCCheckbox extends MDCComponent {
removeClass: (className) => this.root_.classList.remove(className),
setNativeControlAttr: (attr, value) => this.nativeCb_.setAttribute(attr, value),
removeNativeControlAttr: (attr) => this.nativeCb_.removeAttribute(attr),
registerAnimationEndHandler:
(handler) => this.root_.addEventListener(getCorrectEventName(window, 'animationend'), handler),
deregisterAnimationEndHandler:
(handler) => this.root_.removeEventListener(getCorrectEventName(window, 'animationend'), handler),
registerChangeHandler: (handler) => this.nativeCb_.addEventListener('change', handler),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to remove deregisterChangeHandler (I noticed because coverage found it uncovered, but it shouldn't be here anymore)

deregisterChangeHandler: (handler) => this.nativeCb_.removeEventListener('change', handler),
getNativeControl: () => this.nativeCb_,
forceLayout: () => this.root_.offsetWidth,
Expand Down Expand Up @@ -140,6 +146,8 @@ class MDCCheckbox extends MDCComponent {

destroy() {
this.ripple_.destroy();
this.nativeCb_.removeEventListener('change', this.handleChange_);
this.unlisten(getCorrectEventName(window, 'animationend'), this.handleAnimationEnd_);
super.destroy();
}
}
Expand Down
90 changes: 21 additions & 69 deletions test/unit/mdc-checkbox/foundation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,14 @@ function withMockCheckboxDescriptorReturning(descriptor, runTests) {
// `change({checked: true, indeterminate: false})` simulates a change event as the result of a checkbox
// being checked.
function setupChangeHandlerTest() {
let changeHandler;
const {foundation, mockAdapter} = setupTest();
const {isA} = td.matchers;
td.when(mockAdapter.registerChangeHandler(isA(Function))).thenDo(function(handler) {
changeHandler = handler;
});
td.when(mockAdapter.isAttachedToDOM()).thenReturn(true);

foundation.init();

const change = (newState) => {
td.when(mockAdapter.getNativeControl()).thenReturn(newState);
changeHandler();
foundation.handleChange();
};

return {foundation, mockAdapter, change};
Expand Down Expand Up @@ -117,9 +112,8 @@ test('exports numbers', () => {

test('defaultAdapter returns a complete adapter implementation', () => {
verifyDefaultAdapter(MDCCheckboxFoundation, [
'addClass', 'removeClass', 'setNativeControlAttr', 'removeNativeControlAttr', 'registerAnimationEndHandler',
'deregisterAnimationEndHandler', 'registerChangeHandler', 'deregisterChangeHandler', 'getNativeControl',
'forceLayout', 'isAttachedToDOM',
'addClass', 'removeClass', 'setNativeControlAttr', 'removeNativeControlAttr',
'getNativeControl', 'forceLayout', 'isAttachedToDOM',
]);
});

Expand All @@ -138,14 +132,6 @@ test('#init adds aria-checked="mixed" if checkbox is initially indeterminate', (
td.verify(mockAdapter.setNativeControlAttr('aria-checked', strings.ARIA_CHECKED_INDETERMINATE_VALUE));
});

test('#init calls adapter.registerChangeHandler() with a change handler function', () => {
const {foundation, mockAdapter} = setupTest();
const {isA} = td.matchers;

foundation.init();
td.verify(mockAdapter.registerChangeHandler(isA(Function)));
});

test('#init handles case where getNativeControl() does not return anything', () => {
const {foundation, mockAdapter} = setupTest();
td.when(mockAdapter.getNativeControl()).thenReturn(undefined);
Expand All @@ -169,20 +155,6 @@ test('#init handles case when property descriptors are not returned at all (Andr
});
});

test('#destroy calls adapter.deregisterChangeHandler() with a registerChangeHandler function', () => {
const {foundation, mockAdapter} = setupTest();
const {isA} = td.matchers;

let changeHandler;
td.when(mockAdapter.registerChangeHandler(isA(Function))).thenDo(function(handler) {
changeHandler = handler;
});
foundation.init();

foundation.destroy();
td.verify(mockAdapter.deregisterChangeHandler(changeHandler));
});

test('#destroy handles case where getNativeControl() does not return anything', () => {
const {foundation, mockAdapter} = setupTest();
foundation.init();
Expand Down Expand Up @@ -386,59 +358,39 @@ testChangeHandler('no transition classes applied when no state change', [
},
], cssClasses.ANIM_UNCHECKED_CHECKED, {times: 1});

test('animation end handler one-off removes animation class after short delay', () => {
test('animation end handler removes animation class after short delay', () => {
const clock = lolex.install();
const {mockAdapter, change} = setupChangeHandlerTest();
const {isA} = td.matchers;

let animEndHandler;
td.when(mockAdapter.registerAnimationEndHandler(isA(Function))).thenDo(function(handler) {
animEndHandler = handler;
});

change({checked: true, indeterminate: false});
assert.isOk(animEndHandler instanceof Function, 'animationend handler registeration sanity test');

animEndHandler();
const {ANIM_UNCHECKED_CHECKED} = cssClasses;
const {mockAdapter, foundation} = setupTest();

foundation.enableAnimationEndHandler_ = true;
foundation.currentAnimationClass_ = ANIM_UNCHECKED_CHECKED;
td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED), {times: 0});

foundation.handleAnimationEnd();

clock.tick(numbers.ANIM_END_LATCH_MS);
td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED));
td.verify(mockAdapter.deregisterAnimationEndHandler(animEndHandler));
td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED), {times: 1});
assert.isFalse(foundation.enableAnimationEndHandler_);

clock.uninstall();
});

test('change handler debounces changes within the animation end delay period', () => {
test('animation end is debounced if event is called twice', () => {
const clock = lolex.install();
const {mockAdapter, change} = setupChangeHandlerTest();
const {isA} = td.matchers;
const {ANIM_UNCHECKED_CHECKED} = cssClasses;
const {mockAdapter, foundation} = setupChangeHandlerTest();
foundation.enableAnimationEndHandler_ = true;
foundation.currentAnimationClass_ = ANIM_UNCHECKED_CHECKED;

let animEndHandler;
td.when(mockAdapter.registerAnimationEndHandler(isA(Function))).thenDo(function(handler) {
animEndHandler = handler;
});
foundation.handleAnimationEnd();

change({checked: true, indeterminate: false});
assert.isOk(animEndHandler instanceof Function, 'animationend handler registeration sanity test');
// Queue up initial timer
animEndHandler();
td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED), {times: 0});

const {ANIM_UNCHECKED_CHECKED, ANIM_CHECKED_INDETERMINATE} = cssClasses;
foundation.handleAnimationEnd();

change({checked: true, indeterminate: true});
// Without ticking the clock, check that the prior class has been removed.
td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED));
// The animation end handler should not yet have been removed.
td.verify(mockAdapter.deregisterAnimationEndHandler(animEndHandler), {times: 0});

// Call animEndHandler again, and tick the clock. The original timer should have been cleared, and the
// current timer should remove the correct, latest animation class, along with deregistering the handler.
animEndHandler();
clock.tick(numbers.ANIM_END_LATCH_MS);
td.verify(mockAdapter.removeClass(ANIM_CHECKED_INDETERMINATE), {times: 1});
td.verify(mockAdapter.deregisterAnimationEndHandler(animEndHandler), {times: 1});
td.verify(mockAdapter.removeClass(ANIM_UNCHECKED_CHECKED), {times: 1});

clock.uninstall();
});
Expand Down
75 changes: 30 additions & 45 deletions test/unit/mdc-checkbox/mdc-checkbox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {createMockRaf} from '../helpers/raf';
import {MDCCheckbox} from '../../../packages/mdc-checkbox';
import {MDCRipple} from '../../../packages/mdc-ripple';
import {strings} from '../../../packages/mdc-checkbox/constants';
import {getCorrectEventName} from '../../../packages/mdc-animation';
import {getMatchesProperty} from '../../../packages/mdc-ripple/util';

function getFixture() {
Expand Down Expand Up @@ -139,6 +138,36 @@ test('get ripple returns a MDCRipple instance', () => {
assert.isOk(component.ripple instanceof MDCRipple);
});

test('checkbox change event calls #foundation.handleChange', () => {
const {cb, component} = setupTest();
component.foundation_.handleChange = td.func();
domEvents.emit(cb, 'change');
td.verify(component.foundation_.handleChange(), {times: 1});
});

test('root animationend event calls #foundation.handleAnimationEnd', () => {
const {root, component} = setupTest();
component.foundation_.handleAnimationEnd = td.func();
domEvents.emit(root, 'animationend');
td.verify(component.foundation_.handleAnimationEnd(), {times: 1});
});

test('checkbox change event is destroyed on #destroy', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event handler is destroyed

const {cb, component} = setupTest();
component.foundation_.handleChange = td.func();
component.destroy();
domEvents.emit(cb, 'change');
td.verify(component.foundation_.handleChange(), {times: 0});
});

test('root animationend event is destroyed on #destroy', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event handler is destroyed

const {root, component} = setupTest();
component.foundation_.handleAnimationEnd = td.func();
component.destroy();
domEvents.emit(root, 'animationend');
td.verify(component.foundation_.handleAnimationEnd(), {times: 0});
});

test('adapter#addClass adds a class to the root element', () => {
const {root, component} = setupTest();
component.getDefaultFoundation().adapter_.addClass('foo');
Expand All @@ -165,50 +194,6 @@ test('adapter#removeNativeControlAttr removes an attribute from the input elemen
assert.isFalse(cb.hasAttribute('aria-checked'));
});

test('adapter#registerAnimationEndHandler adds an animation end event listener on the root element', () => {
const {root, component} = setupTest();
const handler = td.func('animationEndHandler');
component.getDefaultFoundation().adapter_.registerAnimationEndHandler(handler);
domEvents.emit(root, getCorrectEventName(window, 'animationend'));

td.verify(handler(td.matchers.anything()));
});

test('adapter#deregisterAnimationEndHandler removes an animation end event listener on the root el', () => {
const {root, component} = setupTest();
const handler = td.func('animationEndHandler');
const animEndEvtName = getCorrectEventName(window, 'animationend');
root.addEventListener(animEndEvtName, handler);

component.getDefaultFoundation().adapter_.deregisterAnimationEndHandler(handler);
domEvents.emit(root, animEndEvtName);

td.verify(handler(td.matchers.anything()), {times: 0});
});

test('adapter#registerChangeHandler adds a change event listener to the native checkbox element', () => {
const {root, component} = setupTest();
const nativeCb = root.querySelector(strings.NATIVE_CONTROL_SELECTOR);
const handler = td.func('changeHandler');

component.getDefaultFoundation().adapter_.registerChangeHandler(handler);
domEvents.emit(nativeCb, 'change');

td.verify(handler(td.matchers.anything()));
});

test('adapter#deregisterChangeHandler adds a change event listener to the native checkbox element', () => {
const {root, component} = setupTest();
const nativeCb = root.querySelector(strings.NATIVE_CONTROL_SELECTOR);
const handler = td.func('changeHandler');
nativeCb.addEventListener('change', handler);

component.getDefaultFoundation().adapter_.deregisterChangeHandler(handler);
domEvents.emit(nativeCb, 'change');

td.verify(handler(td.matchers.anything()), {times: 0});
});

test('adapter#getNativeControl returns the native checkbox element', () => {
const {root, component} = setupTest();
const nativeCb = root.querySelector(strings.NATIVE_CONTROL_SELECTOR);
Expand Down