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

[core] feat: support windows high contrast mode #5524

Merged
merged 23 commits into from Sep 27, 2022

Conversation

styu
Copy link
Contributor

@styu styu commented Sep 2, 2022

Changes proposed in this pull request:

This adds windows high contrast mode support for most components in Blueprint:

  • Buttons
  • Form controls (radio buttons, switches, checkboxes)
  • Inputs
  • Callouts
  • Cards + elevations
  • HTML tables
  • Menus
  • Popovers
  • Tooltips
  • Navbar
  • Slider
  • Tags

Reviewers should focus on:

For the components affected, do the following show up correctly in windows high contrast mode:

  • disabled states
  • active states
  • hover states
  • default states

Screenshot

image

@blueprint-bot
Copy link

Add border colors for high contrast mode only

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

typo

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

add a couple more

Previews: documentation | landing | table | demo

@adidahiya adidahiya changed the title Add border colors for high contrast mode only [core] feat: add border colors to buttons & controls for high contrast mode Sep 6, 2022
@adidahiya
Copy link
Contributor

Based on this Microsoft Edge blog post, I think it would be more appropriate to use the following media queries:

@media (forced-colors: active) and (prefers-color-scheme: dark) {
  // Windows High Contrast dark theme 
}

@media (forced-colors: active) and (prefers-color-scheme: light) {
  // Windows High Contrast light theme 
}

see https://developer.mozilla.org/en-US/docs/Web/CSS/@media/forced-colors

@styu
Copy link
Contributor Author

styu commented Sep 14, 2022

@adidahiya sounds good, i'm picking this back up today!

@blueprint-bot
Copy link

fix value type of high-contrast-control-border-color

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

More fixes

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix popovers and more controls

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix elevations, tables, inputs

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix popover2, more controls

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

undo dumb find and replace

Previews: documentation | landing | table | demo

@styu styu changed the title [core] feat: add border colors to buttons & controls for high contrast mode [core] feat: add support for high contrast mode for most components Sep 23, 2022
@blueprint-bot
Copy link

fix disabled styles

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix table styles again

Previews: documentation | landing | table | demo

@styu styu marked this pull request as ready for review September 23, 2022 17:14
@styu styu requested a review from adidahiya September 23, 2022 19:06
@adidahiya
Copy link
Contributor

Visual review only (code review coming separately):

This is a huge improvement over the current state of Blueprint in high contrast mode, nice work! I reviewed all the components in the docs preview and found some areas we could still do better.

To address in this PR

Minimal button groups have extra borders

We should make these look the same as regular buttons, without the double border between adjacent buttons:

Screen Shot 2022-09-26 at 4 31 37 PM

Time picker inputs have no borders

Screen Shot 2022-09-26 at 4 40 15 PM

To address in separate PRs

Listed below roughly in order of importance:

Dialogs and Drawers should get a border to separate them from underlying content

(should look similar to popover/tooltips and dark theme dialogs)

Screen Shot 2022-09-26 at 4 38 54 PM

Screen Shot 2022-09-26 at 4 39 15 PM

Active tab indicator is hard/impossible to see

Screen Shot 2022-09-26 at 4 35 45 PM

Progress bar is invisible

Screen Shot 2022-09-26 at 4 34 51 PM

Slider filled track is invisible

Screen Shot 2022-09-26 at 4 37 48 PM

Non ideal state icon stroke has low contrast

we shouldn't apply the opacity adjustment in high contrast mode:

Screen Shot 2022-09-26 at 4 34 15 PM

Skeleton styling has no effect

maybe we should make the text color transparent?

Screen Shot 2022-09-26 at 4 35 18 PM

Tables

Not 100% sure what we should do here, but adding borders between all cells probably makes the most sense. Also a thicker border between row/col headers and the table body

Screen Shot 2022-09-26 at 4 41 34 PM


// Colors used for Windows high contrast mode
// Because high contrast mode doesn't actually obey any colors we define, but uses system colors,
// we define these variables in terms of System colors so that it's easier to understand the intent.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be helpful to link to documentation here, like https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/system_color_keywords

@@ -31,6 +31,12 @@
box-shadow: input-transition-shadow($input-shadow-color-focus, true), $input-box-shadow-focus;
}

@media (forced-colors: active) and (prefers-color-scheme: dark) {
&::before {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add disabled styles as well; we don't want to keep showing a border in this case:

image

// Windows High Contrast dark theme
border: 1px solid $pt-high-contrast-mode-border-color;
// Because we're using a border for the outline, we need to decrease the top margin
margin-top: 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be $switch-indicator-margin - 1px?

@styu
Copy link
Contributor Author

styu commented Sep 26, 2022

I can address dialog + drawer styling in this same PR (I tried to hit the commonly-used components, but for some reason missed those).

I did try to fix the active tab indicator, but that one's a bit tricky since it throws off all the spacing as a border

@blueprint-bot
Copy link

fix compile?

Previews: documentation | landing | table | demo

@adidahiya
Copy link
Contributor

latest changes look good 👍

image

image

image

image

image

@blueprint-bot
Copy link

alph

Previews: documentation | landing | table | demo

@adidahiya
Copy link
Contributor

I filed #5622 for the follow-up issues

@adidahiya adidahiya changed the title [core] feat: add support for high contrast mode for most components [core] feat: support windows high contrast mode Sep 27, 2022
@adidahiya adidahiya merged commit 094e529 into develop Sep 27, 2022
@adidahiya adidahiya deleted the syu/high-contrast-mode branch September 27, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants