-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[theming] editor.selectionForeground is not working #36490
Comments
It was only supported for high contrast themes. Note that when you set the editor foreground color there won't be any token colors inside the selected range. |
@aeschli any plans to revisit this? it makes stuff very hard to read at times. |
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. |
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:
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. |
I would very much expect the behavior described in the reverted fix:
It is unfortunate this feature seems to not be compatible with "low" contrast themes. |
bump pls |
1.57.1 Still doesn't Work |
Looks like Pycharm is not going to give up... At least for me |
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. |
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. |
bump |
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. |
Bad look for accessibility aspects of VSCode, not sure why this hasn't been tackled? |
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. |
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. |
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. |
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 (Sorry if this note is convoluted. It took me a while to get all the ideas together.) |
Any updates on this? Why does the |
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. |
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 |
Let's focus on process:
Categorization = Labels
AssignmentIt'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? InsightsWhen 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. SummaryIf 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! |
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 #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 3a3f0e4, 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. |
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? |
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.
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. |
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. Personally I think the previous PR should be made available as an opt-in choice. |
I am coming from a dinosaur-era editor (Ultraedit) and this missing "feature" hurts. (Same with non-modifiable editorBracketMatch foreground) |
the "editor.selectionForeground" feature still doesn't seem to work |
Ah, come on. This issue will be six soon, can't fix it now. |
Suggest birthday cake? |
I just re-read a lot of the content above. What strikes me is that lots of time has been spent on considerations about optimizing the rendering of multiple layers, and I believe (though I may be missing something) that the assessment of a potential performance issue also results from this perspective. On that basis, here's my question: Why don't we remove colorization from all content in the selected region, when a selection is established, and re-colorize when the selection is removed? These steps would happen one time, when selection is created or modified, so that the highest performance cost would only occur when the user drags the mouse to change the selection interactively -- as opposed to scrolling or any other operations, which would be simplified by the plain-colored text in the selection. In any case, to repeat the obvious: this problem is important, it's remained unresolved for way too long, and a lot of other editors handle it satisfactorily, even if they were written 20 years ago. Of course any solution could be hidden behind an option if necessary. However, I can't believe we're having a performance discussion about rendering a few lines of text in the correct color, in 2023. |
I'd like to add that I've been using VS Code for years now with a theme based on the high contrast theme (which has this selectionForeground feature enabled) and I'm not experiencing any performance issues that I would notice. And that's on a PC that's a decade old now. |
Would it not be possible to package the change that @aeschli made and @alexdima reverted (see #36490 (comment)) into an addon? That way people who want this feature can add it with the add-on manager and it is not a regression if an addon slows down VS Code. |
Steps to Reproduce:
workbench.colorCustomizations.editor.selectionForeground
Expected 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.
The text was updated successfully, but these errors were encountered: