[MAC] Enabling back sub pixel antialiasing for code view

[nethip]

Defaulting the code view antialiasing to sub-pixel which can be overridden using a new preference fontSmoothing. This preference takes subpixel-antialiased or antialiased. With these changes, the UI would have gray scale anti-aliasing but the code view container editor-holder, would have subpixel-antialiasing on.

The reason to disable subpixel antialiasing for UI is that, with subpixel AA on, SourceSansPro is not rendering fine on low res monitors in Brackets UI.

Historically sub pixel AA was turned off to circumvent the flickering bug. Looks like this is being fixed in CEF2171 as I am unable to repro this bug anymore.

I tested this PR on variety of hardware (retina, non-retina, external monitor) and the code rendering is a little crisper now on MAC.

[nethip]

Fixes #11013, #4640 and #10169

[nethip]

@busykai @marcelgerber Would you mind reviewing this PR? This is targeted for 1.4.

[marcelgerber]

Please replace the underscore in subpixel_antialiased with a hyphen.

[nethip]

@marcelgerber Good catch! Will do that.

[marcelgerber]

Looks good to me otherwise.

[nethip]

@marcelgerber Thanks for taking a look at this PR! I will incorporate the changes you have suggested and update this PR.

[nethip]

After analyzing how fonts render on both retina and non-retina and also comparing the rendering with other apps I have come to the following conclusion.

1.  Editors like Sublime Atom and Visual Studio code enable sub pixel rendering by default, irrespective of whether the display is retina or non-retina.
   

2.  Some editors, like Sublime, allow users to override this default subpixel rendering. In sublime, “font_options”preference can be used to control the code rendering to use sub pixel AA or gray scale AA. This override is limited to just the code area alone. The rest of the UI is still using sub-pixel rendering.
   

So the logical conclusion I have arrived at was that, we should be enabling sub-pixel rendering by default and give an option to user to override this behavior. So I went ahead and added a new preference “fontSmoothing”, which would take “subpixel-antialiased” or “antialiased” as the values for this preference.

Now with this in mind I went ahead and enabled subpixel-AA for the entire app(apply –webkit-font-smoothing:”subpixel-antialised” to the body tag). After that I looked at how the overall rendering looked like. The code area was looking a bit more crisp.

But the problem was with other parts of the UI like project tree, find in files pane, JS linting pane e.t.c. I could immediately see extra artifacts being rendered. The following screenshot was taken on an external monitor with a resolution of 1680 x 1050, connected to Mac Pro, with subpixel-AA on.

This is the same rendered with gray pixel on AA, which is the current scheme on master.

I am putting the screenshots of Brackets with subpixel-AA on and off. The first one is with subpixel-AA on. Just by glancing we can see that the first one(subpixel-AA on) has some extra pixels which is making it look a little out of place.

I tried substituting the UI font SourceSansPro with system fonts like Lucida Grande and Helvetica Neue and I saw that using system fonts, the fonts render a little better. Looks like SourceSansPro is not rendering fine with text color #ffffff on dark backgrounds after turning on subpixel-AA. I checked on how this is with other apps and found out that Sublime, Atom and Visual Studio Code use system default fonts, which render relatively well for subpixel-AA on cases. Also none of them uses text with color #ffffff and that could be the reason why they text is rendering fine in other apps as white text on black background is producing artifacts even with other apps.

So with the above test results, I went ahead and added subpixel-AA only to the code area. (to #editor.holder alone). Now the following screenshot is the one where subpixel-AA is enabled only to the code area.

[nethip]

Tagging @larz0 for his comments on enabling sub pixel AA only for the code area.

[busykai]

@nethip, I'm wondering if brackets.js is the right place to do this. Perhaps either view/MainViewManager.js or view/WorkspaceManager.js are more appropriate (MainViewManager.js more so, WorkspaceManager.js manages resizing, really). Both react to htmlReady and manipulate #editor-holder.

Also, if moved to another module, I'd reduce the use of string literals and define modules vars for values and names.

[marcelgerber]

Taking another look at this line, I think we could also make it a Boolean pref, as we only really have two values (and I don't think there will be more in the future)

[nethip]

Sure! we could do this to be a bool variable. I only went this approach as I had earlier plans to add none to the list. Now I am thinking should we add this option, if not we could go with the bool approach.

[marcelgerber]

Maybe the place where other font prefs like font.fontSize are defined is best.
Can't look it up right now.

[nethip]

@marcelgerber I could move the preference to some place where font prefs are getting initialised.

[nethip]

@busykai I think you are right about the code to be present in MainViewManager.js. I will move it to that place.

[nethip]

@busykai @marcelgerber While I was unit testing with this change on MAC, the text now looks crisper with white text on black background. However it looks a little fuzzy on black text on white background. After doing some thinking I came up with this idea of why not add a new entry in the metadata section of themes. That way theme authors can decide on whether they would want the AA to be sub pixel or grayscale. That way we could then enable sub pixel AA only for our default dark theme. What do you guys think about this?

@MiguelCastillo Do you think it is a good idea to add another property to the package.son that instructs brackets to either enable sub pixel AA or grayscale AA?

CC @ryanstewart

[MiguelCastillo]

@nethipn I had implemented this in a PR a while ago. #8635... You can checkout the thread. So my thoughts on this now are that we can just create an extension for this instead of putting it in core. What do you think?

[MiguelCastillo]

I can create the extension and rope you guys in for review if you would like.

[nethip]

@MiguelCastillo I checked out your PR #8635. So essentially you are doing what I was thinking about in my latest comment!

I am curious to know what you would be packaging as an extension. Even if we decide to go with the code, that you are planning to package as an extension, IMO that should be added to the core. My understanding of an extension is something that the app wills still function as is, even if that extension is disabled/deleted. In this case, the rendering itself is broken and I am thinking it must in the core. But this is just my opinion and if we have a strong reason to ship this as an extension, that is fine too.

[marcelgerber]

@nethip I don't have a Mac, so I can't repro myself, but is the rendering on dark backgrounds really that much worse than it is on light ones?

The thing is, I can think of scenarios like "The font looks awful with your theme", where you can't really blame the developer because he presumably either didn't know or didn't think about including the antialiasing pref in his theme (maybe just because they don't have a Mac themself).

[MiguelCastillo]

@nethip we can always put things in core... Not a problem. But the reason this was not put into core in the first place is because it's not a setting that applies across platforms. So it didn't seem like a good approach for us to add a setting that only worked on Mac and would potentially cause confusion when users in different platforms wouldn't see a difference when switching the setting. This whole trouble was not worth our effort of including it in core to solve a relatively niche problem.

I am not sure what you mean by rendering is broken... If anti-aliasing is breaking rendering, then we fix it and don't make it an option to break it again.

The extension would possibly be JS that exposes a way to switch the anti-aliasing setting on/off via preferences, a small UI, or even a menu setting similar to settings like Word Wrapping. The extension would have in the repo a large note stating that it is ONLY for Mac.

• I myself run a tiny css that disables font smoothing, which is what I expect the extension to do.

html, body {
   -webkit-font-smoothing: subpixel-antialiased !important;
}

[nethip]

@MiguelCastillo

we can always put things in core... Not a problem. But the reason this was not put into core in the first place is because it's not a setting that applies across platforms. So it didn't seem like a good approach for us to add a setting that only worked on Mac and would potentially cause confusion when users in different platforms wouldn't see a difference when switching the setting. This whole trouble was not worth our effort of including it in core to solve a relatively niche problem.

We could make the preference name mac specific, something like font.fontSmoothingMac, making it easier for the user to figure out that this setting is mac specific. Also now that we have code hints for preferences, we could add Mac only in the description.

I am not sure what you mean by rendering is broken... If anti-aliasing is breaking rendering, then we fix it and don't make it an option to break it again.

Looks like people who are on Mac are having a real bad experience with gray scale AA, especially with white text on black background inside Brackets. This is even becoming a deciding factor for users to either switch with Brackets or migrate to other editors. So this is a very serious problem and I am afraid there is no generic solution to this. I have seen that with sub pixel AA enabled on black text on white background, the fonts are looking a little washed out. And that is where I was thinking of making this specific to a theme. But I just proposed an idea. It could be a bad idea too.

html, body {
   -webkit-font-smoothing: subpixel-antialiased !important;
}

As I mentioned in my analysis above, the problem with the above code snippet is that this enables sub pixel AA for the entire app. But the non-code UI is looking washed out with that. So we might want to enable this only for the code area and that is why I am changing #editor-holder style alone.

[nethip]

@marcelgerber I understand that the theme developers now have to worry about whether to do this use the new setting, if we decide to add it to the theme. Now I am thinking maybe this is a not a good idea to add it as a theme setting. How about just changing sub pixel AA for dark theme alone? That should fix all the issues that I have put forth above.

[MiguelCastillo]

@nethip So with users on Mac complaining that AA is causing a problem, then I would pick the default that works consistently across all platforms... Meaning, pick a default that looks/behaves the same across windows/linux/mac, which is subpixel. I considered doing subpixel just for dark themes, but I strongly believe that providing users with a consistent experience is more important. I think it would be a bit odd if people saw different font rendering for light themes vs dark themes.

I probably wouldn't provide an option to change it back - but this is less important to argue about so I can go either way.

[nethip]

@busykai @marcelgerber @MiguelCastillo I have incorporated all the code review comments. The PR is up for review now. Please take a look at it when you get a chance. Thanks!

[marcelgerber]

Why don't we just make it body.platform-mac #editor-holder?

[marcelgerber]

Oh, I see now. Disregard that.

[nethip]

@marcelgerber This is up for review again.

[marcelgerber]

add: to use subpixel antialiasing on Mac

[nethip]

@busykai @marcelgerber Did you get a look at the updated code in this PR? As this is targeted for 1.4, we need to send the strings for translation and I would really appreciate if we could merge this into master as early as possible.

@prafulVaishnav.

[rawat11]

@nethip I did some testing with new SP AA, The editor part seemed more readable specially in the black background. But some of the UI elements are not looking crisp (as they are in Windows) ( Eg. Lines and Column info shown on the bottom side of editor) Can we do something on that front ?

The text in white background looks totally washed away. I think darken them a bit might make them look better.

[nethip]

@rawat11 Thanks for testing it out! As discussed on this thread, we would go with subpixel AA for all the themes. Users now have a preference to turn this off. And also rendering is not in our control to darken the text as it is completely managed by chrome. AA is the only option using which we can control rendering to some extent.

Could you change the font from Source Code Pro to some system font and update us, how it is looking with lighter theme. Also would you mind checking the same in other editors like Sublime Text and Atom (You might want to change the font and the theme in these editors to look like what it is in Brackets so that we have an apples to apples comparison)

Regarding the UI elements: No change has been done there, so the rendering should be as it is on master.

[rawat11]

@nethip I tested with some system fonts and it did not look much of the difference on Black background

With lighter theme (AA on)

With Sub pixel AA on

White theme looks better with SP AA

Also, I tried the same on Atom, but i could only change the theme, could not find the option for font

I guess Atom has SP AA on by default

According to me White themes aren't looking that good with many of the system fonts but changing it to SP AA makes it look a bit better but still not unto the par. With Black it looks good and SP is definitely a good option to move to.

For the UI, I think we have maintained Gray AA but is there no option to make some change on this, it would only add to readability ad well as rendering

[prafulVaishnav]

@nethip I think there are some conflicts.. Can you please resolve it..

[nethip]

@prafulVaishnav I will resolve the conflict and merge the PR.
I have added this waffle card to track adding unit tests to this PR.

[nethip]

@prafulVaishnav I have resolved all the issues and this PR is now merged 😄