Monokai: Use new dropdown.listBackground to correct contrast Fixes: #42480

[raygervais]

Fixes #42480, the drop down widget now accommodates the Monokai theme on Windows and Linux platforms better for currently active list item!

Currently using color #75715E for active item, which looks like this:

If you have a preferred color from the theme, I'd be happy to switch out the current 👍

[cleidigh]

@raygervais
@bpasero

@

I think we have a bit of a sticky issue here. If we change
list.focusBackground to fix the fact that the original Monokai
had the same color for dropdown.background we obviously change
the list color everywhere. The list selection color (also focus)
is one of the few colors specifically called out in the theme
as an original color. Do we really want to change that?

[from theme file]

// This theme's colors are based on the original Monokai:
// #1e1f1c (tab well, borders)
// #272822 (editor background)
// #414339 (selection)
// #75715e (focus)
// #f8f8f2 (editor foreground)
{
"type": "dark",
"colors": {
"dropdown.background": "#414339",
"list.activeSelectionBackground": "#75715E",
"list.focusBackground": "#414339",

When I was working on Monokai a while ago I was told it was very specific
and needed to match originals much as possible.

The problem is we have several very different drop-down background colors.
Debug, Explorer, QuickPick currently look okay as they override the background.
So we need to improve the actionbar based drop downs ideally without changing the others.

We could do this with a style override on the select creation but this would affect
all themes. Other similar approaches really don't solve the problem either.

Other than changing the color for all lists, the really ugly way
would be to force a contrast change if the dropdown.background === list.focusBackground

[raygervais]

I see the issue that you're addressing, and do agree that the ugly way is really really hacky.

Do you have any suggestions / thoughts towards this issue?

If we must follow the original spec of Monokai, in my opinion we are omitting accessibility concern unless the actionbar based dropdowns are updated to follow the same idea as Explorer, QuickPick, etc. This seems to be a bigger issue than just a single line change or style update. I wonder if custom Monokai theme extensions follow suit for dropdowns or omit, simply just curious.

Do you think that updating the actionbar dropdowns is out of the question?

[cleidigh]

@raygervais
Probably the first thing to know is what the team does and does not want to change for this very popular
theme. Something has to change, it obviously can't stay the way it is.

Just changing the action bar clients still as the issues mentioned above.

What you have to realize is the new drop downs I contributed inherited the existing theme list colors.
Prior there were no clashes because the old native select could not use all colors, one of the main reasons for the addition. Any custom theme where the drop-down background color === one
of the list colors for focus or hover would actually inherit this issue as well.

[raygervais]

@cleidigh, my bad. I understand the gravity of this issue now much clearer.
Let's keep brainstorming and see what ways we can address this issue? I'm partially at a loss of what is and isn't possible within the scope of what needs to change, and what the team would like as the end result. Still, here to help in anyway I can with this issue 😄

[cleidigh]

@bpasero
I'm starting to think a minimally problematic solution would be to add a dropdown.expandedBackground
color that could be used if a different background color is desired or needed for the expanded dropdown different from the select header. The result would keep all the list colors constant
and have a similar effect to the quick open look.

[bpasero]

@cleidigh that makes sense to me, maybe it should be called dropdown.listBackground instead?

[raygervais]

I like the concept, and am still onboard if we go that direction @bpasero @cleidigh.
Likewise, this will make it possible for extension themes (if they so chose) to hook into that specific portion of the API too for better accessibly of their own theme, correct?

[raygervais]

Quick question if we do proceed this direction, where do I create a new 'type' so that I can create listBackground for the dropdown object? Furthermore, what we'd need to update is the following function to see if listBackground exists correct? https://github.com/Microsoft/vscode/blob/9a5b6ebc08d530463203c75a92e0ea49b6becc73/src/vs/base/browser/ui/selectBox/selectBoxCustom.ts#L52

[cleidigh]

@raygervais
Hope you don't mind but I am actually going to post this PR as it should really get out in the recovery build. I'm doing it as a separate PR since it's an actual addition. We can then change your PR with the actual color.

If you are online tonight check for my post.

[raygervais]

Not a problem @cleidigh, will update my PR with what is needed tonight.
Thanks for the update 👍

[cleidigh]

@raygervais
Cool. Just so you understand even adding a single color touches several places. You will see and learn a lot once you see it.

Once I do the addition you will have to rebase or update your branch to pick up my changes so you can actually add the color to the theme.

[cleidigh]

@raygervais
more problematic than I expected, stay tuned

[bpasero]

@cleidigh I think we should plan this for February, not January recovery build

[cleidigh]

@bpasero
Yes @Tyriar just let me know that's for higher priority things.
I guess in the meantime the only user fix as if people want to change one of the listed colors manually.

I have the color add PR ready. will use the other issue PR to actually change the theme color

[cleidigh]

@raygervais
okay if you update your branch you can modify your PR to add the new color to the theme.

Using an existing color that matches the quick open I suggest below, This might not end up in the final color if we want more contrast or others have a different opinion. At at least it solves the problem for now.

"dropdown.listBackground": "#1e1f1c",

[raygervais]

Will be done tonight @cleidigh, thanks for the guidance

[raygervais]

I'm pretty sure that git rebase hates me... I'm assuming that this is not the desired output correct?
Can I try a force push on a hard reset back to the original commit?

[cleidigh]

@raygervais
I'm pretty sure git rebase hates everyone ;-)
The easiest thing to do is just create an up-to-date branch and repost a new PR. Especially since it's one file and one line.

I have found that most of the time if I have to do an update I manually reemerge my file changes.
Otherwise I've ended up with the same issue sucking in all the other commits that obviously we don't want.

[raygervais]

@cleidigh, correcting now.
Good advice about remerging only my own commits into updated branch. Will try in close future :)
Ended up fixing this branch to preserve original PR 👍

[cleidigh]

@raygervais
Thanks for your first PR congrats.
FYI notice that the title change this is typically the way things like to be labeled, especially placing the
"Fixes:" issue number at the end of the title

[raygervais]

Thanks @cleidigh, you've been a great mentor for working my way around Code and the new drop down component. I hope to keep contributing where possible 👍

[cleidigh]

@raygervais
Absolutely glad to help, we can probably finish up the other PR soon. I'm hoping to work on it tomorrow.
As you saw even a small contribution tends to have tentacles in a variety of places. I've been working on code for a year now and really just touched the surface and spent quite a bit of time perusing and learning. I be happy to share whatever I can or answer questions if I can.