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

feat: add high contrast implementation #28956

Merged
merged 27 commits into from
Feb 6, 2024
Merged

feat: add high contrast implementation #28956

merged 27 commits into from
Feb 6, 2024

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Feb 1, 2024

Issue number: Internal


What is the current behavior?

Users do not have a way of increasing the contrast in Ionic apps. This is valuable for people with low vision as increasing the contrast between foreground and background content helps improve readability.

What is the new behavior?

  • Adds a high contrast light and high contrast dark theme. As with our other themes, developers can choose between system, class, and always stylesheets.

While we aim to improve contrast for text and UI components, this feature prioritizes text in the event that both text and UI component cannot be improved without one negatively impacting the other.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 7.6.7-dev.11706894781.1cd59fde

Testing instructions:

  1. Open src/themes/test/css-variables. Activate the high contrast light and dark themes to verify that contrast does increase.
  2. Use the dev build to integrate the theme into a test app (conference app, starter app, etc).

I'd recommend using these imports:

@import "@ionic/angular/css/themes/high-contrast.system.css";
@import "@ionic/angular/css/themes/high-contrast-dark.system.css";

Note: Make sure this is imported after core.scss

@github-actions github-actions bot added the package: core @ionic/core package label Feb 1, 2024
@@ -12,4 +12,4 @@ $breadcrumb-baseline-font-size: 16px !default;
$breadcrumb-font-size: dynamic-font($breadcrumb-baseline-font-size) !default;

/// @prop - Color of the breadcrumb separator
$breadcrumb-separator-color: var(--ion-color-step-550, var(--icon-text-color-step-450, #73849a)) !default;
$breadcrumb-separator-color: var(--ion-color-step-550, var(--ion-text-color-step-450, #73849a)) !default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a typo

@@ -25,7 +25,7 @@ $item-ios-paragraph-margin-start: $item-ios-paragraph-margin-end !def
$item-ios-paragraph-font-size: dynamic-font(14px) !default;

/// @prop - Color of the item paragraph
$item-ios-paragraph-text-color: rgba($text-color-rgb, .4) !default;
$item-ios-paragraph-text-color: var(--ion-text-color-step-550, #a3a3a3) !default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed to ship this feature, so let me know if you'd prefer I add this in a separate PR.

Devs can add a p inside of an item. The problem is that p won't participate in high contrast because it's using the --ion-text-color-rgb var with an alpha channel instead of a step color. This change allows it to participate in high contrast. The values I chose are very close to the original computed values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this change makes sense as part of the core changes here. It is a common enough pattern that developers can find themselves in. Having it organized under the same commit as high contrast works for me.


@mixin high-contrast-light-md-theme() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Toast (for both light and dark high contrast themes) was implemented incorrectly because it was scoped to all toasts when it should only have been scoped to MD toasts. This resulted in screenshot diffs.

Comment on lines +141 to +144
& ion-toast {
--color: var(--ion-background-color);
}

Copy link
Contributor Author

@liamdebeasi liamdebeasi Feb 2, 2024

Choose a reason for hiding this comment

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

One thing I missed during the design phase of this toast workaround is that the text stepped colors would also be changing. I built this solution before I worked on the new text stepped colors architecture.

The problem is that --color by default uses a stepped color. But since those stepped colors are changing in high contrast, the way they mix using color-mix also changes. This resulted in text not having sufficient contrast. As a result, all text in high contrast mode will use the background color of the page.

Note that this is not a typo. The MD toast background is a dark gray in light mode, so using the page background color (white) will ensure correct contrast. Similarly, the background is white in dark mode (where the page background is dark gray).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to make the same adjustments in the high contrast dark theme too

@@ -103,7 +150,7 @@ $colors: (
/* stylelint-disable-next-line order/order */
@supports (color: color-mix(in lch, plum, pink)) {
& ion-toast::part(button) {
color: color-mix(in srgb, var(--color) 60%, var(--button-color));
color: color-mix(in srgb, var(--color) 70%, var(--button-color));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to tweak the percentage value here since I was also changing the text color used.

@liamdebeasi liamdebeasi changed the title Fw 5863 feat: add high contrast implementation Feb 2, 2024
@liamdebeasi liamdebeasi marked this pull request as ready for review February 2, 2024 17:30
@liamdebeasi liamdebeasi requested review from brandyscarney and a team as code owners February 2, 2024 17:30
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Feature looks great to me with my testing! I noted the gotcha I ran into with the import order on the stylesheets in our internal thread. We'll have that accounted for in the documentation and starter templates.

@@ -25,7 +25,7 @@ $item-ios-paragraph-margin-start: $item-ios-paragraph-margin-end !def
$item-ios-paragraph-font-size: dynamic-font(14px) !default;

/// @prop - Color of the item paragraph
$item-ios-paragraph-text-color: rgba($text-color-rgb, .4) !default;
$item-ios-paragraph-text-color: var(--ion-text-color-step-550, #a3a3a3) !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this change makes sense as part of the core changes here. It is a common enough pattern that developers can find themselves in. Having it organized under the same commit as high contrast works for me.

Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

Looks great!

@liamdebeasi liamdebeasi merged commit c147895 into FW-5747 Feb 6, 2024
44 checks passed
@liamdebeasi liamdebeasi deleted the FW-5863 branch February 6, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants