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

Add high contrast light theme kind #144759

Merged
merged 32 commits into from
Mar 16, 2022
Merged

Add high contrast light theme kind #144759

merged 32 commits into from
Mar 16, 2022

Conversation

aeschli
Copy link
Contributor

@aeschli aeschli commented Mar 9, 2022

Fixes #144193

  • Adds ColorScheme.HIGH_CONTRAST_LIGHT (value hcLight), renamed ColorScheme.HIGH_CONTRAST (old value hc), to ColorScheme.HIGH_CONTRAST_DARK (new value hcDark)
  • Adds ColorScheme.isHighContrast to test if the current theme is a high contrast theme
  • color registrations default colors now take hcDark and hcLight.
    • hcLight is currently optional and uses either hcDark if it is null or color id else light if it is a color value
    • @daviddossett Fill out the defaults for hcLight in all the registrations (let me know if I can help)
  • @daviddossett Add css rules for hc-light
    • the css class that is active for theme of type hcLight is hc-light (For hcDark it is hc-black, as before
  • @daviddossett provide letterpress-hcLight.svg (currently just a copy of letterpress-hcDark.svg)
  • @aeschli new setting preferredHighContrastLightColorTheme
  • @aeschli color contribution point: new optional property defaults.highContrastLight
    • defaults to value of light
    • highContrast is not renamed to not break any existing users
    "colors": [
      {
        "id": "gitDecoration.addedResourceForeground",
        "description": "%colors.added%",
        "defaults": {
          "light": "#587c0c",
          "dark": "#81b88b",
          "highContrast": "#1b5225"
          "highContrastLight": "#587c0c"
        }
      }
    ]

@bpasero
Copy link
Member

bpasero commented Mar 9, 2022

@aeschli as for breaking web API, I suggest to give John Keech a ping either in an issue or via chat, in the end Codespaces is the only external consumer of our API so far. Not sure they would actually leverage any theming related embedder API we have, other than setting the default theme.

@bpasero
Copy link
Member

bpasero commented Mar 9, 2022

In places like these:

export const ACTIVITY_BAR_BORDER = registerColor('activityBar.border', {
	dark: null,
	light: null,
	hcDark: contrastBorder
}, localize('activityBarBorder', "Activity bar border color separating to the side bar. The activity bar is showing on the far left or right and allows to switch between views of the side bar."));

Shouldn't we also set a default for hcLight or is the idea that the theme itself defines this? Here for example, we do not set an explicit border around the activity bar unless a HC theme is picked and I wonder if this applies equally to light and dark HC themes.

@aeschli
Copy link
Contributor Author

aeschli commented Mar 9, 2022

Shouldn't we also set a default for hcLight or is the idea that the theme itself defines this? Here for example, we do not set an explicit border around the activity bar unless a HC theme is picked and I wonder if this applies equally to light and dark HC themes.

Yes, this is the task Fill out the defaults for hcLight in all the registrations (let me know if I can help)

@aeschli aeschli added this to the March 2022 milestone Mar 10, 2022
@daviddossett
Copy link
Contributor

Awesome, thanks so much for getting this going! Working on my items today/early next week.

@daviddossett
Copy link
Contributor

daviddossett commented Mar 11, 2022

Seeing this warning after adding values for highContrastLight. Any ideas what I'm doing wrong?

color contribution gitDecoration.addedResourceForeground does not provide color 'defaults.highContrastLight'. Using color for `light` instead (#587c0c).

"colors": [
{
"id": "gitDecoration.addedResourceForeground",
"description": "%colors.added%",
"defaults": {
"light": "#587c0c",
"dark": "#81b88b",
"highContrast": "#1b5225",
"highContrastLight": "#374e06"
}
},

@aeschli
Copy link
Contributor Author

aeschli commented Mar 11, 2022

Seeing this warning after adding values for highContrastLight. Any ideas what I'm doing wrong?

color contribution gitDecoration.addedResourceForeground does not provide color 'defaults.highContrastLight'. Using color for `light` instead (#587c0c).

Pushed a fix

@daviddossett
Copy link
Contributor

Finished my tasks. Let me know if anything looks off 🙏

I'll update the letterpress assets in distro as well once this is in.

@@ -36,10 +36,12 @@
animation: monaco-findInput-highlight-1 100ms linear 0s;
}
.hc-black .monaco-findInput.highlight-0 .controls,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there are still hard coded colors in findInput.css that we should externalize.
hcLight uses the same color as hcDark, is that intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed so that hc-light uses the light colors/animations. Thansk for catching that.

As for the hard coded bits, are you referencing the disabled background rules and the animation backgrounds? I'd need to go decide on specific colors for each theme if all of them have been using the same values all this time. Might make sense to do this on a separate issue since it goes beyond HC.

@@ -23,7 +23,7 @@ export function getIconClass(iconPath: { dark: URI; light?: URI } | undefined):
} else {
iconClass = iconClassGenerator.nextId();
dom.createCSSRule(`.${iconClass}`, `background-image: ${dom.asCSSUrl(iconPath.light || iconPath.dark)}`);
dom.createCSSRule(`.vs-dark .${iconClass}, .hc-black .${iconClass}`, `background-image: ${dom.asCSSUrl(iconPath.dark)}`);
dom.createCSSRule(`.vs-dark .${iconClass}, .hc-black .${iconClass}, .hc-light .${iconClass}`, `background-image: ${dom.asCSSUrl(iconPath.dark)}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't hc-light use iconPath.light(if available)

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, fixed.

@aeschli
Copy link
Contributor Author

aeschli commented Mar 15, 2022

Thanks a lot @daviddossett !
There are still some color registrations without hcLight. To see these, just remove the type ColorDefaultsDeprecated in https://github.com/microsoft/vscode/blob/aeschli/hcLightThemeKind/src/vs/platform/theme/common/colorRegistry.ts#L63

@daviddossett
Copy link
Contributor

Added mising registrations. Thanks for the tip on removing the type 👍

@aeschli
Copy link
Contributor Author

aeschli commented Mar 16, 2022

Merging the PR so we get more testing.

@aeschli aeschli merged commit 02e8bd1 into main Mar 16, 2022
@aeschli aeschli deleted the aeschli/hcLightThemeKind branch March 16, 2022 15:41
@daviddossett
Copy link
Contributor

@daviddossett
Copy link
Contributor

daviddossett commented Mar 17, 2022

FYI I updated the letterpress assets in distro to match the new naming scheme e.g. letterpress-hcLight.svg. I noticed that Insiders was falling back to the OSS letterpress on all but dark theme types.

Fixed missing or wrong class additions in 74157b3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dark and light high contrast theme types
3 participants