Skip to content

fix(material/core): disable strong focus indicators in high contrast mode #24120

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 10, 2022
Merged

fix(material/core): disable strong focus indicators in high contrast mode #24120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/material-experimental/mdc-helpers/_focus-indicators.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
pointer-events: none;
border: $border-width $border-style transparent;
border-radius: $border-radius;

.cdk-high-contrast-active & {
display: none;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this approach, rather than disabling the HC styles, because it’s easier to maintain since we only have one place that needs to be disabled. Our HC focus indicators are pretty prominent so this shouldn’t be a problem for the user.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thumbed up, and then thought a bit more about it. Suppose we remove all of the HCM focus styles and instead just rely upon strong focus indicators? I understand we'll be introducing a HCM regression for apps that aren't opted into strong focus indicators, but if they care about HCM focus indication, they should care about all focus indication. It would allow us to simply remove a bunch of HCM styles across the components. WDYT?

My selfish motivation for wanting this is that in my app, we have a bunch of custom components that today use the strong focus indicators for both HCM and non-HCM focus indication. Once this PR lands, we'll need to instead add HCM styles to all of these components to ensure they have proper focus indication. This seems a little silly... as these components already had good focus indication before this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we'd actually remove that many HCM styles if we went down this route. Most of the current HCM styles are "structural" (e.g. menus without outlines) and there are a handful of cases where we need the extra focus indication. If you're concerned about your existing components, I could add an opt-out.

Alternatively, I can spot fix the few places where the strong focus and high contrast indicators conflict, but that leaves more room for bugs.

Copy link
Collaborator

@zelliott zelliott Dec 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my audit, most if not all components with focusable elements have HCM styles that add outlines. For example, just going down the component list, I think we could remove some styles from:

  • Button
  • Checkbox
  • List
  • Stepper
  • Button toggle
  • Chips
  • Datepicker
  • Slide toggle
  • Tabs

And note that this isn't a complete list, I probably only looked at half of the components and stopped midway through because the ones with focusable elements had HCM focus indication could remove. I think this is a non-trivial amount of styles we'd be able to remove.

To me, it's choosing between (1) removing some HCM styles (not all) from a dozen+ components, or (2) adding a few more lines of styles here to the strong focus indicator mixin, but potentially complicating things for consumers. The only downside for option 1 is that - again - people who aren't opted into the strong focus indicator mixin won't have any HCM focus styles. I'm happy to take a stab at approach 1 so we can see what it looks like - it would be more work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a very rough count of how many lines approximately we would be able to remove across the entire library (not including the MDC components):

  • button - 4 lines
  • button toggle - 20 lines
  • checkbox - 5 lines
  • chips - 3 lines
  • core - 5 lines
  • datepicker - 6 lines
  • expansion - 15 lines
  • form-field - 10 lines
  • list - 3 lines
  • menu - 6 lines
  • radio - 8 lines
  • slide-toggle - 4 lines
  • slider - 5 lines
  • stepper - 3 lines
  • tabs - 5 lines
    Total: 102 lines

I counted the mixin itself in the lines (@include high-contrast), but I excluded comments. This is offset somewhat by the ~32 lines that would be added by the strong focus indicators. I'm not convinced that this is enough of reduction for us to end up in a situation where consumers would have to include an extra mixin just to get focus indicators in HCM. There's also the issue that we wouldn't be able to cover users that want focus indication in HCM, but not the strong indication for non-HCM users. I'm open to exploring other approaches, but I don't think that replacing the HCM focus indication with the strong focus indicators is the right solution either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed analysis, I'll defer to your judgment, although maybe it makes sense for one of Jeremy/Miles/Andrew to also chime in.

... consumers would have to include an extra mixin just to get focus indicators in HCM. There's also the issue that we wouldn't be able to cover users that want focus indication in HCM, but not the strong indication for non-HCM users.

My understanding is that MD is heading in the direction of permanent strong focus indication (happy to discuss this over chat). For that reason, I don't think these will be issues long-term. That being said, maybe that means that this issue should be revisited then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel too strongly either way, but this seems like a reasonable approach so as long as nobody objects I'm going to LGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My inclination would be to have the strong focus indicators supercede the default HCM styles, but I don't want to drop HCM styles by default. I don't feel super strongly, though, so long as we're hitting requirements.

One idea: what would it look like if we refactored existing components to use a common class like .mat-hcm-focus for all default HCM styles, and then turned off those styles in the strong focus indicators mixin?

Copy link
Member Author

@crisbeto crisbeto Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having another selector for HCM focus styles will be a larger refactor, because in a lot of cases the existing HCM selectors reuse the same focus selectors that we have for non-HCM users. We'd have to add extra elements to all the templates and pull out all of the HCM focus styles into new selectors, similarly to what we did for strong focus indicators. It isn't complicated, but it's a lot of churn so we might as well reuse the strong focus elements.

Another option that is similar to what @zelliott was proposing above would be to move all the strong focus styles into the individual components, use them for focus in HCM by default and have the opt-in if consumers want them to be visible for non-HCM users as well. Something like:

.mat-checkbox:focus .focus-indicator {
  border: solid 2px transparent; // transparent is visible in HCM
}

@mixin strong-focus-indicators-color {
  .mat-checkbox .focus-indicator {
    border-color: $primary;
  }
}

This is basically what we have now, except that the strong-focus-indicators mixin will become obsolete.

}
}

// By default, all focus indicators are flush with the bounding box of their
Expand Down
4 changes: 4 additions & 0 deletions src/material/core/focus-indicators/_focus-indicators.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
pointer-events: none;
border: $border-width $border-style transparent;
border-radius: $border-radius;

.cdk-high-contrast-active & {
display: none;
}
}

// By default, all focus indicators are flush with the bounding box of their
Expand Down