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

Windows native client has bad anti-aliasing #560

Closed
fadeevab opened this issue Jul 11, 2017 · 35 comments
Closed

Windows native client has bad anti-aliasing #560

fadeevab opened this issue Jul 11, 2017 · 35 comments

Comments

@fadeevab
Copy link

fadeevab commented Jul 11, 2017

Steps to reproduce

Open Windows native client and see

Expected behavior

Fonts have a good quality

Observed behavior (that appears unintentional)

firefox_vs_native
Left: Firefox has good anti-aliasing.
Right: Windows native client has bad anti-aliasing at the same system.
Font: Open Sans.

Seems like there are different anti-aliasing algorithms are enforced. At least.
Left is Firefox.
Right is Mattermost Windows Native client.

firefox_vs_native2

Possible fixes

CSS anti-aliasing settings?

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 11, 2017

What version are you using? We know v3.6.0 had a rendering issue, and believe that it's improved in v3.7.0.

If you still feel it's not good, would you compare to Chrome? The rendering engine is same. So if they have different result, we might be able to consider something.

@fadeevab
Copy link
Author

fadeevab commented Jul 11, 2017

I'm using v.3.7.0.

P.S.: Interesting thing, that Account Settings -> Display -> Display Font list itself looks better that the applied fonts in the chat. So I zoomed in and what I see:

fonts_list

Above: fonts list field before rolling out.
Below: the rolled out list with fonts items.
There are different types of anti-aliasing algorithms.

As far as I know that algorithms could be chosen in CSS.

@fadeevab
Copy link
Author

fadeevab commented Jul 11, 2017

Chrome looks better, not so blur.

Left: Chrome
Right: Windows native app (greyscale anti-aliasing?)

chrome_vs_native

Result of comparison is the same as in above comments.

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 11, 2017

Confirmed on Windows 10 with 100% of DPI scaling. But it looks fine for me in other DPIs like 125 (actually they have different rendering).

Left: Chrome, Right: Mattermost Desktop
125

It happens when using a traditional lower DPI and looks Electron's limitation.
electron/electron#8708

@fadeevab
Copy link
Author

fadeevab commented Jul 11, 2017

@yuya-oc, Mybe it's better than in previous version, but I have 125% DPI scaling by default, and I find it blurry as well as for 100% DPI, especially when you watch it right after browser version.

FYI, Visual Studio Code is on Electron and it has a good anti-aliasing with algorithm like in my examples at the left side. There are lot of streams on github about their struggling with anti-aliasing: maybe some information would be helpful for you.

@fadeevab
Copy link
Author

fadeevab commented Jul 11, 2017

electron/electron#8708 is solved, whether the latest version of Electron is used in your desktop client?
(edited) P.S.: I mean the issue when there two monitors with different DPI... Still, I have two monitors but with the same DPIs.....

@fadeevab
Copy link
Author

P.S.: When I enable display mode to show only one display, restart Mattermost, but it's still grayscale anti-aliased in Windows.

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 12, 2017

@fadeevab The next version, v3.8.0 uses newer Electron that supports different DPI. However it would not resolve the original problem because their reasons are different.

For reference, would you tell me the issue you saw of VSCode?

@fadeevab
Copy link
Author

Let me check wait a minute

@fadeevab
Copy link
Author

The nuance is that VS Code doesn't have an issue with anti-aliasing at the same Windows at the same time.
vscode

@fadeevab fadeevab reopened this Jul 12, 2017
@jasonblais
Copy link
Contributor

@yuya-oc Does this still appear to be an issue with Electron?

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 14, 2017

@jasonblais I couldn't find other open issues. But as far as looking the conclusion of electron/electron#8708, I felt they think it's not fixable.

@fadeevab
Copy link
Author

@yuya-oc, How do you explain, that VSCode has proper subpixel anti-aliasing, not grayscale (my latest screenshot)?

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 15, 2017

@fadeevab I'm worried because I'm not sure why the difference appears. As you said "struggling", it seems not easy.

I tried adding CSS like microsoft/vscode#1896 (comment) by using DevTools console. But I couldn't get better anti-aliasing.

* {
  -webkit-font-smoothing: antialiased !important;
  text-rendering: optimizeLegibility !important;
}

Left: Chrome, Right: Mattermost Desktop
css-antialias

The link below is a minimum implementation of mattermost desktop which uses the latest Electron 1.7.4 (the master branch uses 1.6.10) and CSS insertion like above.

https://www.dropbox.com/s/jcan9895e5bqap1/mattermost-minimum-win32-ia32.zip?dl=0

Wondering if you could test this application and try modifying CSS by using DevTools(Ctrl+Shift+I).

@fadeevab
Copy link
Author

fadeevab commented Jul 15, 2017

(edited): I tested at the one display laptop with 100% DPI, Windows 7.

I did everything and more with CSS, nothing help.

I downloaded and played with basic Electron Demo CSS by toggling developer tools, and I can't make to force subpixel-antialiasing. Electron Demo has grayscale anti-aliasing from the scratch (at least in Windows).

If to download chromium and open index.html with <body>test</body> there is a subpixel antialiasing.

Seems like the Electron issue.
So I suppose VSCode team fought with Electron and found a workaround.

More info about why anti-aliasing could be broken: electron/electron#6344 (comment).

Anti-aliasing could be turned into grayscale by a couple of reasons. One of them is a 3d transform. I just saw in VSCode lines like:

    if (canUseTranslate3d() && !isWindows /* Windows: translate3d breaks subpixel-antialias (ClearType) unless a background is defined */) {
	const transform = `translate3d(0px, -${renderTop}px, 0px)`;
	this.rowsContainer.style.transform = transform;
	this.rowsContainer.style.webkitTransform = transform;
    } else {
        this.rowsContainer.style.top = `-${renderTop}px`;
    }

See https://github.com/Microsoft/vscode/blob/fcd9a8996aca402aa186ad18e5d356ddf0c53e2f/src/vs/base/browser/ui/list/listView.ts.

So, they care about 3d transform to not break subpixel-antialiasing.

I'm not sure about overall Mattermost architecture, but I saw some 3d transform in Platform. Again, I'm not really sure still.

To continue need to force subpixel-antialiasing in the simplest Electron demo, and need to be sure there is no side-effects from the code.

@fadeevab fadeevab reopened this Jul 15, 2017
@fadeevab
Copy link
Author

(I sometimes press this ridiculous "Close and comment" button)

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 16, 2017

@fadeevab Thanks for more detailed feedback! It looks the problem is grayscale anti-aliasing described in electron/electron#6344. When I set backgroundColor for Electron's BrowserWindow, I got good anti-aliasing as well as Chrome.

Actually our app uses <webview> to show mattermost/platform, so we need to set backgroundColor as well. But it's not exposed in Electron. So I added a issue. electron/electron#10025

@fadeevab
Copy link
Author

Great! I saw this point too, but I set just basic background in CSS and it didn't help. Great you managed to deal with it.

@fadeevab
Copy link
Author

Will you add workaround to mattermost or wait for a fix in electron?

@yuya-oc
Copy link
Contributor

yuya-oc commented Jul 16, 2017

Unfortunately there is no workaround we can do in our code. Only Electron's native part can fix this.

@fadeevab
Copy link
Author

I got it. Let's wait than, thank you :)

@RoyYangS
Copy link

RoyYangS commented Aug 5, 2017

It caused by setting the transparent:true !!!! I have a greate font appr now

@fadeevab
Copy link
Author

fadeevab commented Aug 5, 2017

Great!

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 5, 2017

@YangShengHaHa I think transparent:true is not used in the app though, how did you apply your fix? I'm wondering if you can make a PR.

@6a7070
Copy link

6a7070 commented Nov 8, 2017

I'm also interested in a fix for this as well. For me, I avoid using the Windows client because of the lack of focus when I view the text. Is there a solution for this issue? If not, do you know if Electron is actively working to fix it? Thank you!

@yuya-oc
Copy link
Contributor

yuya-oc commented Nov 8, 2017

Hi @6a7070, thanks for your interest. As I wrote at electron/electron#10025, we can know why bad anti-aliasing happens.
To solve this, we need to improve Electron source code. But I haven't ever heard Electron team is working for this.

@jasonblais
Copy link
Contributor

On a side note, @YangShengHaHa we'd be curious to hear how you resolved (where did you set transparent:true)? That might benefit others in the community as well if you were able to find a fix for the problem.

@RoyYangS
Copy link

RoyYangS commented Nov 8, 2017

@jasonblais @yuya-oc when you create your BrowserWindow , set transparent:false in then option of then BrowserWindow constructor.

@yuya-oc
Copy link
Contributor

yuya-oc commented Nov 8, 2017

Hi @YangShengHaHa, unfortunately I could not find difference even when using transparent: false at the current master branch. In the first place, the default value of transparent is false, though.

If transparent: false truly solves the problem, I think we need to set transparent: false to <webview> not only for the main BrowserWindow because their webContents are independent.

I'm wondering if you could show difference as screenshots. And we welcome when you have a pull request.

@fadeevab
Copy link
Author

fadeevab commented Nov 8, 2017

Hey guys, AFAIK, to activate subpixel (good) anti-aliasing need to set backgroundColor to white in some native code, not transparent: "BrowserWindow shows different rendering when using backgroundColor: '#FFF' in the constructor."

@RoyYangS
Copy link

RoyYangS commented Nov 8, 2017

@yuya-oc I can not take picture for our project code because of the security policy, you can write a simple demo and you will get the answer. I consider wherever this is one the cause of font anti... We have the best practice WeLink for electron from Huawei inc. Setting the BrowserWindow transparent to false in main.js resolves our font problem in WeLink ^_^

@6a7070
Copy link

6a7070 commented Nov 18, 2017

Thanks for all the feedback. I'm still unclear how this might be fixed. Is this something I would need to configure in my own compiled version of the desktop client? I find that I don't use the desktop client because of this issue. Is there anything I can do to help test?

@yuya-oc
Copy link
Contributor

yuya-oc commented Nov 18, 2017

In normal Electron apps, @YangShengHaHa's approach would be good to fix the problem. It's to create non-transparent BrowserWindow in main.js. However it's not enough for us.
To show Mattermost webapp, we are using <webview> tag. It has its own renderer process (including background color) apart from the main window. So we need to set non-transparent background for each <webview>, but there are no such APIs in <webview> or its webContents object.
So in order to solve the problem, we need to make a improvement for Electron APIs.

@yuya-oc yuya-oc mentioned this issue Jan 7, 2019
3 tasks
@amyblais
Copy link
Member

Ticket for tracking: https://mattermost.atlassian.net/browse/MM-14133

@devinbinnie
Copy link
Member

Closing old issue, looks like this is working fine. Happy to re-open if we think this is still a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants