Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ripple): Listen for up events at document level #1800

Merged
merged 9 commits into from
Jan 5, 2018
19 changes: 10 additions & 9 deletions packages/mdc-ripple/foundation.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,12 @@ class MDCRippleFoundation extends MDCFoundation {
/** @private {function(!Event)} */
this.deactivateHandler_ = (e) => this.deactivate_(e);

/** @private {function(!Event)} */
/** @private {function(?Event=)} */
this.focusHandler_ = () => requestAnimationFrame(
() => this.adapter_.addClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
);

/** @private {function(!Event)} */
/** @private {function(?Event=)} */
this.blurHandler_ = () => requestAnimationFrame(
() => this.adapter_.removeClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
);
Expand Down Expand Up @@ -238,7 +238,7 @@ class MDCRippleFoundation extends MDCFoundation {
}

/**
* @param {Event} e
* @param {!Event} e
* @private
*/
registerDeactivationHandlers_(e) {
Expand Down Expand Up @@ -280,7 +280,7 @@ class MDCRippleFoundation extends MDCFoundation {
}

/**
* @param {Event} e
* @param {?Event} e
* @private
*/
activate_(e) {
Expand All @@ -293,9 +293,9 @@ class MDCRippleFoundation extends MDCFoundation {
return;
}

// Avoid reacting to follow-on event fired by touch device after an already-processed activation event
// Avoid reacting to follow-on events fired by touch device after an already-processed user interaction
const previousActivationEvent = this.previousActivationEvent_;
const isSameInteraction = previousActivationEvent && previousActivationEvent.type !== e.type &&
const isSameInteraction = previousActivationEvent && e && previousActivationEvent.type !== e.type &&
previousActivationEvent.clientX === e.clientX && previousActivationEvent.clientY === e.clientY;
if (isSameInteraction) {
return;
Expand All @@ -308,7 +308,7 @@ class MDCRippleFoundation extends MDCFoundation {
e.type === 'mousedown' || e.type === 'touchstart' || e.type === 'pointerdown'
);

if (!activationState.isProgrammatic) {
if (e) {
this.registerDeactivationHandlers_(e);
}

Expand Down Expand Up @@ -427,14 +427,15 @@ class MDCRippleFoundation extends MDCFoundation {
}

resetActivationState_() {
// Take note of previous activation event type to ignore follow-on event for the same interaction on touch devices
this.previousActivationEvent_ = this.activationState_.activationEvent;
this.activationState_ = this.defaultActivationState_();
// Touch devices may fire additional events for the same interaction within a short time.
// Store the previous event until it's safe to assume that subsequent events are for new interactions.
setTimeout(() => this.previousActivationEvent_ = null, 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the setTimeout to account for delays between when same-interaction "follow-on" events are triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Do you think my comment above these lines needs improvement?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, explanation about why the 100ms delay would be nice. I get why it's there now that I really read through but on first pass it's a little confusing.

}

/**
* @param {Event} e
* @param {?Event} e
* @private
*/
deactivate_(e) {
Expand Down
20 changes: 20 additions & 0 deletions test/unit/mdc-ripple/foundation-activation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,26 @@ testFoundation('adds activation classes on programmatic activation', ({foundatio
td.verify(adapter.addClass(cssClasses.FG_ACTIVATION));
});

testFoundation('programmatic activation immediately after interaction', ({foundation, adapter, mockRaf}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

A more descriptive name would be helpful (something like "programmatic activation that occurs immediately after interaction adds the FG activation class")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I make the description any longer it will overflow our maximum line length, and that's usually where I draw the line...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hahaha fair enough :) Approved!

const handlers = captureHandlers(adapter, 'registerInteractionHandler');
const documentHandlers = captureHandlers(adapter, 'registerDocumentInteractionHandler');

td.when(adapter.isSurfaceActive()).thenReturn(true);
foundation.init();
mockRaf.flush();

handlers.touchstart({changedTouches: [{pageX: 0, pageY: 0}]});
mockRaf.flush();
documentHandlers.touchend();
mockRaf.flush();

foundation.activate();
mockRaf.flush();

td.verify(adapter.addClass(cssClasses.BG_ACTIVE_FILL), {times: 2});
td.verify(adapter.addClass(cssClasses.FG_ACTIVATION), {times: 2});
});

testFoundation('sets FG position to center on non-pointer activation', ({foundation, adapter, mockRaf}) => {
const handlers = captureHandlers(adapter, 'registerInteractionHandler');
const left = 50;
Expand Down