Open oliversturm opened 7 years ago
aeschli
commented
7 years ago It was only supported for high contrast themes. I changed it to work for any theme.
Note that when you set the editor foreground color there won't be any token colors inside the selected range. @alexandrudima FYI, speak up if you see a problem.
alexdima
commented
7 years ago I've had to revert the commit because it causes #36760
@aeschli Please create PR for changes in this area in the future.
Stanzilla
commented
6 years ago @aeschli any plans to revisit this? it makes stuff very hard to read at times.
mbenkmann
commented
6 years ago Is there an issue for the underlying problem in the renderer that causes times to go up when foreground color is set? It's certainly a bug/flaw in the renderer that the respective developers should look at. There is no logical reason why changing the foreground color should cause repaint time to go up when changing the background color does not. The area that needs to be repainted is exactly the same. In fact because the foreground color makes the syntax coloring go away it should in theory be possible to make rendering faster for selected text with both foreground and background color.
Besides, shouldn't the user decide if the drop in performance bothers him? After all, no one is saying that any of the default themes shipped with VS Code should use the option. This issue would only arise for a user who decides to install an extra theme.
mbenkmann
commented
6 years ago Maybe the respective developers could also look into whether it would be possible to implement a color inversion option for selected text with better performance. I'm thinking of 3 possible ways:
swap colors around in the frame buffer (e.g. R => G => B => R)
It's not really important that the foreground color in particular be implemented. We just need some way to make it more readable. At least one of the above options should be implementable without performance drop.
jdputsch
commented
6 years ago I would very much expect the behavior described in the reverted fix:
Note that when you set the editor foreground color there won't be any token colors inside the selected range.
It is unfortunate this feature seems to not be compatible with "low" contrast themes.
raptor235
commented
6 years ago bump pls
chibicode
commented
6 years ago 
oliversturm
commented
6 years ago This issue turned a year old a few days ago. Here's a bump - still no good presentation solution with VS Code!
kekoafleming
commented
5 years ago Bump for revisit to this issue.
jmvdigital
commented
5 years ago Bump again for revisit.
ddejohn
commented
5 years ago Having this issue in 1.36.0.
equinusocio
commented
5 years ago Bump. Syntax highlighters use many colours, is very hard to set a selection background color that allow an acceptable contrast with the selected text. I can suggest to remove all the token colors for selected text in order to inherit the color from another token, like the editor.foreground or something else.
oliversturm
commented
5 years ago @aeschli This issue will be two years old soon and I'm reminded every time I present using VS Code. Can we ever expect something to be done about this? It really doesn't look like a satisfactory solution would be too complicated...
moluoyu
commented
5 years ago Having this issue in Version: 1.39.2 (user setup) Commit: 6ab598523be7a800d7f3eb4d92d7ab9a66069390 Date: 2019-10-15T15:35:18.241Z Electron: 4.2.10 Chrome: 69.0.3497.128 Node.js: 10.11.0 V8: 6.9.427.31-electron.0 OS: Windows_NT x64 6.1.7601
rediffusion
commented
4 years ago How you use this cuz I can't get it? o_O
editor.selectionForeground
I've tried this in settings.json, but with no success❗
"workbench.colorCustomizations": { editor.selectionForeground }
FrancisVila
commented
4 years ago I think a good way of pointing out the current selection is to use a border. That would bypass all the issues about conflicts with syntax coloring. Strangely, you can set a border on the selectionhighlight (texts that are identical to the selection), but not the selection itself, arguably the most important surface of real estate in the whole application - where things will actually happen when you type on the keyboard. This does not work in the current version, however (18 feb 2020). I opened an issue to that effect: https://github.com/microsoft/vscode/issues/90807
jamesowens91
commented
4 years ago Bump for revisit.
evctn
commented
4 years ago Still doesn't work ver 1.47
ssoher
commented
3 years ago Still doesn't work 1.52.1
kerim371
commented
3 years ago Still doesn't work 1.53.2
jvcmarcenes
commented
3 years ago Still doesn't work 1.54.1
unlocomqx
commented
3 years ago I just encountered this issue
oliversturm
commented
3 years ago <sarcasm> Ah come on, give it another few years. You have to understand that there are way more important things to do than to make VS Code provide solid editor features, look good in presentations and attract users. </sarcasm>
Maybe one of these days I should get the source code and see if I can't fix it myself. But the impression I got at the start of the thread was that even devs closely affiliated with the project didn't know the right way to do this - way to invite 3rd party contributions.
Meanwhile, here's hoping that the integration work on the millionth extra API will be done at some point, so that there'll be time again to work on issues with basic editor functionality... can't help but notice that Emacs handles this same highlighting scenario gracefully, even on the console.
efibutov
commented
3 years ago Would really appreciate if the feature would be implemented. It is really disappointing there is no such a feature.
tvivt
commented
3 years ago 1.57.1 Still doesn't Work
efibutov
commented
3 years ago Looks like Pycharm is not going to give up... At least for me
JustDre
commented
3 years ago The fundamental problem is that the DOM container called div.view-overlays is, despite its name, laid under (logically before) the container div.view-lines. It consists of lines and boxes that are used to subtly highlight text related to the current selection (for example) but only the background and not the actual text. Were it an actual overlay, the text from the .view-lines layer would have to be included in the .view-overlays layer so that text could be "rendered" in a fully transparent color to allow the actual text color to show through from below.
So the fix would be to prioritize text selection and add only the selected text to the overlay layer, and swap the order of the layers. This sounds simple in theory, but when a user selects all (or, at least, everything visible on the screen) then this results in a slight memory and noticeable performance hit. I believe there was a previous commit that solved our problem, but was reverted because perf testing showed a huge regression.
Note that this problem only occurs for dark themes that don't play well with the "additive" nature of the overlays design. IMO the perf hit of the previous solution should be opt-in for those who choose a dark theme. To avoid the perf impact, an idea that I explored was using CSS "mix-blend-mode" and its different settings to increase contrast between a transparent overlay and any text that's below it. One side effect among many is that this technique changes the text color for all overlays. Plus, this would not be appropriate for light themes.
eykamp
commented
3 years ago Great explanation! It sounds as if you have one performant solution that works for light themes, and a different one for dark themes. Maybe there's a way to select one based on the theme characteristics and solve the problem that way.
pencil-user
commented
3 years ago bump
rctlmk
commented
2 years ago Happy New Year guys.
jvcmarcenes
commented
2 years ago This feature can be opt-in, for users that don't mind the performance hit, or that won't be hitting ctrl+a on a random file with over 30,000 lines (which is a very odd edge-case).
But this feature is clearly supported in different editors, and it's needed not just because of looks, but for accessibility and presentation.
aaronransley
commented
2 years ago Bad look for accessibility aspects of VSCode, not sure why this hasn't been tackled?
TonyGravagno
commented
2 years ago It's time for a different approach to this challenge.
This issue was assigned to @aeschli three years ago. It got his initial attention, but after a welcome but unfortunately failed attempt at a solution, he apparently put this very low onto his personal priority list. Now it sits, assigned to someone who may never work on it.
OK, we get how this works. It's FOSS. We're grateful for the time anyone spends on it. But FOSS or not, issues that are not processed to completion by an assigned technician, within some "reasonable" period of time, need to be reassigned to someone else. We need to move forward.
In this case I respectfully request that this issue be reassigned, or set as unassigned - so that we can get someone on the issue who has time, skill, and interest to solve this constant UI irritation. OR, perhaps @aeschli can reprioritize this. OR, perhaps he can reassign some other tasks to others with competence, so that he can focus his unique abilities on this challenge.
Again, this is with respect and gratitude to @aeschli for his time on this issue, on all VSCode issues, and on all FOSS to which he contributes.
JustDre
commented
2 years ago OK, we get how this works. It's FOSS. We're grateful for the time anyone spends on it.
You're assuming this problem can be solved by a single person. I respectfully disagree, as there were architectural decisions made, probably for performance reasons and that constrains the possible solutions. We've been painted into a corner, and there is no solution without causing someone else to be unhappy, possibly very much.
TonyGravagno
commented
2 years ago I thank @aeschli for unassignment. That at least moves this ticket into a new category to get some eyes on it. If you have not yet given a thumbs up to the OP, please do to ensure there is no bot intervention, auto-close, or other triage process that closes this ticket on the basis of lack of community support. I rarely get behind an issue but this one deserves to be resolved.
Good suggestions for alternative resolutions are on the table, including:
With appropriate scheduling for attention by qualified personnel at Microsoft and elsewhere, can we get an evaluation of the merits and concerns of each of those options? If any of those solutions seem viable then maybe we can get the ticket re-assigned and backlogged for a new milestone. Thanks.
TonyGravagno
commented
2 years ago Here's another related suggestion based on the above, which might be possible with an extension, though I rather doubt it given explanations so far :
I have no idea if that solution will result in the same performance hit as the original solution. I don't think it will because the theming is not done for every inline element. It's a single final override that will more likely result in minor flashing on hardware/configurations that are inclined to be slow anyway. ... and I'm sorry but I don't know if that even fits the VSCode model since I'm not a core contributor.
The select/save of "I can't read this" values might be possible with a third-party extension. Or it can be done manually. I'm not aware of an extension that tells us the styling that's applied to a single character in an edit item, or that has the ability to accurately tell us the RGB as it's been assigned in the UI.
That means the solution for this foreground color issue doesn't need to include a UI for a new User settings field - it just needs to read from a structure that will be created elsewhere, even manually.
As to "how do we know what to override?" From what I see the div tags have a class. Might this be as simple as understanding the pattern to the class names and then theming those elements? Maybe we don't need a code change at all - I have no idea if user-defined theming can be applied to an edit page after all other styling has been applied ... but that itself seems like a dirt-simple and rather obvious feature, no?
(Sorry if this note is convoluted. It took me a while to get all the ideas together.)
ykrx
commented
2 years ago Any updates on this? Why does the editor.selectionForeground setting exist at all if it does nothing?
surrealsymmetry
commented
2 years ago Would love to see this implemented properly.
Bright highlighting in dark mode was the first thing I tried to customize upon using the program.
I would expect it to override token colors. I just want black text on a color that pops, like I can already do with the block cursor.
mustafaabobakr
commented
2 years ago The dark theme needs a bright highlight background which requires a color that goes well with this new bright background
ykrx
commented
2 years ago Still no updates about this...
As others have pointed out, very strange how something this blatant of an accessibility issue has been in limbo for almost FIVE years. I'd argue this isn't as "low priority" as it was originally brushed off to be; the fact that this thread is still active since 2017 obviously dismisses that notion. Competing editors support this and although VS Code is otherwise the most customizable one by far, "elephant-in-the-room"-ing this issue for this long is just bizarre. If the implementation of full-blown selection foreground colors is this architecturally-nuanced for public contributors to write, then some naive/temporary fix shouldn't be difficult to come up with — even if that consists of literally just having a black/white option setting for selected text — to at least improve readability of selected text (or automatic black/white foreground depending on contrast ratio with selection background).
I'm not aware of the details about how extensions like Color Highlight work, but they do, so this is clearly possible:
Otherwise, I think it's time to remove the editor.selectionForeground setting altogether until it actually serves a purpose; it's only attracting new attention to this issue that is clearly not being addressed.
TonyGravagno
commented
1 year ago Let's focus on process:
It's fine that @aeschli will not be processing this. But who will? When this has been tagged as above, please assign people who can at least triage this issue and get concensus on what needs to be done. If specific people are not going to work on this, remove them from assignment. But let's see some movement. @alexdima @sandy081 : Can you manage the assignments?
When this issue is being discussed, please link to a Slack discussion. Developers, create a thread when you go through this and link to it so that others can benefit/comment/contend.
If this wasn't such a daily annoyance I wouldn't be so vocal here. It's been 5 years. C'mon. It's time to bump the priority, get some eyes on it, decide what needs to be done, and then try to allocate resources to address it. I know it's FOSS. People at Microsoft have insight that might help someone in the community to work out a PR to help. Write the functionality out as too tough to handle (and deal with those consequences) or take another shot at this. But lets' not let it languish like this for more years to come.
Thanks!
alexdima
commented
1 year ago The problem here is that we can't enable this by default because it has a high performance cost and impacts editor frame rendering times. See https://github.com/microsoft/vscode/issues/36760 which shows an extreme example where a frame now takes 55ms instead of 4ms to render. That means 18 FPS if everything goes well on the browser side (most likely 15FPS) instead of 60 FPS. The root cause is that the editor renders multiple layers, the text and the selection are on different layers. When the selection changes, we manage to only re-render the selection layer and keep the text layer untouched. The text layer is the most expensive one, as it involves Chromium doing text layouting. But in order to change the selected text foreground (which this issue asks for), we need to re-render also the text layer. This is reasonable if someone really really needs this, like in the case of the high contrast themes, but this can't be default, as it will slow down VS Code tremendously for everyone. A potential solution could look into restoring some of the changes in https://github.com/microsoft/vscode/commit/3a3f0e41968ddd3cef6c1e4a090ca8ef8bc6b049, but combined with an editor setting such that opt in is explicit via an editor setting where we can explain that slowness will occur. A solution where simply installing a theme results in immediate poor performance would not be sufficient. PR welcome.
TonyGravagno
commented
1 year ago OK, @alexdima - thank you sincerely for the clear explanation. Based on the performance issue, I'm thinking most people would agree that a default change is completely undesirable, and then move straight to wondering about the opt-in option.
Since "selection foreground" is only a concern when there is a selection, and when a selection is in the current viewport, what about this: An option that re-renders the text foreground only when a selection is active, and only when a selection is present in the current viewport. This last part probably goes without saying (duh), but if 100 selections are present it would be nice if we could at least not have the UI crippled when none of them are in-view. Computation time to assess the presence of selected text might make this last part invalid.
I don't know what kind of performance hit is present with a dynamic toggle of rendering like that, or if a reload is required of the app or window to change that behavior. If this cannot be dynamically toggled (and I'm guessing not) then we're back to an opt-in switch.
So let's follow-up on that. Assume we go to the .json, change and save the rendering option for text foreground re-render. Does saving that cause significant overhead? Again, we only need this to be active when we have a selection active, not all the time. So if it's "somewhat" painless, I'm thinking we could use a hotkey to toggle the .json setting, and manually toggle this rendering only when we know we want it. Elegant? No. But at least the text will be readable in these rare spots where it's not under specifiic conditions. Personally, I have no problem with some flicker that's easily toggled - like most I'd opt out if it can't.
For anyone curious about what 15FPS is like, see this appspot demo. I dunno yet, but we're not dealing with "graphics" here, we're dealing with text. Is 15FPS on a screen that's not moving around too much truly that painful? That's subjective. On higher-end graphics cards we might not go that low and notice any degradataion.
I'm sorry that I'm not familiar with this code to look deeper and suggest a PR. But if you can point me/us to where some of this happens, any of us would be in a better position to research it. How painful would it be to alpha a build with a toggle on the rendering to see what we're really talking about here?
Thoughts? Thanks!
randrew
commented
1 year ago I'm only an occasional VS Code user, but I do use it to check compatibility with some of my projects every once in a while. I noticed the lack of ability to have high-contrast selections in normal themes. It's not the end of the world, but it's a nice thing to have.
@alexdima — Thank you for the explanation of why VS Code doesn't have this capability by default. It makes sense. Of course, doing re-rasterization of text when the selection changes is slow. If it's doing re-layout, that's even slower.
I don't know very much about web browsers, so I might be wrong. But, when you talk about layers, I'm assuming that these layers are composited on a GPU or a dedicated software compositor to produce the final image.
Quickly updating the text selection was an issue that old software also had to deal with back in the day. There was a simple trick to doing this effectively on older, much slower hardware. The existing pixel buffer would simply be inverted in-place. When deselecting, just invert it again, restoring the original image.
In this animation, the word processing software is not doing any text rendering. It's just modifying the already-rendered image in the pixel buffer.
I'm wondering if you could use the layers technique you already have to leverage a similar trick, but without having to mutate any pixel buffers. I looked up some information about CSS blending and found this: https://developer.mozilla.org/en-US/docs/Web/CSS/mix-blend-mode
I don't know if the VS Code rendering layers system is using the same blending modes available in this CSS thing. But it looks like it has the capability of doing difference and exclusion blending modes. Maybe something like changing the blending mode of the selection layer from normal (alpha) mode to difference or exclusion mode, so that it inverts the text and background color behind it, could allow selection color inversion to be added without changing too much existing code or affecting performance?
There are some limitations of this approach. Interference with sub-pixel rendering patterns ("ClearType") producing slight color fringing, if sub-pixel rendering is enabled. And not having full control over both the resulting text and resulting background color. But I think it could be good for accessibility purposes, without slowing down the editor.
I apologize if this is totally off base and not feasible for this project.
JustDre
commented
1 year ago I'd played with the idea of CSS blend modes to see if that would work, but VSCode has many layers and colors that interact in a complex way.
The basic idea of the "overlay" is to replace the background color very quickly, potentially in hardware. Unfortunately, it does nothing for the text since it is literally just rectangular blocks with no text, and is actually an underlay. To get text to change color, there would have to be an actual overlay and there would have to be text in the blocks. However, the code that generates the blocks does not have access to the text and all the necessary data to render it in the correct face and style. It might be simple to pass that data, or call the services which generate the inline CSS for the text, but you'd have to discard or override anything that affects the color.
After all that you might be worse off performance-wise than the previous, simpler solution that was rejected due to performance regression.
Personally I think the previous PR should be made available as an opt-in choice.
crmb
commented
1 year ago I am coming from a dinosaur-era editor (Ultraedit) and this missing "feature" hurts.
(Same with non-modifiable editorBracketMatch foreground)
beshad
commented
1 year ago the "editor.selectionForeground" feature still doesn't seem to work
oliversturm
commented
1 year ago Ah, come on. This issue will be six soon, can't fix it now.
vsjoshtait
commented
1 year ago Suggest birthday cake?
Steps to Reproduce:
workbench.colorCustomizations.editor.selectionForegroundExpected result: a text selection should use the configured foreground color Actual result: it doesn't.
I notice that the tooltip in the settings editor says "Color of the selected text for high contrast". I'm not sure what that means, my only thought was that it might apply to the "High Contrast" theme I see mentioned in the docs. I tried to switch to that theme, but it doesn't make any difference whatsoever (seriously, none at all - it's weird because I thought I did that before and things looked different then - is high contrast broken now?).
In any case, I'd like to see selection foreground color implemented regardless of theme. For presentation purposes, I want a bright selection background color (my preference is bright yellow), and this is generally incompatible with any kind of syntax-highlight colored text.