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

Extension View's publisher have greyscale rendering #86192

Closed
octref opened this issue Dec 3, 2019 · 15 comments
Closed

Extension View's publisher have greyscale rendering #86192

octref opened this issue Dec 3, 2019 · 15 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug extensions Issues concerning extensions insiders-released Patch has been released in VS Code Insiders lcd-text-rendering verified Verification succeeded
Milestone

Comments

@octref
Copy link
Contributor

octref commented Dec 3, 2019

Ref #85143

Version: 1.41.0-insider
Commit: 9785578
Date: 2019-12-03T05:31:49.954Z
Electron: 6.1.5
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Linux x64 4.15.0-55-generic

image

@sandy081 sandy081 added the extensions Issues concerning extensions label Dec 4, 2019
@sandy081 sandy081 added this to the November 2019 milestone Dec 4, 2019
@sandy081 sandy081 added the bug Issue identified by VS Code Team member as probable bug label Dec 4, 2019
@sandy081 sandy081 modified the milestones: November 2019, December 2019 Dec 4, 2019
@Tyriar
Copy link
Member

Tyriar commented Dec 5, 2019

The title of disabled items is also greyscale:

image

@bpasero
Copy link
Member

bpasero commented Dec 7, 2019

If you know the background color precisely you can compute the opacity by blending colors together and thus avoid opacity which is likely causing the issue in this case.

Example:

return color.isOpaque() ? color : color.makeOpaque(WORKBENCH_BACKGROUND(theme));

@bpasero bpasero removed their assignment Dec 7, 2019
@sandy081 sandy081 modified the milestones: January 2020, February 2020 Jan 27, 2020
@sandy081 sandy081 modified the milestones: February 2020, March 2020 Feb 20, 2020
@sandy081 sandy081 modified the milestones: March 2020, April 2020 Mar 30, 2020
@sandy081 sandy081 modified the milestones: April 2020, May 2020 Apr 24, 2020
@sandy081 sandy081 modified the milestones: May 2020, Backlog Jun 3, 2020
@sandy081
Copy link
Member

I am using opacity in both the cases

@bpasero Do you suggest to use above computation instead of setting css opacity ? If so can you help me in understanding which colours to use for bending.

@bpasero
Copy link
Member

bpasero commented Jun 10, 2020

Yes I think #86192 (comment) is still a good solution, but you might need to be aware of wether your view is inside sidebar or panel now.

@sandy081
Copy link
Member

I am sorry if my question does not make sense - these are rendered inside the list widget and are you saying that every time I have to use opacity I have to do this computation?

I am assuming the list / tree widget takes care of this grey scale rendering and the elements inside it does not need to, is not it @joaomoreno ?

@bpasero
Copy link
Member

bpasero commented Jun 10, 2020

As far as I remember, any element with opacity will not be rendered with LCD anti-aliasing, thus if you know precisely what background color you are on top of, you can compute it manually to avoid this penalty from Chrome renderer.

@bpasero
Copy link
Member

bpasero commented Jun 10, 2020

Hm, I think disregard what I said, I can actually not reproduce the original issue when I tried just now on Windows:

image

@bpasero
Copy link
Member

bpasero commented Jun 10, 2020

So this still reproduces for me on Linux: only the publisher name is not using LCD rendering, though other lists that have text with opacity do not suffer from this issue as far as I can tell. So my suggestion for avoiding opacity might not be the correct solution after all.

image

@joaomoreno
Copy link
Member

I am assuming the list / tree widget takes care of this grey scale rendering and the elements inside it does not need to, is not it @joaomoreno ?

Not sure I understood the question. The tree/list doesn't prevent you from having subpixel AA. But it also doesn't guarantee it for its users, since there are things done on row content, eg opacity, which could prevent it.

@sandy081
Copy link
Member

sandy081 commented Jun 12, 2020

Used the trick @bpasero suggested and it worked

image

sandy081 added a commit that referenced this issue Jun 12, 2020
@sandy081
Copy link
Member

Here are the layers rendered (which I got with the help of @joaomoreno)

image

As from @joaomoreno
It seems there are no new layers around publisher field it could be another bug altogether then and could even potentially be an electron issue

@bpasero @deepak1556 any idea if this is an electron issue and if not shall we have to follow this blending trick at all such places?

@bpasero
Copy link
Member

bpasero commented Jun 12, 2020

@sandy081 we have had a lot of discussion with the Chrome team in the past and I think Alex made a good list of conditions under which Chrome may decide to switch to greyscale rendering (https://bugs.chromium.org/p/chromium/issues/detail?id=1016062#c31). I doubt it is Electron to blame, rather Chrome, but then again unlikely to get a fix imho.

As for opacity in our code: this is not a guarantee for loosing LCD rendering. A lot of places use opacity and render just fine. However, in some specific cases we seem to be loosing LCD rendering and you can find all issues we filed via this query: lcd-text-rendering

As you can see, some of these issues are still open or closed "as designed", simply because the technical solution was not straight forward.

tl;dr;: we try our best to get LCD rendering, but by no means are we thinking that we get to 100%. Just to give an example: notification toasts do not render with LCD rendering because of many factors, such as animations, border shadow, etc. I did not have the energy to make it work there.

Ah, and btw other browsers are fine, Firefox always renders in LCD, no matter what. And this is even platform specific, e.g. I was not able to see this issue on Windows, only Linux.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 27, 2020
@rzhao271
Copy link
Contributor

rzhao271 commented Nov 2, 2020

@bpasero @sandy081 is there an easy way to verify this issue? It seems there's a lot going on here

@rzhao271 rzhao271 added the verification-steps-needed Steps to verify are needed for verification label Nov 2, 2020
@sandy081
Copy link
Member

sandy081 commented Nov 3, 2020

This can be verified only on Linux.

  • Open extensions view and zoom in and make sure publisher's name is not rendered with grey scale.

@sandy081 sandy081 removed the verification-steps-needed Steps to verify are needed for verification label Nov 3, 2020
@rzhao271
Copy link
Contributor

rzhao271 commented Nov 3, 2020

I had a bit of trouble with this until I realized the best way is to take a screenshot and then zoom in, not the other way around. Verified!

@rzhao271 rzhao271 added the verified Verification succeeded label Nov 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug extensions Issues concerning extensions insiders-released Patch has been released in VS Code Insiders lcd-text-rendering verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@joaomoreno @bpasero @Tyriar @octref @rzhao271 @sandy081 and others