Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

[MAC] Enabling back sub pixel antialiasing for code view #11235

Merged
merged 9 commits into from
Jul 10, 2015
Merged

[MAC] Enabling back sub pixel antialiasing for code view #11235

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1015223
Fix the rendering issue on Mac. Enabling back sub pixel anti-aliasing…
nethip Jun 4, 2015
698f52b
Defaulting the antialiasing to auto
nethip Jun 8, 2015
c7cafae
Merge branch 'master' into prashant/mac-anti-alias-fix
nethip Jun 8, 2015
7a95c0a
Add a preference to turn on/off subpixel-antialiasing
Jun 8, 2015
74e7ff9
Default the code view antialiasing to subpixel which can be overridde…
nethip Jun 9, 2015
7e769b4
Changed the way AA type is getting set. Added a new class by name sub…
nethip Jul 2, 2015
0582f1c
Incorporating some code review comments from @MarcelGerber. Mainly re…
nethip Jul 3, 2015
5efe7a8
Some string changes got from code review comments. Also updated some …
nethip Jul 8, 2015
faad138
Merge branch 'master' into prashant/mac-anti-alias-fix
nethip Jul 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion src/brackets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,42 @@ define(function (require, exports, module) {

// read URL params
params.parse();


Copy link
Contributor

Choose a reason for hiding this comment

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

Could you undo these changes to the whitespace lines? Brackets by default does not trim whitespaces. There's no reason to introduce a change to this. If you have a trim extension, it should probably be disabled for brackets project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@busykai There was some piece of code here in the earlier commits and i just deleted that piece of code based on the code review comments and that is why it is showing as blank space removal change. I could revert this, but I think it is a good idea to remove the extra spaces wherever possible.

// Define a preference for font smoothing mode on Mac.
// By default fontSmoothing is set to subpixel-antialiased
// for the text inside code editor. It can be overridden
// to "antialiased", that would set text rendering AA to use
// gray scale antialiasing.

if (brackets.platform === "mac") {

PreferencesManager.definePreference("fontSmoothing", "string", "subpixel_antialiased", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace the underscore in subpixel_antialiased with a hyphen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcelgerber Good catch! Will do that.

description: Strings.DESCRIPTION_FONT_SMOOTHING,
values: ["subpixel-antialiased", "antialiased"]
});

PreferencesManager.on("change", "fontSmoothing", function () {

var aaType = PreferencesManager.get("fontSmoothing");

// For any other value, set this to subpixel-antialiased
if (aaType !== "subpixel-antialiased" && aaType !== "antialiased") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

aaType = "subpixel-antialiased";
}

$("#editor-holder").css("-webkit-font-smoothing", aaType);

});

AppInit.htmlReady(function () {
// The code editor's text anti-aliasing should be defaulted to sub pixel
// antialiasing on mac
if (brackets.platform === "mac") {
$("#editor-holder").css("-webkit-font-smoothing", "subpixel-antialiased");
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably, it's easier (and more semantically correct) to just add this as an extra .platform-mac #editor-holder selector to the stylesheet.
Chrome may even be optimized for that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcelgerber I could do a .platform-mac #editor-holder but that will make AA non-configurable. The idea is to make the subpixel-antialiased controlled through a preference.

I am thinking of creating another selector and attach/unattach it to #editor-holder based on the preference. Does this make sense? Or the current scheme of things is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

That'd be nicer, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this line set the pref value instead of presumed default?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have access to the prefs at this early point, IIRC

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, only view state is ready by then. My point, though, that it probably should be done when the prefs are ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@busykai I evaluated setting the value when the prefs are ready. But I am thinking that experience would then be the user first sees the gray antialised text and then sub pixel AA text. The might give a feeling of a flicker effect. And that is why I did not go with that approach. Correct me if I am wrong.

}

});
}

/**
* Setup test object
Expand Down
3 changes: 2 additions & 1 deletion src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,5 +743,6 @@ define({
"DESCRIPTION_USE_THEME_SCROLLBARS" : "True to allow custom scroll bars",
"DESCRIPTION_LINTING_COLLAPSED" : "True to collapse linting panel",
"DESCRIPTION_FONT_FAMILY" : "Change font family",
"DESCRIPTION_FONT_SIZE" : "Change font size; e.g, 13px"
"DESCRIPTION_FONT_SIZE" : "Change font size; e.g, 13px",
"DESCRIPTION_FONT_SMOOTHING" : "\"subpixel-antialiased\" to enable sub-pixel antialiasing and \"antialiased\" for gray scale antialiasing"
});
18 changes: 7 additions & 11 deletions src/styles/brackets.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,14 @@ html, body {

/* And make sure we get a pointer cursor even over text */
cursor: default;

/* Turn off subpixel antialiasing on Mac since it flickers during animations. */
-webkit-font-smoothing: antialiased;

// This is a hack to avoid flicker when animations (like inline editors) that use the GPU complete.
// It seems that we have to put it here rather than on the animated element in order to prevent the
// entire window from flashing.
// See: http://stackoverflow.com/questions/3461441/prevent-flicker-on-webkit-transition-of-webkit-transform
// Mac-only though, since this would turn off subpixel antialiasing (ClearType) on Windows

&.platform-mac {
-webkit-backface-visibility: hidden;
backface-visibility: hidden;
// Use gray scale antialiasing for UI. Code view editor-holder
// overrides this to use subpixel antialiasing, which then
Copy link
Contributor

Choose a reason for hiding this comment

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

add: to use subpixel antialiasing on Mac

// can be overridden by setting "fontSmoothing" preference to
// "antialiased". Gray scale AA is used for UI has SourceSansPro
// font does not look good with subpixel AA well on low res monitors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Gray scale AA is used for the UI parts which use SourceSansPro font, which doesn't look good with subpixel AA

-webkit-font-smoothing: antialiased;
Copy link
Contributor

Choose a reason for hiding this comment

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

This setting applies to html, body while the pref applies to #editor-holder. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is correct. The entire UI would have gray scale anti aliasing except for the code editor. Subpixel AA for the UI was not looking good. It exposed some artifacts which was making the entire editor look out of place. I will update the PR with my findings and PR, that will help understand the scenario. Thanks!

}

.dark,
Expand Down