Skip to content

Commit

Permalink
fix: Remove edge detection for CSS custom properties (#5264)
Browse files Browse the repository at this point in the history
BREAKING CHANGE: Removed `$edgeOptOut` option from `mdc-theme-prop()` Sass mixin.
  • Loading branch information
abhiomkar authored Nov 22, 2019
1 parent 19ecc29 commit fe444ac
Show file tree
Hide file tree
Showing 14 changed files with 20 additions and 158 deletions.
4 changes: 0 additions & 4 deletions packages/mdc-button/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,3 @@ Mixin | Description
`mdc-button-outline-color($color)` | Sets the outline color to the given color for an enabled button.
`mdc-button-disabled-outline-color($color)` | Sets the outline color to the given color for a disabled button.
`mdc-button-outline-width($width, $padding)` | Sets the outline width to the given number (defaults to 2px) and adjusts padding accordingly. `$padding` is only required in cases where `mdc-button-horizontal-padding` is also included with a custom value.

##### Caveat: Edge and CSS Custom Properties

In browsers that fully support CSS custom properties, the above mixins will work if you pass in a [MDC Theme](../mdc-theme) property (e.g. `primary`) as an argument. However, Edge does not fully support CSS custom properties. If you are using the `mdc-button-container-fill-color` mixin, you must pass in an actual color value for support in Edge.
2 changes: 1 addition & 1 deletion packages/mdc-button/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ $query: mdc-feature-all()) {
$feat-color: mdc-feature-create-target($query, color);

@include mdc-feature-targets($feat-color) {
@include mdc-theme-prop(background-color, $color, $edgeOptOut: true);
@include mdc-theme-prop(background-color, $color);
}
}

Expand Down
6 changes: 0 additions & 6 deletions packages/mdc-checkbox/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,6 @@ Mixin | Description

The ripple effect for the Checkbox component is styled using [MDC Ripple](../mdc-ripple) mixins.

#### Caveat: Edge and CSS Variables

In browsers that fully support CSS variables, MDC Checkbox references CSS variables wherever theme properties are used.
However, due to Edge's buggy CSS variable support, the `background-color` for `.mdc-checkbox__background::before` will not honor CSS variables in Edge.
This means you will need to override this style manually for Edge if you alter the CSS variable for the primary color.

## `MDCCheckbox` Properties and Methods

Property Name | Type | Description
Expand Down
4 changes: 2 additions & 2 deletions packages/mdc-checkbox/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ $mdc-checkbox-ripple-target: ".mdc-checkbox__ripple";
.mdc-checkbox__native-control:checked ~ .mdc-checkbox__background::before,
.mdc-checkbox__native-control:indeterminate ~ .mdc-checkbox__background::before {
@include mdc-feature-targets($feat-color) {
@include mdc-theme-prop(background-color, $color, $edgeOptOut: true);
@include mdc-theme-prop(background-color, $color);
}
}

Expand Down Expand Up @@ -626,7 +626,7 @@ $mdc-checkbox-ripple-target: ".mdc-checkbox__ripple";

.mdc-checkbox__background::before {
@include mdc-feature-targets($feat-color) {
@include mdc-theme-prop(background-color, on-surface, $edgeOptOut: true);
@include mdc-theme-prop(background-color, on-surface);
}
}

Expand Down
4 changes: 0 additions & 4 deletions packages/mdc-fab/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,6 @@ Mixin | Description

The ripple effect for the FAB component is styled using [MDC Ripple](../mdc-ripple) mixins.

#### Caveat: Edge and CSS Variables

In browsers that fully support CSS custom properties, the above mixins will work if you pass in a [MDC Theme](../mdc-theme) property (e.g. `primary`) as an argument. However, Edge does not fully support CSS custom properties. If you are using the `mdc-fab-container-color` mixin, you must pass in an actual color value for support in Edge.

### Additional Information

#### Accessibility
Expand Down
2 changes: 1 addition & 1 deletion packages/mdc-fab/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ $mdc-fab-ripple-target: ".mdc-fab__ripple";
$feat-color: mdc-feature-create-target($query, color);

@include mdc-feature-targets($feat-color) {
@include mdc-theme-prop(background-color, $color, $edgeOptOut: true);
@include mdc-theme-prop(background-color, $color);
}
}

Expand Down
4 changes: 0 additions & 4 deletions packages/mdc-radio/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,6 @@ Mixin | Description
`mdc-radio-ripple-size($size)` | Sets custom ripple size of radio.
`mdc-radio-density($density-scale)` | Sets density scale for radio. Supported density scale values are `-3`, `-2`, `-1` and `0` (default).

#### Caveat: Edge and CSS Custom Properties

In browsers that fully support CSS custom properties, the above mixins will work if you pass in a [MDC Theme](../mdc-theme) property (e.g. `primary`) as an argument. However, Edge does not fully support CSS custom properties. If you are using any of the Sass mixins, you must pass in an actual color value for support in Edge.

## `MDCRadio` Properties and Methods

Property | Value Type | Description
Expand Down
2 changes: 1 addition & 1 deletion packages/mdc-radio/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ $mdc-radio-ripple-target: ".mdc-radio__ripple";

.mdc-radio__background::before {
@include mdc-feature-targets($feat-color) {
@include mdc-theme-prop(background-color, $color, $edgeOptOut: true);
@include mdc-theme-prop(background-color, $color);
}
}
}
Expand Down
13 changes: 1 addition & 12 deletions packages/mdc-ripple/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ Method Signature | Description
| `computeBoundingRect() => ClientRect` | Returns the ClientRect for the surface |
| `getWindowPageOffset() => {x: number, y: number}` | Returns the `page{X,Y}Offset` values for the window object |

> _NOTE_: When implementing `browserSupportsCssVars`, please take the [Edge](#caveat-edge) and [Safari 9](#caveat-safari) considerations into account. We provide a `supportsCssVariables` function within the `util.js` which we recommend using, as it handles this for you.
> _NOTE_: When implementing `browserSupportsCssVars`, please take the [Safari 9](#caveat-safari) considerations into account. We provide a `supportsCssVariables` function within the `util.js` which we recommend using, as it handles this for you.
### `MDCRippleFoundation`

Expand Down Expand Up @@ -308,17 +308,6 @@ Method Signature | Description
## Caveats

### Caveat: Edge

> TL;DR ripples are disabled in Edge because of issues with its support of CSS variables in pseudo elements.
Edge introduced CSS variables in version 15. Unfortunately, there are
[known issues](https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11495448/)
involving its implementation for pseudo-elements, which cause ripples to behave incorrectly.
We feature-detect Edge's buggy behavior as it pertains to `::before`, and do not initialize ripples if the bug is
observed. Earlier versions of Edge (and IE) do not support CSS variables at all,
and as such ripples are never initialized.

### Caveat: Safari 9

> TL;DR ripples are disabled in Safari 9 because of a bug with CSS variables.
Expand Down
21 changes: 1 addition & 20 deletions packages/mdc-ripple/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@

@mixin mdc-ripple-common($query: mdc-feature-all()) {
$feat-animation: mdc-feature-create-target($query, animation);
$feat-structure: mdc-feature-create-target($query, structure);

// Ensure that styles needed by any component using MDC Ripple are emitted, but only once.
// (Every component using MDC Ripple imports these mixins, but doesn't necessarily import
Expand All @@ -78,24 +77,6 @@
@include mdc-ripple-keyframes_;
}
}

@include mdc-feature-targets($feat-structure) {
@include mdc-base-emit-once("mdc-ripple/common/structure") {
// Styles used to detect buggy behavior of CSS custom properties in Edge.
// See: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11495448/
// This is included in _mixins.scss rather than mdc-ripple.scss so that it will be
// present for other components which rely on ripple as well as mdc-ripple itself.
.mdc-ripple-surface--test-edge-var-bug {
--mdc-ripple-surface-test-edge-var: 1px solid #000;

visibility: hidden;

&::before {
border: var(--mdc-ripple-surface-test-edge-var);
}
}
}
}
}

@mixin mdc-ripple-surface($query: mdc-feature-all(), $ripple-target: "&") {
Expand Down Expand Up @@ -200,7 +181,7 @@
#{$ripple-target}::after {
@include mdc-feature-targets($feat-color) {
@if alpha(mdc-theme-prop-value($color)) > 0 {
@include mdc-theme-prop(background-color, $color, $edgeOptOut: true);
@include mdc-theme-prop(background-color, $color);
} @else {
// If a color with 0 alpha is specified, don't render the ripple pseudo-elements at all.
// This avoids unnecessary transitions and overflow.
Expand Down
29 changes: 2 additions & 27 deletions packages/mdc-ripple/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,6 @@ import {MDCRipplePoint} from './types';
*/
let supportsCssVariables_: boolean | undefined;

function detectEdgePseudoVarBug(windowObj: Window): boolean {
// Detect versions of Edge with buggy var() support
// See: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11495448/
const document = windowObj.document;
const node = document.createElement('div');
node.className = 'mdc-ripple-surface--test-edge-var-bug';
// Append to head instead of body because this script might be invoked in the
// head, in which case the body doesn't exist yet. The probe works either way.
document.head.appendChild(node);

// The bug exists if ::before style ends up propagating to the parent element.
// Additionally, getComputedStyle returns null in iframes with display: "none" in Firefox,
// but Firefox is known to support CSS custom properties correctly.
// See: https://bugzilla.mozilla.org/show_bug.cgi?id=548397
const computedStyle = windowObj.getComputedStyle(node);
const hasPseudoVarBug = computedStyle !== null && computedStyle.borderTopStyle === 'solid';
if (node.parentNode) {
node.parentNode.removeChild(node);
}
return hasPseudoVarBug;
}

export function supportsCssVariables(windowObj: Window, forceRefresh = false): boolean {
const {CSS} = windowObj;
let supportsCssVars = supportsCssVariables_;
Expand All @@ -70,11 +48,8 @@ export function supportsCssVariables(windowObj: Window, forceRefresh = false): b
CSS.supports('color', '#00000000')
);

if (explicitlySupportsCssVars || weAreFeatureDetectingSafari10plus) {
supportsCssVars = !detectEdgePseudoVarBug(windowObj);
} else {
supportsCssVars = false;
}
supportsCssVars =
explicitlySupportsCssVars || weAreFeatureDetectingSafari10plus;

if (!forceRefresh) {
supportsCssVariables_ = supportsCssVars;
Expand Down
2 changes: 1 addition & 1 deletion packages/mdc-theme/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ CSS Class | Description

Mixin | Description
--- | ---
`mdc-theme-prop($property, $style, $important, $edgeOptOut)` | Applies a theme color or a custom color to a CSS property, optionally with `!important`. If `$edgeOptOut` is `true` and a theme color is passed, the style will be wrapped in a `@supports` clause to exclude the style in Edge to avoid issues with its buggy CSS variable support.
`mdc-theme-prop($property, $style, $important)` | Applies a theme color or a custom color to a CSS property, optionally with `!important`.

#### `mdc-theme-prop` Properties

Expand Down
75 changes: 10 additions & 65 deletions packages/mdc-theme/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -65,78 +65,23 @@
// Applies the correct theme color style to the specified property.
// $property is typically color or background-color, but can be any CSS property that accepts color values.
// $style should be one of the map keys in $mdc-theme-property-values (_variables.scss), or a color value.
// $edgeOptOut controls whether to feature-detect around Edge to avoid emitting CSS variables for it,
// intended for use in cases where interactions with pseudo-element styles cause problems due to Edge bugs.
@mixin mdc-theme-prop($property, $style, $important: false, $edgeOptOut: false) {
@mixin mdc-theme-prop($property, $style, $important: false) {
$important-rule: if($important, "!important", "");

@if mdc-theme-is-var-with-fallback_($style) {
@if $important {
#{$property}: mdc-theme-get-var-fallback_($style) !important;
/* @alternate */
#{$property}: mdc-theme-var_($style) !important;
} @else {
#{$property}: mdc-theme-get-var-fallback_($style);
/* @alternate */
#{$property}: mdc-theme-var_($style);
}
#{$property}: mdc-theme-get-var-fallback_($style) #{$important-rule};
/* @alternate */
#{$property}: mdc-theme-var_($style) #{$important-rule};
} @else if mdc-theme-is-valid-theme-prop-value_($style) {
@if $important {
#{$property}: $style !important;
} @else {
#{$property}: $style;
}
#{$property}: $style #{$important-rule};
} @else {
@if not map-has-key($mdc-theme-property-values, $style) {
@error "Invalid style: '#{$style}'. Choose one of: #{map-keys($mdc-theme-property-values)}";
}

$value: map-get($mdc-theme-property-values, $style);

@if $important {
#{$property}: $value !important;

@if $edgeOptOut {
// stylelint-disable max-nesting-depth
@at-root {
// IE 11 doesn't understand this syntax and ignores the entire block.
// Edge understands this syntax and skips the entire block to avoid a nasty :before/:after pseudo-element bug.
// All other browsers apply the styles within the block.
@supports not (-ms-ime-align: auto) {
// stylelint-disable scss/selector-no-redundant-nesting-selector
& {
/* @alternate */
#{$property}: var(--mdc-theme-#{$style}, $value) !important;
}
// stylelint-enable scss/selector-no-redundant-nesting-selector
}
}
// stylelint-enable max-nesting-depth
} @else {
/* @alternate */
#{$property}: var(--mdc-theme-#{$style}, $value) !important;
}
} @else {
#{$property}: $value;

@if $edgeOptOut {
// stylelint-disable max-nesting-depth
@at-root {
// IE 11 doesn't understand this syntax and ignores the entire block.
// Edge understands this syntax and skips the entire block to avoid a nasty :before/:after pseudo-element bug.
// All other browsers apply the styles within the block.
@supports not (-ms-ime-align: auto) {
// stylelint-disable scss/selector-no-redundant-nesting-selector
& {
/* @alternate */
#{$property}: var(--mdc-theme-#{$style}, $value);
}
// stylelint-enable scss/selector-no-redundant-nesting-selector
}
}
// stylelint-enable max-nesting-depth
} @else {
/* @alternate */
#{$property}: var(--mdc-theme-#{$style}, $value);
}
}
#{$property}: $value #{$important-rule};
/* @alternate */
#{$property}: var(--mdc-theme-#{$style}, $value) #{$important-rule};
}
}
10 changes: 0 additions & 10 deletions test/unit/mdc-ripple/util.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,6 @@ test('#supportsCssVariables returns true when getComputedStyle returns null (e.g
assert.equal(windowObj.appendedNodes, 0, 'All nodes created in #supportsCssVariables should be removed');
});

test('#supportsCssVariables returns false when feature-detecting Edge var() bug with pseudo selectors', () => {
const windowObj = createMockWindowForCssVariables();
td.when(windowObj.CSS.supports('--css-vars', td.matchers.anything())).thenReturn(true);
td.when(windowObj.getComputedStyle(td.matchers.anything())).thenReturn({
borderTopStyle: 'solid',
});
assert.isNotOk(util.supportsCssVariables(windowObj, true), 'false if Edge bug is detected');
assert.equal(windowObj.appendedNodes, 0, 'All nodes created in #supportsCssVariables should be removed');
});

test('#supportsCssVariables returns false when CSS.supports() returns false for css vars', () => {
const windowObj = {
CSS: {
Expand Down

0 comments on commit fe444ac

Please sign in to comment.