Minor style updates #60304

infinnie · 2018-10-09T11:57:40Z

Update to #60288. Sorry for the wrong author name in the initial pull request.

Screenshot:

(Custom color settings below)

{
    "workbench.colorTheme": "Visual Studio Light",
    "workbench.colorCustomizations": {
        "menu.selectionBackground": "#eeeeee",
        "menu.selectionForeground": "#222222",
        "widget.shadow": "#00000033"
    }
}

infinnie · 2018-10-11T10:16:35Z

This also fixes #52854 (comment)

sbatten · 2018-10-12T18:10:14Z

src/vs/base/browser/ui/menu/menu.css

@@ -53,6 +53,14 @@
 	line-height: 1;
 }

+.monaco-menu .monaco-action-bar.vertical .keybinding {
+	opacity: 0.55;


this shouldn't apply in high contrast mode

sbatten · 2018-10-12T18:10:58Z

src/vs/base/browser/ui/menu/menu.css

@@ -124,6 +132,8 @@

 .monaco-menu .monaco-action-bar.vertical .action-item {
 	border: 1px solid transparent; /* prevents jumping behaviour on hover or focus */
+	border-left: 0;


this introduces jumping behavior, please remove

The jumping in High Contrast mode?

Jumping behavior has been fixed.

sbatten · 2018-10-12T18:19:11Z

src/vs/workbench/electron-browser/media/shell.css

@@ -79,7 +79,7 @@
 }

 .monaco-shell .monaco-menu .monaco-action-bar.vertical .action-menu-item {
-	height: 1.8em;
+	height: 2em;


i don't think we should increase the size of the menu items

miguelsolorio

This opacity change doesn't pass the color contrast ratio (4.5:1) in either of our default dark and light themes:

Light	Dark

Please ensure that the opacity change passes the color contrast ratio

infinnie · 2018-10-15T01:36:46Z

@misolori this would not be shipped in high contrast mode.

miguelsolorio · 2018-10-15T16:55:52Z

@infinnie the color contrast ratio has to pass on our default themes (light, dark and high contrast), so it still applies in this instance for light & dark.

infinnie · 2018-10-16T02:44:39Z

@misolori Maybe what is needed is that we should use a darker color for menu text.

miguelsolorio · 2018-10-16T15:29:42Z

@infinnie we use the global foreground color for most of our text, I don't think we'd want to introduce a separate color just for the menu, especially since it's only to use a lighter color shortcut for the keyboard shortcuts.

Since the menu is on a white background, the lightest color you can go with is #757575, so you'd need to get your opacity color to get close to that in order to pass the color contrast ratio (and double-check the dark theme). That would look like this (not sure if that's worth the effort):

usernamehw · 2018-10-16T18:56:38Z

Can't it have ratio less than 4.5:1 but using good contrast (maybe even the same color as text) on hover/focus?

Those tooltips are the secondary info, 99.9% of the time people don't care about them.

miguelsolorio · 2018-10-16T19:32:54Z

@usernamehw regardless of importance, all text must be accessible. The WCAG 2 requirements for contrast state that all text must pass a minimum contrast of 4.5:1

infinnie · 2018-10-17T00:32:56Z

~~Even currently, text such as that in disabled menu items does not pass the test. Maybe we should do something to improve contrast for disabled menu items first.~~

miguelsolorio · 2018-10-17T01:22:52Z

Disabled/Inactive items are part of the exception:

WCAG 2.0 defines four types of "incidental" text that are not required to meet the contrast requirements.

Inactive: An inactive element, like a disabled Submit button, is identified visually by its lower-contrast state.

infinnie · 2018-10-17T01:52:14Z

Even Microsoft Edge does not pass that color contrast ratio check!

infinnie · 2018-10-17T02:01:44Z

Visual Studio Code’s own TypeScript plugin uses an opacity for unused variables too small to pass the color contrast ratio check. Maybe this should be changed first.

Many color themes also do not provide enough color contrast for certain types of tokens like comments.

miguelsolorio · 2018-10-17T02:36:07Z

@infinnie While I would encourage you to file bugs against those items, I'd suggest to keep this conversation on-topic for this PR.

infinnie · 2018-10-17T02:39:20Z

OK, the aforementioned problem has been fixed in my previous commit.

infinnie · 2018-10-17T04:33:59Z

@misolori this is what it looks like now:

miguelsolorio · 2018-10-17T15:03:17Z

src/vs/workbench/common/theme.ts

@@ -122,7 +122,7 @@ export const TAB_ACTIVE_FOREGROUND = registerColor('tab.activeForeground', {

 export const TAB_INACTIVE_FOREGROUND = registerColor('tab.inactiveForeground', {
 	dark: transparent(TAB_ACTIVE_FOREGROUND, 0.5),
-	light: transparent(TAB_ACTIVE_FOREGROUND, 0.5),
+	light: transparent(TAB_ACTIVE_FOREGROUND, 0.7),


Please don't add fixes for issues not related to this PR (menu styling). Thanks!

OK I would edit that in another branch later.

This reverts commit 43bf878.

Sync latest

miguelsolorio · 2018-10-23T18:00:37Z

After discussing this with the team internally, the consensus is we'd be happy to introduce a new theme color for keybinding text that would allow theme authors to style that property. However, we would not update our default themes since the native menus do not use this style:

So if you'd like, you can update your PR to reflect this change or @sbatten can do it on our end.

infinnie · 2018-10-24T01:39:21Z

@misolori I guess a better option is that one could configure the opacity of the keyboard shortcuts instead of the color. This would work better if he or she were to switch from a dark theme to a light one or vice versa.

miguelsolorio · 2018-10-24T05:03:41Z

@infinnie I think by default this inherits the menu foreground color so they wouldn't have to worry about it. Also, you can control the opacity of the color by utilizing the 8 digit hex color (#RRGGBBAA). So ~~#FF000010~~ #FF00001A would render #FF0000 at 10% opacity.

usernamehw · 2018-10-24T05:34:40Z

Isn't 10% - 1A in hex?

miguelsolorio · 2018-10-24T05:38:11Z

@infinnie sorry, you're right.

infinnie · 2018-10-24T06:32:01Z

@misolori you mentioned the wrong person.

sbatten · 2018-10-29T23:03:36Z

@infinnie Thanks for the PR. I have made some updates as I wanted to merge the PR this iteration and you have a good fix for #61564. If you want to revisit some of the other changes that I removed, feel free to open another PR.

outdated

Minor style updates: fix author

bb2dc66

infinnie mentioned this pull request Oct 9, 2018

Minor style updates #60288

Closed

egamma assigned sbatten Oct 9, 2018

infinnie added 3 commits October 10, 2018 09:36

Set hover opacity to 1

b3d0d77

fix: submenu opacity

f60bcc0

fix typo

f3706df

vikaskyadav approved these changes Oct 11, 2018

View reviewed changes

sbatten reviewed Oct 12, 2018

View reviewed changes

miguelsolorio suggested changes Oct 12, 2018

View reviewed changes

infinnie added 2 commits October 15, 2018 10:04

High contrast updates

da49754

CSS rule order

1db6d36

Color contrast improvements

92330ea

Tab color update (microsoft#61106)

43bf878

miguelsolorio previously requested changes Oct 17, 2018

View reviewed changes

infinnie added 2 commits October 18, 2018 09:16

Revert "Tab color update (microsoft#61106)"

423902f

This reverts commit 43bf878.

Merge pull request #1 from Microsoft/master

e58e626

Sync latest

miguelsolorio self-assigned this Oct 22, 2018

infinnie and others added 3 commits October 24, 2018 19:15

OK, remove keybinding opacity.

415329e

updating because I want to get your PR in

488a5be

one more

df40e61

sbatten merged commit a14b154 into microsoft:master Oct 29, 2018

sbatten added this to the October 2018 milestone Oct 29, 2018

sbatten added the workbench-menu label Oct 29, 2018

sbatten mentioned this pull request Oct 29, 2018

Windows: submenu indicator rendered very large #61564

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor style updates #60304

Minor style updates #60304

infinnie commented Oct 9, 2018 •

edited

Loading

infinnie commented Oct 11, 2018 •

edited

Loading

sbatten Oct 12, 2018

sbatten Oct 12, 2018

infinnie Oct 15, 2018

infinnie Oct 15, 2018

sbatten Oct 12, 2018

miguelsolorio left a comment •

edited

Loading

infinnie commented Oct 15, 2018

miguelsolorio commented Oct 15, 2018 •

edited

Loading

infinnie commented Oct 16, 2018

miguelsolorio commented Oct 16, 2018

usernamehw commented Oct 16, 2018

miguelsolorio commented Oct 16, 2018

infinnie commented Oct 17, 2018 •

edited

Loading

miguelsolorio commented Oct 17, 2018

infinnie commented Oct 17, 2018 •

edited

Loading

infinnie commented Oct 17, 2018 •

edited

Loading

miguelsolorio commented Oct 17, 2018 •

edited

Loading

infinnie commented Oct 17, 2018 •

edited

Loading

infinnie commented Oct 17, 2018

miguelsolorio Oct 17, 2018

infinnie Oct 18, 2018

miguelsolorio commented Oct 23, 2018

infinnie commented Oct 24, 2018

miguelsolorio commented Oct 24, 2018 •

edited

Loading

usernamehw commented Oct 24, 2018

miguelsolorio commented Oct 24, 2018

infinnie commented Oct 24, 2018

sbatten commented Oct 29, 2018

Minor style updates #60304

Minor style updates #60304

Conversation

infinnie commented Oct 9, 2018 • edited Loading

infinnie commented Oct 11, 2018 • edited Loading

sbatten Oct 12, 2018

Choose a reason for hiding this comment

sbatten Oct 12, 2018

Choose a reason for hiding this comment

infinnie Oct 15, 2018

Choose a reason for hiding this comment

infinnie Oct 15, 2018

Choose a reason for hiding this comment

sbatten Oct 12, 2018

Choose a reason for hiding this comment

miguelsolorio left a comment • edited Loading

Choose a reason for hiding this comment

infinnie commented Oct 15, 2018

miguelsolorio commented Oct 15, 2018 • edited Loading

infinnie commented Oct 16, 2018

miguelsolorio commented Oct 16, 2018

usernamehw commented Oct 16, 2018

miguelsolorio commented Oct 16, 2018

infinnie commented Oct 17, 2018 • edited Loading

miguelsolorio commented Oct 17, 2018

infinnie commented Oct 17, 2018 • edited Loading

infinnie commented Oct 17, 2018 • edited Loading

miguelsolorio commented Oct 17, 2018 • edited Loading

infinnie commented Oct 17, 2018 • edited Loading

infinnie commented Oct 17, 2018

miguelsolorio Oct 17, 2018

Choose a reason for hiding this comment

infinnie Oct 18, 2018

Choose a reason for hiding this comment

miguelsolorio commented Oct 23, 2018

infinnie commented Oct 24, 2018

miguelsolorio commented Oct 24, 2018 • edited Loading

usernamehw commented Oct 24, 2018

miguelsolorio commented Oct 24, 2018

infinnie commented Oct 24, 2018

sbatten commented Oct 29, 2018

infinnie commented Oct 9, 2018 •

edited

Loading

infinnie commented Oct 11, 2018 •

edited

Loading

miguelsolorio left a comment •

edited

Loading

miguelsolorio commented Oct 15, 2018 •

edited

Loading

infinnie commented Oct 17, 2018 •

edited

Loading

infinnie commented Oct 17, 2018 •

edited

Loading

infinnie commented Oct 17, 2018 •

edited

Loading

miguelsolorio commented Oct 17, 2018 •

edited

Loading

infinnie commented Oct 17, 2018 •

edited

Loading

miguelsolorio commented Oct 24, 2018 •

edited

Loading