feat(cdk/a11y): add high-contrast mode detection #17378

jelbourn · 2019-10-11T20:50:25Z

Add a new HighContrastModeDetector service that's can detect whether
the browser is in high-contrast mode (and which variety).
Augments A11yModule to use this serve to add classes to the document
body indicating the high-contrast mode
Adds A11yModule to MatCommonModule so that this runs for all of
the material components
Updates the cdk-high-contrast Sass mixin to include the new CSS
classes

mmalerba · 2019-10-11T22:37:53Z

src/cdk/a11y/_a11y.scss

@@ -28,4 +28,10 @@
  @media (-ms-high-contrast: $target) {


can we detect this in JS and apply the appropriate class instead so that we don't need to duplicate all of the styles?

We potentially don't need the @media query at all if the class works for IE11 and Edge, but I don't have a Windows machine available to confirm at the moment (our test workstation is busted). I'll chat with Kristiyan about it

Sounds good. If what you have currently doesn't work we can probably add some extra logic to detect on IE/Edge which would likely wind up smaller than duplicating all the CSS

I wasn't super concerned because the high-contrast styles tend to be pretty small and relatively infrequent, but agreed that it's better to avoid if possible.

crisbeto

I tested this out against IE, Edge, Firefox and Chrome on Windows. These are the results:

IE and Edge work after fixing the issue with classList that I commented on below and I tweaked the color for detecting black-on-white mode.
Firefox also seems to work since it follows a similar implementation to the Microsoft browsers.
It doesn't detect high contrast mode in Chrome that is added through the extension. It looks like Chrome returns the original colors from getComputedStyle. We could work around it by checking the attribute that the extension sets on the body, but that might be a little fragile.

I like the approach since it allows us to at least cover Firefox users which we couldn't before, however I'm not sure whether we can apply the same styles to all browsers and expect it to work consistently. E.g. the places where we add an outline are working correctly, but some of the more advanced cases, like using a border-image to remove the border from form fields, don't work on Firefox.

crisbeto · 2019-10-13T07:15:28Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+    // Create a test element with an arbitrary background-color that is neither black nor
+    // white; high-contrast mode will coerce the color to either black or white. Also ensure that
+    // appending the test element to the DOM does not affect layout by absolutely positioning it
+    const testElement = this._document.createElement('div');


Should we cache this in a similar way to how we cache the viewport size? It's unlikely for this to change dynamically and people might end up calling it frequently, if they don't know that it can be expensive.

We invalidate the viewport size cache on window resize, though. We don't have a similar way to know to invalidate for this, so I want to avoid caching for now.

crisbeto · 2019-10-13T07:16:21Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+    // Get the computed style ofor the background color, collapsing spaces to normalize between
+    // browsers. Once we get this color, we no longer need the test element. Access the `window`
+    // via the document so we can fake it in tests.
+    const documentWindow = this._document.defaultView!;


FWIW you could also fake it in tests using spyOn(window, 'getComputedStyle').and.returnValue.

I know, I'm just not a big fan of using Jasmine spies in place of more full-blown fakes.

crisbeto · 2019-10-13T07:17:34Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+  _applyBodyHighContrastModeCssClasses(): void {
+    if (this._platfrom.isBrowser) {
+      const body = this._document.body;
+      body.classList.remove(


The signature of classlist.remove with multiple classes isn't supported on IE. You need to do 3 separate remove calls.

crisbeto · 2019-10-13T07:18:01Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+
+
+/** Set of possible high-contrast mode backgrounds. */
+export enum HighContrastMode {


Should it be a const enum so that TS inlines the values during compilation?

Done, meant to do that

crisbeto · 2019-10-13T07:24:13Z

src/cdk/a11y/_a11y.scss

@@ -28,4 +28,10 @@
  @media (-ms-high-contrast: $target) {
    @content;
  }
+
+  @at-root {
+    .cdk-high-contrast-#{$target} #{&} {


While we're at it, should we have some compile-time validation that the target is valid?

This produces an unintended effect for cases where #{&} includes multiple comma-delimited selectors.

For example, consider the following Sass:

.element-a, .element-b { @include cdk-high-contrast() { background: blue; } }

This will actually produce the following:

.cdk-high-contrast .element-a, .element-b { background: blue; }

https://www.sassmeister.com/gist/30c179f33ac9dc40dab1ef20cd374dfc

In short, Sass is not prepending .cdk-high-contrast to each selector and instead just putting it once before the whole rule.

Looks like this fix may be viable, where we loop over each item in the active selector:

@at-root { @each $i in & { .cdk-high-contrast #{$i} { @content } } }

Thanks for looking into the failures. Using @each here will duplicate the content for each part of the compound selector. Instead I did this:

@at-root { $selector-context: #{&}; .cdk-high-contrast-#{$target} { #{$selector-context} { @content } } }

For the "body not present yet" issue, I'm going to just add the check for now and we can figure out if further action is necessary as a follow-up. I suspect this is really just an issue with tests and not with real applications.

crisbeto · 2019-10-13T07:26:30Z

src/cdk/a11y/public-api.ts

+export {
+  HighContrastModeDetector,
+  HighContrastMode,
+}from './high-contrast-mode/high-contrast-mode-detector';


Needs a space before from.

crisbeto · 2019-10-13T07:28:06Z

src/material/core/common-behaviors/common-module.ts

@@ -53,7 +54,7 @@ export interface GranularSanityChecks {
 * This module should be imported to each top-level component module (e.g., MatTabsModule).
 */
 @NgModule({
-  imports: [BidiModule],
+  imports: [A11yModule, BidiModule],


I'm not sure whether we should be adding the A11yModule to all of the Material modules since it's one of our largest modules and we only need the high contrast detection. It might be better to just inject the HighContrastModeDetector since it's a tree-shakeable provider and add the call to set the body class. We can do the same in the A11yModule to cover users that import the CDK, but not Material.

Done out of additional caution, but adding A11yModule shouldn't increase any size if we don't actually consume any of the directives or injectables.

crisbeto · 2019-10-13T07:30:47Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+
+      const mode = this.getHighContrastMode();
+      if (mode === HighContrastMode.BLACK_ON_WHITE) {
+        body.classList.add(HIGH_CONTRAST_MODE_ACTIVE_CSS_CLASS, BLACK_ON_WHITE_CSS_CLASS);


IE doesn't support add with multiple arguments either.

crisbeto · 2019-10-13T07:35:47Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+
+    switch (computedColor) {
+      case 'rgb(0,0,0)': return HighContrastMode.WHITE_ON_BLACK;
+      case 'rgb(1,1,1)': return HighContrastMode.BLACK_ON_WHITE;


The white-on-black detection here works in IE, but not the black-on-white. The return value here should be rgb(255,255,255).

Yep, this should always have been 255; pure brain misfire. Done.

Once in a while we hit an issue because we use some of the `classList` methods in a way that isn't supported in all browsers (e.g. passing multiple parameters to `add` in angular#17378). These changes add a simple lint rule to catch these cases earlier.

Once in a while we hit an issue because we use some of the `classList` methods in a way that isn't supported in all browsers (e.g. passing multiple parameters to `add` in #17378). These changes add a simple lint rule to catch these cases earlier.

jelbourn

@crisbeto addressed comments, could you take another look? In particular, I want to see if removing the ms- media queries still works for IE

jelbourn · 2019-10-14T17:50:49Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+    // Create a test element with an arbitrary background-color that is neither black nor
+    // white; high-contrast mode will coerce the color to either black or white. Also ensure that
+    // appending the test element to the DOM does not affect layout by absolutely positioning it
+    const testElement = this._document.createElement('div');


We invalidate the viewport size cache on window resize, though. We don't have a similar way to know to invalidate for this, so I want to avoid caching for now.

jelbourn · 2019-10-14T17:59:05Z

src/cdk/a11y/_a11y.scss

@@ -28,4 +28,10 @@
  @media (-ms-high-contrast: $target) {
    @content;
  }
+
+  @at-root {
+    .cdk-high-contrast-#{$target} #{&} {


jelbourn · 2019-10-14T17:59:36Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+
+
+/** Set of possible high-contrast mode backgrounds. */
+export enum HighContrastMode {


Done, meant to do that

jelbourn · 2019-10-14T18:00:26Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+    // Get the computed style ofor the background color, collapsing spaces to normalize between
+    // browsers. Once we get this color, we no longer need the test element. Access the `window`
+    // via the document so we can fake it in tests.
+    const documentWindow = this._document.defaultView!;


I know, I'm just not a big fan of using Jasmine spies in place of more full-blown fakes.

jelbourn · 2019-10-14T18:02:43Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+
+    switch (computedColor) {
+      case 'rgb(0,0,0)': return HighContrastMode.WHITE_ON_BLACK;
+      case 'rgb(1,1,1)': return HighContrastMode.BLACK_ON_WHITE;


Yep, this should always have been 255; pure brain misfire. Done.

jelbourn · 2019-10-14T18:04:45Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+  _applyBodyHighContrastModeCssClasses(): void {
+    if (this._platfrom.isBrowser) {
+      const body = this._document.body;
+      body.classList.remove(


jelbourn · 2019-10-14T18:04:48Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+
+      const mode = this.getHighContrastMode();
+      if (mode === HighContrastMode.BLACK_ON_WHITE) {
+        body.classList.add(HIGH_CONTRAST_MODE_ACTIVE_CSS_CLASS, BLACK_ON_WHITE_CSS_CLASS);


jelbourn · 2019-10-14T18:05:04Z

src/cdk/a11y/public-api.ts

+export {
+  HighContrastModeDetector,
+  HighContrastMode,
+}from './high-contrast-mode/high-contrast-mode-detector';


jelbourn · 2019-10-14T18:10:44Z

src/material/core/common-behaviors/common-module.ts

@@ -53,7 +54,7 @@ export interface GranularSanityChecks {
 * This module should be imported to each top-level component module (e.g., MatTabsModule).
 */
 @NgModule({
-  imports: [BidiModule],
+  imports: [A11yModule, BidiModule],


Done out of additional caution, but adding A11yModule shouldn't increase any size if we don't actually consume any of the directives or injectables.

crisbeto

LGTM. I re-tested it and it seems to work without the -ms- media queries. Some of the workarounds that I did for IE/Edge don't seem to be working for Firefox though (e.g. the one in the form field I mentioned above).

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

jelbourn · 2019-10-15T18:28:28Z

Comments addressed. For cases like form-field, we can potentially add handing in specific components that checks both the platform and the high-contrast mode to apply alternative styles.

andrewseguin · 2019-11-06T19:14:32Z

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts

+  /** Applies CSS classes indicating high-contrast mode to document body (browser-only). */
+  _applyBodyHighContrastModeCssClasses(): void {
+    if (this._platfrom.isBrowser) {
+      const bodyClasses = this._document.body.classList;


It appears that this function may be called too early, when body does not yet exist (e.g. if script is in head). It looks like we need some delay to make sure this is applied.

See _checkThemeIsPresent in common-module.ts for a simliar comment about checking for this._document.body

andrewseguin · 2019-11-06T20:13:00Z

Confirmed that two changes need to occur, and then all internal tests pass.

In _a11y.scss, the mixin needs to be changed to loop over selectors.

@at-root {
  @each $i in & {
    .cdk-high-contrast-#{$target} #{$i} {
      @content
    }
  }
}

In high-contrast-mode-detector.ts, document.body may not exist at the moment and needs to be delayed until it does.

* Add a new `HighContrastModeDetector` service that's can detect whether the browser is in high-contrast mode (and which variety). * Augments `A11yModule` to use this serve to add classes to the document body indicating the high-contrast mode * Have `MatCommonModule` apply high-contrast css classes * Replace `ms-` media queries in `cdk-high-contrast` Sass mixin with the new CSS classes

jelbourn · 2019-11-07T12:41:47Z

@andrewseguin updated the code so that it should work universally now. I didn't use the Sass loop since that would end up duplicating the content; what I did instead should still fix the problem.

I tried to run a presubmit (since it's 4am in California now), but I can't get my ssh session to pick up my security key

angular-automatic-lock-bot · 2019-12-09T00:06:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jelbourn added Accessibility This issue is related to accessibility (a11y) target: major This PR is targeted for the next major release labels Oct 11, 2019

jelbourn requested review from crisbeto and mmalerba October 11, 2019 20:50

jelbourn requested a review from devversion as a code owner October 11, 2019 20:50

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 11, 2019

jelbourn force-pushed the high-contrast-mode-detector branch from 284d733 to 31024f2 Compare October 11, 2019 21:02

mmalerba reviewed Oct 11, 2019

View reviewed changes

crisbeto reviewed Oct 13, 2019

View reviewed changes

crisbeto mentioned this pull request Oct 13, 2019

build: add linting for classList usage #17388

Merged

jelbourn force-pushed the high-contrast-mode-detector branch from 31024f2 to 6e3873b Compare October 14, 2019 18:22

jelbourn commented Oct 14, 2019

View reviewed changes

jelbourn force-pushed the high-contrast-mode-detector branch from 6e3873b to c4b930c Compare October 14, 2019 20:21

crisbeto approved these changes Oct 14, 2019

View reviewed changes

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts Outdated Show resolved Hide resolved

crisbeto reviewed Oct 14, 2019

View reviewed changes

src/cdk/a11y/high-contrast-mode/high-contrast-mode-detector.ts Outdated Show resolved Hide resolved

jelbourn force-pushed the high-contrast-mode-detector branch 2 times, most recently from c212d57 to 5f30ca0 Compare October 15, 2019 18:26

jelbourn added G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround labels Oct 15, 2019

jelbourn force-pushed the high-contrast-mode-detector branch from 5f30ca0 to 7627011 Compare October 15, 2019 18:53

mmalerba approved these changes Oct 16, 2019

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 16, 2019

jelbourn force-pushed the high-contrast-mode-detector branch from 7627011 to 3a1a20e Compare October 16, 2019 15:33

andrewseguin reviewed Nov 6, 2019

View reviewed changes

jelbourn force-pushed the high-contrast-mode-detector branch 2 times, most recently from 2ede62b to 4f2af2c Compare November 7, 2019 11:19

jelbourn force-pushed the high-contrast-mode-detector branch from 4f2af2c to 697c3ea Compare November 7, 2019 12:32

andrewseguin merged commit 6b7f091 into angular:master Nov 8, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 9, 2019



		/** Set of possible high-contrast mode backgrounds. */
		export enum HighContrastMode {

feat(cdk/a11y): add high-contrast mode detection #17378

feat(cdk/a11y): add high-contrast mode detection #17378

Conversation

jelbourn commented Oct 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

jelbourn commented Oct 15, 2019

Choose a reason for hiding this comment

andrewseguin commented Nov 6, 2019

jelbourn commented Nov 7, 2019

angular-automatic-lock-bot bot commented Dec 9, 2019

crisbeto left a comment •

edited

Loading

andrewseguin Nov 6, 2019 •

edited

Loading