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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a8d4c20
WIP fix(ripple): Listen for up events at document level
Nov 29, 2017
05ae636
WIP Remove mostly-pointless ripple helper method
Dec 11, 2017
97769c4
WIP Update tests to pass after event handler refactor
Dec 11, 2017
463d266
WIP Fix double-ripple possible on touch devices due to follow-on events
Dec 18, 2017
0a8112e
WIP More test fixes
Dec 18, 2017
9b2aae7
WIP Reinstate destroy no-op test for branch coverage
Dec 18, 2017
91f061c
WIP Fix type annotations and fix/test a bug
Jan 5, 2018
fcaf741
Merge branch 'master' into fix/ripple-up-events
Jan 5, 2018
e55a962
Merge branch 'master' into fix/ripple-up-events
kfranqueiro Jan 5, 2018
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
2 changes: 2 additions & 0 deletions packages/mdc-ripple/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ ripple to. The adapter API is as follows:
| `removeClass(className: string) => void` | Removes a class from the ripple surface |
| `registerInteractionHandler(evtType: string, handler: EventListener) => void` | Registers an event handler that's invoked when the ripple is interacted with using type `evtType`. Essentially equivalent to `HTMLElement.prototype.addEventListener`. |
| `deregisterInteractionHandler(evtType: string, handler: EventListener) => void` | Unregisters an event handler that's invoked when the ripple is interacted with using type `evtType`. Essentially equivalent to `HTMLElement.prototype.removeEventListener`. |
| `registerDocumentInteractionHandler(evtType: string, handler: EventListener) => void` | Registers an event handler that's invoked when the documentElement is interacted with using type `evtType` |
| `deregisterDocumentInteractionHandler(evtType: string, handler: EventListener) => void` | Unregisters an event handler that's invoked when the documentElement is interacted with using type `evtType` |
| `registerResizeHandler(handler: Function) => void` | Registers a handler to be called when the surface (or its viewport) resizes. Our default implementation adds the handler as a listener to the window's `resize()` event. |
| `deregisterResizeHandler(handler: Function) => void` | Unregisters a handler to be called when the surface (or its viewport) resizes. Our default implementation removes the handler as a listener to the window's `resize()` event. |
| `updateCssVariable(varName: string, value: (string or null)) => void` | Programmatically sets the css variable `varName` on the surface to the value specified. |
Expand Down
12 changes: 12 additions & 0 deletions packages/mdc-ripple/adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,18 @@ class MDCRippleAdapter {
*/
deregisterInteractionHandler(evtType, handler) {}

/**
* @param {string} evtType
* @param {!Function} handler
*/
registerDocumentInteractionHandler(evtType, handler) {}

/**
* @param {string} evtType
* @param {!Function} handler
*/
deregisterDocumentInteractionHandler(evtType, handler) {}

/**
* @param {!Function} handler
*/
Expand Down
219 changes: 118 additions & 101 deletions packages/mdc-ripple/foundation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {getNormalizedEventCoords} from './util';
* hasDeactivationUXRun: (boolean|undefined),
* wasActivatedByPointer: (boolean|undefined),
* wasElementMadeActive: (boolean|undefined),
* activationStartTime: (number|undefined),
* activationEvent: Event,
* isProgrammatic: (boolean|undefined)
* }}
Expand Down Expand Up @@ -61,16 +60,11 @@ let ListenersType;
*/
let PointType;

/**
* @enum {string}
*/
const DEACTIVATION_ACTIVATION_PAIRS = {
mouseup: 'mousedown',
pointerup: 'pointerdown',
touchend: 'touchstart',
keyup: 'keydown',
blur: 'focus',
};
// Activation events registered on the root element of each instance for activation
const ACTIVATION_EVENT_TYPES = ['touchstart', 'pointerdown', 'mousedown', 'keydown'];

// Deactivation events registered on documentElement when a pointer-related down event occurs
const POINTER_DEACTIVATION_EVENT_TYPES = ['touchend', 'pointerup', 'mouseup'];

/**
* @extends {MDCFoundation<!MDCRippleAdapter>}
Expand Down Expand Up @@ -98,6 +92,8 @@ class MDCRippleFoundation extends MDCFoundation {
removeClass: (/* className: string */) => {},
registerInteractionHandler: (/* evtType: string, handler: EventListener */) => {},
deregisterInteractionHandler: (/* evtType: string, handler: EventListener */) => {},
registerDocumentInteractionHandler: (/* evtType: string, handler: EventListener */) => {},
deregisterDocumentInteractionHandler: (/* evtType: string, handler: EventListener */) => {},
registerResizeHandler: (/* handler: EventListener */) => {},
deregisterResizeHandler: (/* handler: EventListener */) => {},
updateCssVariable: (/* varName: string, value: string */) => {},
Expand Down Expand Up @@ -127,26 +123,21 @@ class MDCRippleFoundation extends MDCFoundation {
/** @private {number} */
this.maxRadius_ = 0;

/** @private {!Array<{ListenerInfoType}>} */
this.listenerInfos_ = [
{activate: 'touchstart', deactivate: 'touchend'},
{activate: 'pointerdown', deactivate: 'pointerup'},
{activate: 'mousedown', deactivate: 'mouseup'},
{activate: 'keydown', deactivate: 'keyup'},
{focus: 'focus', blur: 'blur'},
];

/** @private {!ListenersType} */
this.listeners_ = {
activate: (e) => this.activate_(e),
deactivate: (e) => this.deactivate_(e),
focus: () => requestAnimationFrame(
() => this.adapter_.addClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
),
blur: () => requestAnimationFrame(
() => this.adapter_.removeClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
),
};
/** @private {function(!Event)} */
this.activateHandler_ = (e) => this.activate_(e);

/** @private {function(!Event)} */
this.deactivateHandler_ = (e) => this.deactivate_(e);

/** @private {function(!Event)} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Event annotation should be nullable since it's not required inside the function

this.focusHandler_ = () => requestAnimationFrame(
() => this.adapter_.addClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
);

/** @private {function(!Event)} */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

this.blurHandler_ = () => requestAnimationFrame(
() => this.adapter_.removeClass(MDCRippleFoundation.cssClasses.BG_FOCUSED)
);

/** @private {!Function} */
this.resizeHandler_ = () => this.layout();
Expand Down Expand Up @@ -174,6 +165,9 @@ class MDCRippleFoundation extends MDCFoundation {
this.activationAnimationHasEnded_ = true;
this.runDeactivationUXLogicIfReady_();
};

/** @private {?Event} */
this.previousActivationEvent_ = null;
}

/**
Expand All @@ -197,7 +191,6 @@ class MDCRippleFoundation extends MDCFoundation {
hasDeactivationUXRun: false,
wasActivatedByPointer: false,
wasElementMadeActive: false,
activationStartTime: 0,
activationEvent: null,
isProgrammatic: false,
};
Expand All @@ -207,7 +200,7 @@ class MDCRippleFoundation extends MDCFoundation {
if (!this.isSupported_()) {
return;
}
this.addEventListeners_();
this.registerRootHandlers_();

const {ROOT, UNBOUNDED} = MDCRippleFoundation.cssClasses;
requestAnimationFrame(() => {
Expand All @@ -219,16 +212,73 @@ class MDCRippleFoundation extends MDCFoundation {
});
}

destroy() {
if (!this.isSupported_()) {
return;
}
this.deregisterRootHandlers_();
this.deregisterDeactivationHandlers_();

const {ROOT, UNBOUNDED} = MDCRippleFoundation.cssClasses;
requestAnimationFrame(() => {
this.adapter_.removeClass(ROOT);
this.adapter_.removeClass(UNBOUNDED);
this.removeCssVars_();
});
}

/** @private */
addEventListeners_() {
this.listenerInfos_.forEach((info) => {
Object.keys(info).forEach((k) => {
this.adapter_.registerInteractionHandler(info[k], this.listeners_[k]);
});
registerRootHandlers_() {
ACTIVATION_EVENT_TYPES.forEach((type) => {
this.adapter_.registerInteractionHandler(type, this.activateHandler_);
});
this.adapter_.registerInteractionHandler('focus', this.focusHandler_);
this.adapter_.registerInteractionHandler('blur', this.blurHandler_);
this.adapter_.registerResizeHandler(this.resizeHandler_);
}

/**
* @param {Event} e
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a non-nullable annotation for Event

* @private
*/
registerDeactivationHandlers_(e) {
if (e.type === 'keydown') {
this.adapter_.registerInteractionHandler('keyup', this.deactivateHandler_);
} else {
POINTER_DEACTIVATION_EVENT_TYPES.forEach((type) => {
this.adapter_.registerDocumentInteractionHandler(type, this.deactivateHandler_);
});
}
}

/** @private */
deregisterRootHandlers_() {
ACTIVATION_EVENT_TYPES.forEach((type) => {
this.adapter_.deregisterInteractionHandler(type, this.activateHandler_);
});
this.adapter_.deregisterInteractionHandler('focus', this.focusHandler_);
this.adapter_.deregisterInteractionHandler('blur', this.blurHandler_);
this.adapter_.deregisterResizeHandler(this.resizeHandler_);
}

/** @private */
deregisterDeactivationHandlers_() {
this.adapter_.deregisterInteractionHandler('keyup', this.deactivateHandler_);
POINTER_DEACTIVATION_EVENT_TYPES.forEach((type) => {
this.adapter_.deregisterDocumentInteractionHandler(type, this.deactivateHandler_);
});
}

/** @private */
removeCssVars_() {
const {strings} = MDCRippleFoundation;
Object.keys(strings).forEach((k) => {
if (k.indexOf('VAR_') === 0) {
this.adapter_.updateCssVariable(strings[k], null);
}
});
}

/**
* @param {Event} e
* @private
Expand All @@ -243,13 +293,24 @@ class MDCRippleFoundation extends MDCFoundation {
return;
}

// Avoid reacting to follow-on event fired by touch device after an already-processed activation event
const previousActivationEvent = this.previousActivationEvent_;
const isSameInteraction = previousActivationEvent && previousActivationEvent.type !== e.type &&
previousActivationEvent.clientX === e.clientX && previousActivationEvent.clientY === e.clientY;
if (isSameInteraction) {
return;
}

activationState.isActivated = true;
activationState.isProgrammatic = e === null;
activationState.activationEvent = e;
activationState.wasActivatedByPointer = activationState.isProgrammatic ? false : (
e.type === 'mousedown' || e.type === 'touchstart' || e.type === 'pointerdown'
);
activationState.activationStartTime = Date.now();

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

requestAnimationFrame(() => {
// This needs to be wrapped in an rAF call b/c web browsers
Expand Down Expand Up @@ -365,48 +426,38 @@ class MDCRippleFoundation extends MDCFoundation {
this.adapter_.computeBoundingRect();
}

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_();
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
* @private
*/
deactivate_(e) {
const {activationState_: activationState} = this;
const activationState = this.activationState_;
// This can happen in scenarios such as when you have a keyup event that blurs the element.
if (!activationState.isActivated) {
return;
}
// Programmatic deactivation.

const state = /** @type {!ActivationStateType} */ (Object.assign({}, activationState));

if (activationState.isProgrammatic) {
const evtObject = null;
const state = /** @type {!ActivationStateType} */ (Object.assign({}, activationState));
requestAnimationFrame(() => this.animateDeactivation_(evtObject, state));
this.activationState_ = this.defaultActivationState_();
return;
}

const actualActivationType = DEACTIVATION_ACTIVATION_PAIRS[e.type];
const expectedActivationType = activationState.activationEvent.type;
// NOTE: Pointer events are tricky - https://patrickhlauke.github.io/touch/tests/results/
// Essentially, what we need to do here is decouple the deactivation UX from the actual
// deactivation state itself. This way, touch/pointer events in sequence do not trample one
// another.
const needsDeactivationUX = actualActivationType === expectedActivationType;
let needsActualDeactivation = needsDeactivationUX;
if (activationState.wasActivatedByPointer) {
needsActualDeactivation = e.type === 'mouseup';
}

const state = /** @type {!ActivationStateType} */ (Object.assign({}, activationState));
requestAnimationFrame(() => {
if (needsDeactivationUX) {
this.resetActivationState_();
} else {
this.deregisterDeactivationHandlers_();
requestAnimationFrame(() => {
this.activationState_.hasDeactivationUXRun = true;
this.animateDeactivation_(e, state);
}

if (needsActualDeactivation) {
this.activationState_ = this.defaultActivationState_();
}
});
this.resetActivationState_();
});
}
}

/**
Expand All @@ -430,40 +481,6 @@ class MDCRippleFoundation extends MDCFoundation {
}
}

destroy() {
if (!this.isSupported_()) {
return;
}
this.removeEventListeners_();

const {ROOT, UNBOUNDED} = MDCRippleFoundation.cssClasses;
requestAnimationFrame(() => {
this.adapter_.removeClass(ROOT);
this.adapter_.removeClass(UNBOUNDED);
this.removeCssVars_();
});
}

/** @private */
removeEventListeners_() {
this.listenerInfos_.forEach((info) => {
Object.keys(info).forEach((k) => {
this.adapter_.deregisterInteractionHandler(info[k], this.listeners_[k]);
});
});
this.adapter_.deregisterResizeHandler(this.resizeHandler_);
}

/** @private */
removeCssVars_() {
const {strings} = MDCRippleFoundation;
Object.keys(strings).forEach((k) => {
if (k.indexOf('VAR_') === 0) {
this.adapter_.updateCssVariable(strings[k], null);
}
});
}

layout() {
if (this.layoutFrame_) {
cancelAnimationFrame(this.layoutFrame_);
Expand Down
4 changes: 4 additions & 0 deletions packages/mdc-ripple/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ class MDCRipple extends MDCComponent {
instance.root_.addEventListener(evtType, handler, util.applyPassive()),
deregisterInteractionHandler: (evtType, handler) =>
instance.root_.removeEventListener(evtType, handler, util.applyPassive()),
registerDocumentInteractionHandler: (evtType, handler) =>
document.documentElement.addEventListener(evtType, handler, util.applyPassive()),
deregisterDocumentInteractionHandler: (evtType, handler) =>
document.documentElement.removeEventListener(evtType, handler, util.applyPassive()),
registerResizeHandler: (handler) => window.addEventListener('resize', handler),
deregisterResizeHandler: (handler) => window.removeEventListener('resize', handler),
updateCssVariable: (varName, value) => instance.root_.style.setProperty(varName, value),
Expand Down
Loading