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(cdk/a11y): make cdk-high-contrast work w/ emulated view encapsulation #18152

Open
wants to merge 3 commits into
base: master
from

Conversation

@jelbourn
Copy link
Member

jelbourn commented Jan 11, 2020

Say you have this in your component:

.some-element {
  @include cdk-high-contrast() {
    border: 1px solid white;
  }
}

With this change, this will output:

.cdk-high-contrast-active .some-element,
.cdk-high-contrast-active :host .some-element {
  border: 1px solid white;
}

Here, .cdk-high-contrast-active .some-element will apply in places
where encapsulation is turned off, and .cdk-high-contrast-active :host .some-element will apply in cases where encapsulation is emulated.
Neither will work in Shadow DOM (which we don't officially support).
AFAIK, Shadow DOM would need to use :host-content(), which we could
consider adding if we get an additional request.

This adds a few more bytes, but high-contrast styles tend to be pretty
limited.

Fixes #18147

@jelbourn jelbourn added this to the 9.0.0 milestone Jan 11, 2020
@jelbourn jelbourn requested a review from devversion as a code owner Jan 11, 2020
@googlebot googlebot added the cla: yes label Jan 11, 2020
@jelbourn jelbourn requested a review from crisbeto Jan 11, 2020
@jelbourn jelbourn force-pushed the jelbourn:high-contrast-fix branch from d19608a to 7257817 Jan 11, 2020
@@ -35,7 +35,8 @@
// context. We address this by nesting the selector context under .cdk-high-contrast.
@at-root {
$selector-context: #{&};
.cdk-high-contrast-#{$target} {
.cdk-high-contrast-#{$target},
.cdk-high-contrast-#{$target} :host {

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 11, 2020

Member

I tested it out and this breaks the high contrast styles for Edge and IE for unencapsulated components. The problem is that when encapsulation is disabled, Angular leaves the :host in place which IE/Edge don't support and causes the entire selector to be invalidated. It can be worked around by outputting a separate selector, but it means that we generate double the amount of CSS:

.cdk-high-contrast-#{$target} {
  @content;
}

.cdk-high-contrast-#{$target} :host {
  @content;
}

We could also add another parameter to the mixin that allows the consumer to tell us whether the styles are encapsulated and we can output the correct styles based on it. By default it can output both.

This comment has been minimized.

Copy link
@biolauri

biolauri Jan 11, 2020

I'd definitely prefer to add a parameter to toggle this.

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 15, 2020

Author Member

I see- I was assuming IE would ignore a pseudo-class it didn't recognize. I pushed an update that adds a param, defaulting to emitting both styles for maximum compatibility.

…tion

Say you have this in your component:
```scss
.some-element {
  @include cdk-high-contrast() {
    border: 1px solid white;
  }
}
```

With this change, this will output:
```scss
.cdk-high-contrast-active .some-element,
.cdk-high-contrast-active :host .some-element {
  border: 1px solid white;
}
```

Here, `.cdk-high-contrast-active .some-element` will apply in places
where encapsulation is turned off, and `.cdk-high-contrast-active :host
.some-element` will apply in cases where encapsulation is emulated.
Neither will work in Shadow DOM (which we don't officially support).
AFAIK, Shadow DOM would need to use `:host-content()`, which we could
consider adding if we get an additional request.

This adds a few more bytes, but high-contrast styles tend to be pretty
limited.
@jelbourn jelbourn force-pushed the jelbourn:high-contrast-fix branch from 7257817 to f6643c9 Jan 15, 2020
Copy link

biolauri left a comment

LGTM, thanks for the quick PR & fix for (my|the) issue! 🙇

/// * `on` - works for `Emulated`, `Native`, and `ShadowDom`
/// * `off` - works for `None`
/// * `both` - works for all encapsulation modes by emitting the CSS twice (default).
@mixin cdk-high-contrast($target: active, $encapsulation: 'both') {

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 15, 2020

Member

I'm not sure the both value makes sense since encapsulation is either on or off.

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 16, 2020

Author Member

What would you do instead? I want the mixin to "just work" by default, even if it is emitting extra styles.

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 16, 2020

Member

I think the thing that's throwing me off is that it's called both which sounds like it can have more than one style encapsulations at the same time. Maybe can make it on | off | undefined and output both styles if it's emitted? Another approach could be to match the naming from Angular.

This comment has been minimized.

Copy link
@biolauri

biolauri Jan 16, 2020

I understand your point, but I think, both is fine here, because with this option, the mixin supports both general states of view/style encapsulation. So it's rather an option of which policies are supported.
Matching the naming from Angular would be complicated, as AFAIK there's no explicit naming from Angular which addresses Emulated, Native, and ShadowDom together.

undefined is not a restricted value in Sass, so null would be the way to go. But I'm feeling that both fits better.

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 16, 2020

Author Member

How about 'any'?

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 16, 2020

Member

Sure, that works too.

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 16, 2020

Author Member

Changed to any

Copy link
Member

crisbeto left a comment

LGTM, but the CI is failing. It looks like not all references to both were updated.

@crisbeto crisbeto added the pr: lgtm label Jan 16, 2020
@jelbourn

This comment has been minimized.

Copy link
Member Author

jelbourn commented Jan 16, 2020

That's what I get for using the in-browser editor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.