From 77685dfb1995a3fd739cc6a03f7b94862fc30696 Mon Sep 17 00:00:00 2001 From: Alex Sheehan Date: Tue, 18 Apr 2017 12:59:07 -0400 Subject: [PATCH 1/4] WIP --- demos/checkbox.html | 28 +++++++++++++++++++++++++ packages/mdc-checkbox/README.md | 2 ++ packages/mdc-checkbox/index.js | 1 + packages/mdc-radio/README.md | 2 ++ packages/mdc-radio/index.js | 1 + packages/mdc-ripple/README.md | 1 + packages/mdc-ripple/foundation.js | 6 ++++++ packages/mdc-ripple/index.js | 1 + test/unit/mdc-ripple/foundation.test.js | 9 ++++---- 9 files changed, 47 insertions(+), 4 deletions(-) diff --git a/demos/checkbox.html b/demos/checkbox.html index 302d5030b35..e1e07148a0c 100644 --- a/demos/checkbox.html +++ b/demos/checkbox.html @@ -128,6 +128,31 @@

With Javascript

+ +
+

Disabled

+
+
+ +
+ + + +
+ +
+
+ +
+
+

Dark Theme

@@ -161,6 +186,9 @@

Dark Theme

var checkbox = document.getElementById('mdc-js-checkbox'); var checkboxInstance = new MDCCheckbox(checkbox); + var disabledCheckbox = document.getElementById('mdc-disabled-js-checkbox'); + var disabledCheckboxInstance = new MDCCheckbox(disabledCheckbox); + var formField = checkbox.parentElement; var formFieldInstance = new MDCFormField(formField); formFieldInstance.input = checkboxInstance; diff --git a/packages/mdc-checkbox/README.md b/packages/mdc-checkbox/README.md index 31d22f027fd..79ff52b7f41 100644 --- a/packages/mdc-checkbox/README.md +++ b/packages/mdc-checkbox/README.md @@ -205,6 +205,8 @@ The adapter for checkboxes must provide the following functions, with correct si | `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.` | +> **NOTE**: There are some cases where we do not want ripples to activate. Most of this logic is handled by native control attributes such as `disabled` on input fields. One caveat is that ripples will activate on disabled elements if their corresponding `label` element is clicked. For this, we override the ripple's `shouldIgnoreRippleActivation()` adapter method in `mdc-checkbox`'s `getDefaultFoundation()`, and check if the control is disabled before activating the ripple. + #### MDCCheckboxFoundation API ##### MDCCheckboxFoundation.isChecked() => boolean diff --git a/packages/mdc-checkbox/index.js b/packages/mdc-checkbox/index.js index ec1e9cb983c..e7844c4865d 100644 --- a/packages/mdc-checkbox/index.js +++ b/packages/mdc-checkbox/index.js @@ -64,6 +64,7 @@ export class MDCCheckbox extends MDCComponent { getDefaultFoundation() { return new MDCCheckboxFoundation({ + shouldIgnoreRippleActivation: () => this.disabled, addClass: (className) => this.root_.classList.add(className), removeClass: (className) => this.root_.classList.remove(className), registerAnimationEndHandler: diff --git a/packages/mdc-radio/README.md b/packages/mdc-radio/README.md index b7bd424e86d..6be91fae9e9 100644 --- a/packages/mdc-radio/README.md +++ b/packages/mdc-radio/README.md @@ -156,6 +156,8 @@ Since MDC Radio is primarily driven by its native control, the adapter API is ex | `addClass(className: string) => void` | Adds a class to the root element. | | `removeClass(className: string) => void` | Removes a class from the root element. | +> **NOTE**: There are some cases where we do not want ripples to activate. Most of this logic is handled by native control attributes such as `disabled` on input fields. One caveat is that ripples will activate on disabled elements if their corresponding `label` element is clicked. For this, we override the ripple's `shouldIgnoreRippleActivation()` adapter method in `mdc-radio`'s `getDefaultFoundation()`, and check if the control is disabled before activating the ripple. + #### The full foundation API ##### MDCRadioFoundation.isChecked() => boolean diff --git a/packages/mdc-radio/index.js b/packages/mdc-radio/index.js index 17a78bd1c99..650b57c5776 100644 --- a/packages/mdc-radio/index.js +++ b/packages/mdc-radio/index.js @@ -61,6 +61,7 @@ export class MDCRadio extends MDCComponent { initRipple_() { const adapter = Object.assign(MDCRipple.createAdapter(this), { + shouldIgnoreRippleActivation: () => this.disabled, isUnbounded: () => true, // Radio buttons technically go "active" whenever there is *any* keyboard interaction. This is not the // UI we desire. diff --git a/packages/mdc-ripple/README.md b/packages/mdc-ripple/README.md index d72bc46d0b3..c8b2670f048 100644 --- a/packages/mdc-ripple/README.md +++ b/packages/mdc-ripple/README.md @@ -260,6 +260,7 @@ ripple to. The adapter API is as follows: | `browserSupportsCssVars() => boolean` | Whether or not the given browser supports CSS Variables. When implementing this, please take the [Safari considerations](#caveat-safari) into account. We provide a `supportsCssVariables` function within the `util.js` which we recommend using, as it handles this for you. | | `isUnbounded() => boolean` | Whether or not the ripple should be considered unbounded. | | `isSurfaceActive() => boolean` | Whether or not the surface the ripple is acting upon is [active](https://www.w3.org/TR/css3-selectors/#useraction-pseudos). We use this to detect whether or not a keyboard event has activated the surface the ripple is on. This does not need to make use of `:active` (which is what we do); feel free to supply your own heuristics for it. | +| `shouldIgnoreRippleActivation() => boolean` | Initially set to false, this method is intended to be overridden at the individual component level so that the ripple knows whether or not to activate. e.g.: disabled `mdc-radio` components should not activate a ripple when their corresponding label is clicked. | | `addClass(className: string) => void` | Adds a class to the ripple surface | | `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`. | diff --git a/packages/mdc-ripple/foundation.js b/packages/mdc-ripple/foundation.js index d4c77621918..2789b8daa72 100644 --- a/packages/mdc-ripple/foundation.js +++ b/packages/mdc-ripple/foundation.js @@ -45,6 +45,7 @@ export default class MDCRippleFoundation extends MDCFoundation { browserSupportsCssVars: () => /* boolean - cached */ {}, isUnbounded: () => /* boolean */ {}, isSurfaceActive: () => /* boolean */ {}, + shouldIgnoreRippleActivation: () => /* boolean */ {}, addClass: (/* className: string */) => {}, removeClass: (/* className: string */) => {}, registerInteractionHandler: (/* evtType: string, handler: EventListener */) => {}, @@ -67,6 +68,7 @@ export default class MDCRippleFoundation extends MDCFoundation { constructor(adapter) { super(Object.assign(MDCRippleFoundation.defaultAdapter, adapter)); + this.layoutFrame_ = 0; this.frame_ = {width: 0, height: 0}; this.activationState_ = this.defaultActivationState_(); @@ -142,6 +144,10 @@ export default class MDCRippleFoundation extends MDCFoundation { } activate_(e) { + if (this.adapter_.shouldIgnoreRippleActivation()) { + return; + } + const {activationState_: activationState} = this; if (activationState.isActivated) { return; diff --git a/packages/mdc-ripple/index.js b/packages/mdc-ripple/index.js index 10fa98c3690..8f2d281ad84 100644 --- a/packages/mdc-ripple/index.js +++ b/packages/mdc-ripple/index.js @@ -37,6 +37,7 @@ export class MDCRipple extends MDCComponent { browserSupportsCssVars: () => supportsCssVariables(window), isUnbounded: () => instance.unbounded, isSurfaceActive: () => instance.root_[MATCHES](':active'), + shouldIgnoreRippleActivation: () => false, addClass: (className) => instance.root_.classList.add(className), removeClass: (className) => instance.root_.classList.remove(className), registerInteractionHandler: (evtType, handler) => instance.root_.addEventListener(evtType, handler), diff --git a/test/unit/mdc-ripple/foundation.test.js b/test/unit/mdc-ripple/foundation.test.js index 72d17ea8088..8265fb1abba 100644 --- a/test/unit/mdc-ripple/foundation.test.js +++ b/test/unit/mdc-ripple/foundation.test.js @@ -37,11 +37,12 @@ test('numbers returns constants.numbers', () => { assert.deepEqual(MDCRippleFoundation.numbers, numbers); }); -test('defaultAdapter returns a complete adapter implementation', () => { +test.only('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCRippleFoundation, [ - 'browserSupportsCssVars', 'isUnbounded', 'isSurfaceActive', 'addClass', 'removeClass', - 'registerInteractionHandler', 'deregisterInteractionHandler', 'registerResizeHandler', - 'deregisterResizeHandler', 'updateCssVariable', 'computeBoundingRect', 'getWindowPageOffset', + 'browserSupportsCssVars', 'isUnbounded', 'isSurfaceActive', 'shouldIgnoreRippleActivation', + 'addClass', 'removeClass', 'registerInteractionHandler', 'deregisterInteractionHandler', + 'registerResizeHandler', 'deregisterResizeHandler', 'updateCssVariable', + 'computeBoundingRect', 'getWindowPageOffset', ]); }); From 98997f8c031bac3a60385b02fa55ed5badbe2405 Mon Sep 17 00:00:00 2001 From: Alex Sheehan Date: Wed, 19 Apr 2017 09:38:34 -0400 Subject: [PATCH 2/4] fix(ripple): Ripple should not activate on disabled label click --- test/unit/mdc-ripple/foundation.test.js | 2 +- test/unit/mdc-ripple/mdc-ripple.test.js | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/test/unit/mdc-ripple/foundation.test.js b/test/unit/mdc-ripple/foundation.test.js index 8265fb1abba..ce6d384fb5b 100644 --- a/test/unit/mdc-ripple/foundation.test.js +++ b/test/unit/mdc-ripple/foundation.test.js @@ -37,7 +37,7 @@ test('numbers returns constants.numbers', () => { assert.deepEqual(MDCRippleFoundation.numbers, numbers); }); -test.only('defaultAdapter returns a complete adapter implementation', () => { +test('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCRippleFoundation, [ 'browserSupportsCssVars', 'isUnbounded', 'isSurfaceActive', 'shouldIgnoreRippleActivation', 'addClass', 'removeClass', 'registerInteractionHandler', 'deregisterInteractionHandler', diff --git a/test/unit/mdc-ripple/mdc-ripple.test.js b/test/unit/mdc-ripple/mdc-ripple.test.js index f336e9ed430..14ca87851f4 100644 --- a/test/unit/mdc-ripple/mdc-ripple.test.js +++ b/test/unit/mdc-ripple/mdc-ripple.test.js @@ -111,6 +111,12 @@ test('adapter#isSurfaceActive calls the correct :matches API method on the root assert.isOk(component.getDefaultFoundation().adapter_.isSurfaceActive()); }); +test('adapter#shouldIgnoreRippleActivation is initially set to return false', () => { + const {component} = setupTest(); + + assert.isFalse(component.getDefaultFoundation().adapter_.shouldIgnoreRippleActivation()); +}); + test('adapter#addClass adds a class to the root', () => { const {root, component} = setupTest(); component.getDefaultFoundation().adapter_.addClass('foo'); From 682b1b41567ee76494fa55fb0d454c34d7b7088a Mon Sep 17 00:00:00 2001 From: Alex Sheehan Date: Tue, 9 May 2017 13:43:28 -0400 Subject: [PATCH 3/4] test(ripple): change default adapter for ripple --- packages/mdc-checkbox/index.js | 1 - packages/mdc-radio/index.js | 1 - packages/mdc-ripple/foundation.js | 4 ++-- packages/mdc-ripple/index.js | 2 +- test/unit/mdc-ripple/foundation-activation.test.js | 14 ++++++++++++++ test/unit/mdc-ripple/foundation.test.js | 2 +- test/unit/mdc-ripple/mdc-ripple.test.js | 6 +++--- 7 files changed, 21 insertions(+), 9 deletions(-) diff --git a/packages/mdc-checkbox/index.js b/packages/mdc-checkbox/index.js index e7844c4865d..ec1e9cb983c 100644 --- a/packages/mdc-checkbox/index.js +++ b/packages/mdc-checkbox/index.js @@ -64,7 +64,6 @@ export class MDCCheckbox extends MDCComponent { getDefaultFoundation() { return new MDCCheckboxFoundation({ - shouldIgnoreRippleActivation: () => this.disabled, addClass: (className) => this.root_.classList.add(className), removeClass: (className) => this.root_.classList.remove(className), registerAnimationEndHandler: diff --git a/packages/mdc-radio/index.js b/packages/mdc-radio/index.js index 650b57c5776..17a78bd1c99 100644 --- a/packages/mdc-radio/index.js +++ b/packages/mdc-radio/index.js @@ -61,7 +61,6 @@ export class MDCRadio extends MDCComponent { initRipple_() { const adapter = Object.assign(MDCRipple.createAdapter(this), { - shouldIgnoreRippleActivation: () => this.disabled, isUnbounded: () => true, // Radio buttons technically go "active" whenever there is *any* keyboard interaction. This is not the // UI we desire. diff --git a/packages/mdc-ripple/foundation.js b/packages/mdc-ripple/foundation.js index 2789b8daa72..aaf9af0ad32 100644 --- a/packages/mdc-ripple/foundation.js +++ b/packages/mdc-ripple/foundation.js @@ -45,7 +45,7 @@ export default class MDCRippleFoundation extends MDCFoundation { browserSupportsCssVars: () => /* boolean - cached */ {}, isUnbounded: () => /* boolean */ {}, isSurfaceActive: () => /* boolean */ {}, - shouldIgnoreRippleActivation: () => /* boolean */ {}, + isSurfaceDisabled: () => /* boolean */ {}, addClass: (/* className: string */) => {}, removeClass: (/* className: string */) => {}, registerInteractionHandler: (/* evtType: string, handler: EventListener */) => {}, @@ -144,7 +144,7 @@ export default class MDCRippleFoundation extends MDCFoundation { } activate_(e) { - if (this.adapter_.shouldIgnoreRippleActivation()) { + if (this.adapter_.isSurfaceDisabled()) { return; } diff --git a/packages/mdc-ripple/index.js b/packages/mdc-ripple/index.js index 8f2d281ad84..f4919b2fb96 100644 --- a/packages/mdc-ripple/index.js +++ b/packages/mdc-ripple/index.js @@ -37,7 +37,7 @@ export class MDCRipple extends MDCComponent { browserSupportsCssVars: () => supportsCssVariables(window), isUnbounded: () => instance.unbounded, isSurfaceActive: () => instance.root_[MATCHES](':active'), - shouldIgnoreRippleActivation: () => false, + isSurfaceDisabled: () => instance.disabled, addClass: (className) => instance.root_.classList.add(className), removeClass: (className) => instance.root_.classList.remove(className), registerInteractionHandler: (evtType, handler) => instance.root_.addEventListener(evtType, handler), diff --git a/test/unit/mdc-ripple/foundation-activation.test.js b/test/unit/mdc-ripple/foundation-activation.test.js index 1be61071fdd..81fe74daa85 100644 --- a/test/unit/mdc-ripple/foundation-activation.test.js +++ b/test/unit/mdc-ripple/foundation-activation.test.js @@ -21,6 +21,20 @@ import {cssClasses, strings, numbers} from '../../../packages/mdc-ripple/constan suite('MDCRippleFoundation - Activation Logic'); +testFoundation('does nothing if component if isSurfaceDisabled is true', + ({foundation, adapter, mockRaf}) => { + const handlers = captureHandlers(adapter); + foundation.init(); + mockRaf.flush(); + + td.when(adapter.isSurfaceDisabled()).thenReturn(true); + + handlers.mousedown(); + + td.verify(adapter.addClass(cssClasses.BG_ACTIVE_FILL), {times: 0}); + td.verify(adapter.addClass(cssClasses.FG_ACTIVATION), {times: 0}); +}); + testFoundation('adds activation classes on mousedown', ({foundation, adapter, mockRaf}) => { const handlers = captureHandlers(adapter); foundation.init(); diff --git a/test/unit/mdc-ripple/foundation.test.js b/test/unit/mdc-ripple/foundation.test.js index ce6d384fb5b..32c0b8b74a2 100644 --- a/test/unit/mdc-ripple/foundation.test.js +++ b/test/unit/mdc-ripple/foundation.test.js @@ -39,7 +39,7 @@ test('numbers returns constants.numbers', () => { test('defaultAdapter returns a complete adapter implementation', () => { verifyDefaultAdapter(MDCRippleFoundation, [ - 'browserSupportsCssVars', 'isUnbounded', 'isSurfaceActive', 'shouldIgnoreRippleActivation', + 'browserSupportsCssVars', 'isUnbounded', 'isSurfaceActive', 'isSurfaceDisabled', 'addClass', 'removeClass', 'registerInteractionHandler', 'deregisterInteractionHandler', 'registerResizeHandler', 'deregisterResizeHandler', 'updateCssVariable', 'computeBoundingRect', 'getWindowPageOffset', diff --git a/test/unit/mdc-ripple/mdc-ripple.test.js b/test/unit/mdc-ripple/mdc-ripple.test.js index 14ca87851f4..fdd92fbff3e 100644 --- a/test/unit/mdc-ripple/mdc-ripple.test.js +++ b/test/unit/mdc-ripple/mdc-ripple.test.js @@ -111,10 +111,10 @@ test('adapter#isSurfaceActive calls the correct :matches API method on the root assert.isOk(component.getDefaultFoundation().adapter_.isSurfaceActive()); }); -test('adapter#shouldIgnoreRippleActivation is initially set to return false', () => { +test('adapter#shouldIgnoreRippleActivation is set to the instance\'s disabled state', () => { const {component} = setupTest(); - - assert.isFalse(component.getDefaultFoundation().adapter_.shouldIgnoreRippleActivation()); + component.disabled = true; + assert.isTrue(component.getDefaultFoundation().adapter_.shouldIgnoreRippleActivation()); }); test('adapter#addClass adds a class to the root', () => { From 4d68e606c29483265dff5c98df5c42246af6353e Mon Sep 17 00:00:00 2001 From: Alex Sheehan Date: Tue, 9 May 2017 13:49:46 -0400 Subject: [PATCH 4/4] test(ripple): update tests with new adapter methods --- packages/mdc-checkbox/README.md | 1 - packages/mdc-radio/README.md | 1 - packages/mdc-ripple/README.md | 2 +- test/unit/mdc-ripple/mdc-ripple.test.js | 4 ++-- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/mdc-checkbox/README.md b/packages/mdc-checkbox/README.md index 79ff52b7f41..90e93654c2d 100644 --- a/packages/mdc-checkbox/README.md +++ b/packages/mdc-checkbox/README.md @@ -205,7 +205,6 @@ The adapter for checkboxes must provide the following functions, with correct si | `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.` | -> **NOTE**: There are some cases where we do not want ripples to activate. Most of this logic is handled by native control attributes such as `disabled` on input fields. One caveat is that ripples will activate on disabled elements if their corresponding `label` element is clicked. For this, we override the ripple's `shouldIgnoreRippleActivation()` adapter method in `mdc-checkbox`'s `getDefaultFoundation()`, and check if the control is disabled before activating the ripple. #### MDCCheckboxFoundation API diff --git a/packages/mdc-radio/README.md b/packages/mdc-radio/README.md index 6be91fae9e9..e0b4f81ffa9 100644 --- a/packages/mdc-radio/README.md +++ b/packages/mdc-radio/README.md @@ -156,7 +156,6 @@ Since MDC Radio is primarily driven by its native control, the adapter API is ex | `addClass(className: string) => void` | Adds a class to the root element. | | `removeClass(className: string) => void` | Removes a class from the root element. | -> **NOTE**: There are some cases where we do not want ripples to activate. Most of this logic is handled by native control attributes such as `disabled` on input fields. One caveat is that ripples will activate on disabled elements if their corresponding `label` element is clicked. For this, we override the ripple's `shouldIgnoreRippleActivation()` adapter method in `mdc-radio`'s `getDefaultFoundation()`, and check if the control is disabled before activating the ripple. #### The full foundation API diff --git a/packages/mdc-ripple/README.md b/packages/mdc-ripple/README.md index c8b2670f048..0e61c567606 100644 --- a/packages/mdc-ripple/README.md +++ b/packages/mdc-ripple/README.md @@ -260,7 +260,7 @@ ripple to. The adapter API is as follows: | `browserSupportsCssVars() => boolean` | Whether or not the given browser supports CSS Variables. When implementing this, please take the [Safari considerations](#caveat-safari) into account. We provide a `supportsCssVariables` function within the `util.js` which we recommend using, as it handles this for you. | | `isUnbounded() => boolean` | Whether or not the ripple should be considered unbounded. | | `isSurfaceActive() => boolean` | Whether or not the surface the ripple is acting upon is [active](https://www.w3.org/TR/css3-selectors/#useraction-pseudos). We use this to detect whether or not a keyboard event has activated the surface the ripple is on. This does not need to make use of `:active` (which is what we do); feel free to supply your own heuristics for it. | -| `shouldIgnoreRippleActivation() => boolean` | Initially set to false, this method is intended to be overridden at the individual component level so that the ripple knows whether or not to activate. e.g.: disabled `mdc-radio` components should not activate a ripple when their corresponding label is clicked. | +| `isSurfaceDisabled() => boolean` | Whether or not the ripple is attached to a disabled component. If true, the ripple will not activate. | | `addClass(className: string) => void` | Adds a class to the ripple surface | | `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`. | diff --git a/test/unit/mdc-ripple/mdc-ripple.test.js b/test/unit/mdc-ripple/mdc-ripple.test.js index fdd92fbff3e..10ec77398fa 100644 --- a/test/unit/mdc-ripple/mdc-ripple.test.js +++ b/test/unit/mdc-ripple/mdc-ripple.test.js @@ -111,10 +111,10 @@ test('adapter#isSurfaceActive calls the correct :matches API method on the root assert.isOk(component.getDefaultFoundation().adapter_.isSurfaceActive()); }); -test('adapter#shouldIgnoreRippleActivation is set to the instance\'s disabled state', () => { +test('adapter#isSurfaceDisabled delegates to component\'s disabled getter', () => { const {component} = setupTest(); component.disabled = true; - assert.isTrue(component.getDefaultFoundation().adapter_.shouldIgnoreRippleActivation()); + assert.isTrue(component.getDefaultFoundation().adapter_.isSurfaceDisabled()); }); test('adapter#addClass adds a class to the root', () => {