Feat/implement improved graceful degradation ripple

[cristobalchao]

Resolves #189

[Garbee]

How exactly should the "enhanced" states look when tabbing through a document? Currently there is no indication of what button has focus which is extremely inaccessible.

[traviskaufman]

@cristobalchao not sure that this branch was cleanly rebased from master since it looks like there's a bunch of extemporaneous commits prior to 64b9db3. You may want to try and git pull --rebase origin master on this branch, checkout a fresh branch and cherry-pick 64b9db3 and 7a4df35 over, or use some other strategy.

[traviskaufman]

can we make those attributes data-demo-no-js instead of no-js so that they are valid HTML attrs? I also think having the demo in there is useful since it signals to users that the data attribute is not part of the component's API and exists for demo purposes only

[cristobalchao]

SGTM

[traviskaufman]

nit: How about "CSS Only" instead of "Gracefully degraded"?

[cristobalchao]

SGTM

[traviskaufman]

How about "CSS Only" instead of "Gracefully degraded"?

[cristobalchao]

SGTM

[traviskaufman]

should we add a media query here that sets it to 100% width if the viewport is smaller than 400px? I'm thinking of how this would appear on v. small screens

[cristobalchao]

SGTM

[Garbee]

Or just max-width: 400px; so on small screens it just gets smaller as-needed. But doesn't grow above that.

[traviskaufman]

same comment regarding no-js --> data-demo-no-js

[cristobalchao]

SGTM

[traviskaufman]

I think this can just be @include mdc-theme-dark { here. We use the first argument in the case where we need to style an BEM element class to adhere to a dark theme treatment

.mdc-component__element {
  @include mdc-theme-dark(".mdc-component") {
    // ...
  } 
}

And we usually use the second argument when we're styling a modifier class for dark theme treatment, e.g.

.mdc-component--mod {
  @include mdc-theme-dark(".mdc-component", true) {
    // ...
  }
}

The mdc-theme README has more info on this.

[cristobalchao]

SGTM

[traviskaufman]

Should we put a TODO here and in other components containing elevation that we want to disable ink wash on hover and alter elevation instead as part of #194?

[cristobalchao]

SGTM

[traviskaufman]

This paragraph looks a bit broken...

Maybe we should say:

MDC Ripple also works without javascript, where it gracefully degrades to a simpler CSS-Only implementation.

[cristobalchao]

Much better!

[traviskaufman]

I think we can remove the portion of text that says "when it contains this class", since I'm pretty sure "this class" refers to mdc-ripple-upgraded, which we now take care of automatically.

[cristobalchao]

SGTM

[traviskaufman]

Change "the ripple would be on a graceful degraded state" to "the ripple will still work, but it would use a simpler, CSS-Only implementation which relies on :hover, :active, and :focus".

[cristobalchao]

SGTM

[traviskaufman]

this should just be @include mdc-theme-dark { with no argument

[cristobalchao]

Yup!

[traviskaufman]

Can we switch this to a // Sass style comment? This way it's not output in the built CSS

[cristobalchao]

sgtm

[traviskaufman]

nit: :hover, :active, and :focus, not double-colon. Double-colons are for pseudo-elements, whereas single-colons are for pseudo-classes

[cristobalchao]

Ups, my bad!

[traviskaufman]

Should this just be transition: none; instead?

[cristobalchao]

Much better!

[traviskaufman]

Don't we have to remove the manual graceful degredation styles components that have ripples as well?

λ for f in $(grep -l '@material/ripple' $(find packages -name "package.json")); do echo $(dirname $f); done;
packages/material-components-web
packages/mdc-button
packages/mdc-checkbox
packages/mdc-fab
packages/mdc-icon-toggle
packages/mdc-list
packages/mdc-radio
packages/mdc-ripple

[traviskaufman]

Eek! This should be transform, not transition 😝 by bad! Mind fixing this?

[cristobalchao]

Lol! I totally missed that too. Fixed!

[sgomes]

Generally LGTM, left a few comments.

[sgomes]

The fallback ripple seems larger than the JS-enabled one. Is this intentional?

[sgomes]

Nit: no need for hyphen in "un-invasive".

[sgomes]

In order to make var() as bulletproof as possible, consider following the pattern:

transform: scale(0);
transform: scale(var(--mdc-ripple-fg-scale, 0));

(replace 0 with whatever default value you deem fit)

The first line gives you a fallback for browsers that don't understand custom properties, whereas the second value in the var() function gives you a fallback in case --mdc-ripple-fg-scale isn't defined.

Another option is to stick a big a big @supports around everything (which to be fair, you may already be doing in JavaScript), but you should at least keep the second parameter to var().

[sgomes]

Same comment as above, and everywhere else var() is used in the file.

[traviskaufman]

Do we need to use the dual-property method for .mdc-ripple-upgraded? I think we left that off initially b/c we don't even upgrade the ripple if Custom Vars aren't supported by the browser. Or, should we just do it for consistency?

[sgomes]

Yeah, if we're checking in JS, there's no need for the first fallback. Still might be useful to do var(--mdc-ripple-fg-size, 16px) (or whatever sensible default), in case --mdc-ripple-fg-size isn't defined.

[traviskaufman]

True, the second arg fallback is always a good call

[cristobalchao]

All issues addressed PTAL

[traviskaufman]

These style rules should be put within the .mdc-button declaration below, which is the top-level declaration for mdc-button. Having an extra one seems redundant, no?

[traviskaufman]

forgot to remove?

[cristobalchao]

Ups...my bad!

[traviskaufman]

I don't think we need the fallback here, since this gets styled by using mdc-ripple-upgraded--unbounded. Since the JS checks for CSS Var support before attaching any of the mdc-ripple-upgraded classes, we can guarantee here that CSS variables are supported and thus you can remove this.

[traviskaufman]

You can remove the inline comment; this is a convention in our codebase which we talk about in our mdc-theme package.

[traviskaufman]

Same comment here regarding not needing fallback for mdc-ripple-upgraded

[traviskaufman]

forgot to remove?

[cristobalchao]

My bad again!

[cristobalchao]

All issues addressed PTAL

[traviskaufman]

this fallback is not needed, since the property is under .mdc-ripple-upgraded

[traviskaufman]

Couple of issues I noticed in the demo:

• JS Checkboxes and JS Radios no longer seem to use the correct ripple functionality
• I think for upgraded ripples, there's a jump-cut when hovering over the elements. I think we need to remove the hover styles attached by the graceful degredation fallback when the ripple is upgraded. This is especially apparent for unbounded ripples.

[cristobalchao]

All issues addressed PTAL

[traviskaufman]

Excellent work @cristobalchao!

[omtinez]

Does this address elements retaining the "ripple" on focus? E.g. the radio buttons keep a "halo" after clicking them: http://i.imgur.com/NoP7SsU.png

I though I would verify since this seemed like it addresses that before opening a new issue. Also, related: are there any plans to allow users from disabling the ripple effects altogether?

[Garbee]

@omtinez That seems like intended functionality. That is focus state for accessibility purposes and is highly required.

Ripple effects are added via JavaScript instantiation. Otherwise there should be a CSS-only effect for these states that looks like a ripple but doesn't function like the actual ripples. If you use the JS though for these form controls, Ripples are basically bundled-in since they are a specification detail.

[cristobalchao]

@omtinez That's the intended behavior. As @Garbee mentioned, the focus state is required for all components for accessibility reasons. About disabling the ripple effects altogether, there are no plans in the short term future.

[omtinez]

I commend your focus on accessibility, but I think that the current look is a bit over the top (in my opinion, hideous) by making a radio button appear twice as big. Specially considering that the Javascript-enabled behavior is a ripple for a fraction of a second; I'm not sure that the Javascript-enabled behavior is much more accessible e.g. to someone visually impaired. I'm happy to open a separate issue if you are willing to discuss the idea there (or want it just for tracking purposes, even if it's going to be immediately closed).

For the sake of documenting, even though it breaks the accessibility argument, this is the CSS workaround that I'm using and disables the enormous halo around radio buttons:

.mdc-radio__background::before {
    display: none;
}

[Garbee]

These states are explicitly defined in the spec. So it adheres to MD guidance as-is and is accessible. Disabling this is discouraged unless you replace it with some other focus indicator for people who need this. However unless the MD team provides us with alternative measures to implement for a11y in this case it shouldn't be changed.

I believe the focus state is shown initially then removed with JS since that is a part of the ripple effect specification somewhere else. IMO it isn't as useful for a11y but that is purely subjective conjecture and I have no data to back it up to influence any changes.