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.

Sign up for GitHub

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

Jump to bottom

fix: overly thin font rendering on mojave #15007

Merged
merged 2 commits into from Oct 9, 2018
Merged

fix: overly thin font rendering on mojave #15007

merged 2 commits into from Oct 9, 2018

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 7, 2018 •
edited

Description of Change

Resolves #14948.

Backports this CL from skia, which changes behavior to detect if requesting smoothing does not change the rendering, if it changes the rendering, or if it changes the rendering and applies subpixel coverage.

/cc @deepak1556 @MarshallOfSound

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines
Release Notes

Notes: fix too-thin font rendering in MacOS Mojave

@codebytere codebytere requested a review from a team October 7, 2018 07:05
deepak1556
deepak1556 reviewed Oct 7, 2018
View reviewed changes
patches/common/skia/fix_font_thickness.patch Outdated
UniqueCFRef<CGColorSpaceRef> colorspace(CGColorSpaceCreateDeviceRGB());
- UniqueCFRef<CGContextRef> cgContext(
- CGBitmapContextCreate(&bitmap, 16, 16, 8, 16*4, colorspace.get(), BITMAP_INFO_RGB));
+ SkUniqueCFRef<CGContextRef> noSmoothContext(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SkUniqueCFRef was added recently in https://skia-review.googlesource.com/c/skia/+/155612, the skia version on our master doesn't have it, it should be UniqueCFRef in this patch.

deepak1556
deepak1556 reviewed Oct 7, 2018
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch LGTM with minor change, but please do verify if it fixes the issue before merging since its a fairly recent change in upstream. Thanks!

@cebor cebor mentioned this pull request Oct 8, 2018
Closed
@codebytere
Copy link
Member Author

@deepak1556 verdict: it improves the issue significantly on mojave

@jkleinsc jkleinsc merged commit 532ee2d into master Oct 9, 2018
@release-clerk
Copy link

release-clerk bot commented Oct 9, 2018

Release Notes Persisted

fix too-thin font rendering in MacOS Mojave

@codebytere codebytere deleted the fix-pixel-rendering branch October 9, 2018 14:57
@cebor
Copy link

cebor commented Oct 12, 2018 •
edited

Is there a plan when this will be backported to 2.0.x and 3.0.x?

@jkleinsc jkleinsc restored the fix-pixel-rendering branch October 12, 2018 13:54
@jkleinsc jkleinsc deleted the fix-pixel-rendering branch October 12, 2018 13:54
@codebytere
Copy link
Member Author

@cebor i made an attempt this today and it looks like the patch diffs are incompatible between Chromium M62/M66 and M69, so this may be impossible 😢

I'll try to see what i can do moving forward.

@cebor
Copy link

cebor commented Oct 26, 2018

Ok, does it mean the patch is not compatible with 2.0.x and 3.0.x or only with 2.0.x?

BTW: Great work, can't wait all my electron apps are fixed, cause overwriting the system defaults is not an option for me. 👍

@dalDevelo
Copy link

dalDevelo commented Nov 21, 2018 •
edited

@codebytere I'm running the vscode exploration build which is currently using electron 4.0.0-beta 7. However, I'm seeing zero improvement in font rendering on a retina screen compared to the release version. I still have to run defaults write -g CGFontRenderingFontSmoothingDisabled -bool FALSE in order for fonts in vscode to display as they did prior to Mojave.

@MarshallOfSound
Copy link
Member

/trop run backport-to 4-0-x

@trop
Copy link
Contributor

trop bot commented Nov 21, 2018

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

@electron electron deleted a comment from trop bot Nov 21, 2018
@Zehua-Chen
Copy link

I tried to load microsoft.github.io using the just-released electron 4 beta 8, on macOS Mojave (10.14.1), but I do not think there are any improvements to font rendering compared to the current vs code release.

Electron 4 Beta 8

electron 4 beta 8

VS Code 1.29.1

code

Safari

safari

@dalDevelo
Copy link

@Zehua-Chen . I'm guessing the upgrade to Chromiuim 70 #15405 has to happen before we will see any progress on this issue, as @codebytere mentioned above...

i made an attempt this today and it looks like the patch diffs are incompatible between Chromium M62/M66 and M69, so this may be impossible 😢

@ckerr ckerr mentioned this pull request Dec 18, 2018
Closed
@olizilla olizilla mentioned this pull request Mar 14, 2019
Closed
@miniak miniak mentioned this pull request Apr 9, 2019
Merged
4 tasks
@miniak
Copy link
Contributor

miniak commented Apr 9, 2019

@cebor backported to Electron 3 electron/libchromiumcontent#759

@miniak
Copy link
Contributor

miniak commented Apr 9, 2019

maybe we should backport this one as well https://skia-review.googlesource.com/c/skia/+/177880

@miniak miniak mentioned this pull request Apr 9, 2019
Merged
4 tasks
@dalDevelo
Copy link

This is not fixed in 4.1.5, as per my comment in #17737.

@bpasero bpasero mentioned this pull request Jun 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

@nornagon nornagon Awaiting requested review from nornagon

@alexeykuzmin alexeykuzmin Awaiting requested review from alexeykuzmin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overly thin font rendering in macOS 10.14 Mojave
8 participants
@codebytere @cebor @dalDevelo @MarshallOfSound @Zehua-Chen @miniak @deepak1556 @jkleinsc