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

Restore color rendering following Electron 2.0 update #17380

Merged
merged 9 commits into from Jun 4, 2018

Conversation

Projects
None yet
8 participants
@jasonrudolph
Member

jasonrudolph commented May 21, 2018

Fixes #17356

Description of the Change

Background

In #17356 and electron/electron#10732, users reported an unwelcome change to Atom's color rendering following the recent update to Electron 2.0.

This doc from the Chrome team (shared in electron/electron#10732 (comment)) explains that these color differences are the result of a change in Chromium, and that the change is intentional:

Historically, Chrome has been inconsistent in how it handles colors, and would make no effort to ensure that a web designer’s color selection look the same on all monitors. As of Chrome 61, we respect monitors’ color profiles, so all web pages should appear the same on all devices.

However, the document goes on to acknowledge that this change has received a fair bit of pushback:

Many users have filed bugs that Chrome looks different now. Most of these users have been on Linux or Windows, have set a color profile for their monitor (or have had one set for them), but do not want Chrome to respect this color profile. ... The users’ system settings specify how colors should be interpreted on their monitor, but there exist users who are used to the “wrong” colors that they had before.

Chromium provides a flag that allows you to force a particular color profile:

The [“Force color profile” option] tells Chrome to ignore the operating system settings and use some other value instead. Setting this to “sRGB” will make Chrome behave as it did before on Linux and Windows (for 99% of content).

Approach

This pull request allows Atom users to opt in to using the force-color-profile chromium flag.

Usage

To restore Atom's previous color behavior:

  1. Open Atom's settings UI (i.e., Settings View: Open in the command palette)
  2. Click the "Core" tab, find the "Color Profile" option, and choose "Use sRGB color profile"
  3. Restart Atom

color-profile-settings

Alternate Designs

  • We could always force the color profile to srgb.

    However, as described in the doc from the Chrome team, this would essentially "lock all users forever into 1996-era technology," and we'd prefer to avoid that.

  • We could do nothing, but some users want Atom to behave like most other apps by ignoring the user's custom color profile. #17356 (comment), #17380 (comment)

Benefits

Provides an option to instruct Atom to ignore the user's custom color profile, thus causing Atom 1.28's color rendering to match the rendering used in Atom 1.27 and earlier (i.e., the color rendering as it existed prior to the Electron 2.0 upgrade in #17273).

Possible Drawbacks

Adding more code to the startup process means there are more things that can go wrong in the startup process. If we encounter problems reading and/or parsing the config file, it may cause Atom to crash at startup in ways that are difficult to debug.

Verification Process

Using Ubuntu 18.04, build Atom from this branch (which uses Electron 2.0.1) and verify that:

  • With a custom color profile configured:
    • When no config file exists, Atom launches successfully and respects the user's custom color profile
    • When the colorProfile option is not present in the config file, Atom launches successfully and respects the user's custom color profile
    • When the colorProfile option is present in the config file and has a value of "srgb", Atom ignores the user's custom color profile, and Atom's color rendering matches the color rendering seen in Atom 1.27.1
  • Without a custom color profile configured, when the colorProfile option is present in the config file and has a value of "srgb", Atom launches successfully and Atom's color rendering matches the color rendering seen in Atom 1.27.1
  • Changing an existing setting (e.g., font size) is still possible via Atom's settings UI

/fyi @codebytere

jasonrudolph added some commits May 18, 2018

@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented May 21, 2018

Maybe we could alter Atom's CSS in some way to cope with Chromium's updated color rendering behavior?

The problem isn't with the colors in the CSS, but that the colors are now accurate, where before they were oversaturated due to the incorrect mapping being done before. When you are used to the incorrect, oversaturated, colors when they are suddenly now rendered correctly it usually looks "washed out".

there may be some Atom users that feel that the color "regression" reported in #17356 is actually an enhancement, and they may consider it to be a bug to restore the old color rendering.

I definitely fall into that category, you should always be rendering the correct colors 😉.

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented May 21, 2018

Verification Process

Build Atom from this branch (which uses Electron 2.0.1) and verify that this branch's color rendering matches the color rendering seen in Atom 1.27.1 (which uses Electron > 1.7.15):

  • Linux
    • With custom display profile configured
    • Without custom display profile configured

With custom display profile configured

Atom 1.27.1

2018-05-21 at 13 26 16 pm ubuntu 1 27 with custom color profile

Atom 1.28.0-beta1

screen shot 2018-05-21 at 14 57 26 pm

Atom built from this branch

screen shot 2018-05-21 at 15 00 07 pm

Without custom display profile configured

Atom 1.27.1

2018-05-21 at 13 24 50 pm ubuntu 1 27 with default color profile

Atom 1.28.0-beta1

screen shot 2018-05-21 at 14 58 39 pm

Atom built from this branch

screen shot 2018-05-21 at 15 00 42 pm

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented May 21, 2018

Verification Process

Build Atom from this branch (which uses Electron 2.0.1) and verify that this branch's color rendering matches the color rendering seen in Atom 1.27.1 (which uses Electron > 1.7.15):

  • macOS
    • Without custom display profile configured
    • With custom display profile configured

Without custom display profile configured

Using the display's default profile, I see the following results:

screen_shot_2018-05-21_at_14_27_58_pm

With custom display profile configured

Using a custom display profile (ACES CG Linear), I see the following results:

screen_shot_2018-05-21_at_14_30_19_pm

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented May 21, 2018

Verification Process

Build Atom from this branch (which uses Electron 2.0.1) and verify that this branch's color rendering matches the color rendering seen in Atom 1.27.1 (which uses Electron > 1.7.15):

  • Windows
    • Without custom display profile configured
    • With custom display profile configured

Without custom display profile configured

Using the display's default profile, I see the following results:

default-display-profile

With custom display profile configured

Using a custom display profile (Adobe RGB 1998), I see the following results:

custom-display-profile

@simurai

This comment has been minimized.

Member

simurai commented May 23, 2018

@jasonrudolph Thanks for taking all the screenshots. 👌

I guess the "safe thing to do ™️" would be to merge this PR to avoid change.

But the "right thing to do ™️" would probably be to use the system's color profile. On macOS and Windows the differences seem to be noticeable, but it doesn't make Atom "unusable".

Just on Linux, that example with the custom profile:

screen shot 2018-05-21 at 14 57 26 pm

That's quite the drastic change and makes it hard to use. But then I wonder, people that actually use that color profile, they should be used to not a lot of contrast with dark backgrounds, so they are probably using a light theme instead?

So with all that in mind, I lean slightly towards not merging this PR, even tough some people might get upset. Because you can also look at it from this angle: Atom finally respects the system's color profile that the user can tweak and doesn't force you into using sRGB, like it did before.

@simurai

This comment has been minimized.

Member

simurai commented May 23, 2018

Made a test similar to the one in discuss, but on macOS. The CSS value of the background color is: rgb(42, 42, 60):

screen shot 2018-05-23 at 9 30 23 pm

  • When using the default profile for my monitor Thunderbolt Display, the value with the color picker is:
    • Atom 1.27: rgb(42, 42, 61)
    • Atom 1.28: rgb(42, 43, 61)
    • This branch: rgb(42, 42, 60)
  • When switching to the sRGB profile it's exactly like authored:
    • Atom 1.27: rgb(42, 42, 61)
    • Atom 1.28: rgb(42, 42, 60)
    • This branch: rgb(42, 42, 60)

colors

So by using the sRGB color profile in your system settings, you might can get close to "how it used to be" in Atom 1.27 and earlier.

Personally, I'll keep the monitor's default with the slightly corrected colors.

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented May 23, 2018

Possible Drawbacks

According to the Google doc shared in electron/electron#10732 (comment), the updated colors are the "correct" colors, and users are surprised by the updated colors because they "are used to the 'wrong' colors that they had before." Following that line of thought, there may be some Atom users that feel that the color "regression" reported in #17356 is actually an enhancement, and they may consider it to be a bug to restore the old color rendering.

The more I research this topic, the more I'm in favor of closing this pull request. Given the context in the Google doc from the Chrome dev team and the resources that the doc links to, Atom 1.28's color rendering seems to be a move in the right direction (as opposed to being a regression).

#17356 (comment) describes my current understanding of the situation, and I welcome feedback. 🙇

@daviwil

This comment has been minimized.

Member

daviwil commented May 29, 2018

I'm also in favor of closing this PR and keeping things as they are. Thanks for the extremely thorough investigation @jasonrudolph! 🙇

@simurai

This comment has been minimized.

Member

simurai commented May 30, 2018

Having thought about it some more.. I'm in favor of this alternative:

We could potentially make this behavior configurable, either via a command-line switch or a preference specified by the user's local Atom configuration. In other words, the user could choose whether that want to force Atom to use a particular color profile.

Add a config option: [ ] Use the sRGB color profile. Disabled by default.

It is yet another checkbox to maintain. 🙈 But when Atom 1.28 goes to stable, we would have to tell people that experience this issue the following:

  • Without config: "Please change your custom color profile"
  • With config: "Please consider changing your custom color profile or enable the sRGB color profile in Atom's settings"

I just feel that "forcing" people to change their custom color profile is a bit too much to ask.

@daviwil daviwil referenced this pull request May 30, 2018

Closed

Iteration Plan: May 29, 2018 #17426

8 of 17 tasks complete

jasonrudolph added some commits May 31, 2018

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Jun 1, 2018

I've updated the implementation to allow users to opt in to using the force-color-profile chromium flag. I've updated the pull request body to reflect this approach.

@daviwil @maxbrunsfeld: Would you mind reviewing this pull request? I'd love to get your feedback on the design (as described in the pull request body) and on the implementation.

@jasonrudolph jasonrudolph requested review from daviwil and maxbrunsfeld Jun 1, 2018

jasonrudolph added a commit to jasonrudolph/dotfiles that referenced this pull request Jun 1, 2018

Instruct Atom to ingore your display's color profile
This change demonstrates the configuration option implemented in
atom/atom#17380.
@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Jun 1, 2018

I've updated the implementation to use Atom's standard config file (i.e., config.cson or config.json in your Atom home directory) to store this config option. I've updated the pull request body accordingly.

jasonrudolph added some commits Jun 1, 2018

☠☕ Decaffienate ScopeDescriptor
Oddly, I expect this to resolve the CI failure seen in 
https://travis-ci.org/atom/atom/builds/386819124#L1200.
@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Jun 1, 2018

Let's look at the behavior before and after these changes.

Before

#17356 (comment) demonstrated the behavior in Atom 1.28.0-beta1. For this test, we observed how the background color of the text editor is rendered when using Atom's One Dark syntax theme.
We saw that when applying the Adobe RGB 1998 color profile, Chrome 66 and Atom 1.28.0-beta1 both render #282c34 as #2d3037:

demo

After

With the changes in this pull request, Atom still respects the operating system's color profile by default, but the user can optionally force Atom to use an sRGB color profile. We see this in the demo below.

When applying the Adobe RGB 1998 color profile to the operating system, by default Atom respects the color profile and renders #282c34 as #2d3037. When instructing Atom to use an sRGB color profile, Atom renders #282c34 as #282c34.

demo

@maxbrunsfeld

maxbrunsfeld approved these changes Jun 1, 2018 edited

This looks great! Thanks for taking the time to address the UX around color profiles so thoroughly.

@@ -39,6 +42,12 @@ module.exports = function start (resourcePath, startTime) {
atomPaths.setUserData(app)
setupCompileCache()
const config = getConfig()

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jun 1, 2018

Contributor

It would be nice if we could reuse this Config and/or ConfigFile instance in AtomApplication, in order to avoid reading the config file twice on startup (and also to avoid duplicating the code for locating the config file). That said, I don't think it is necessarily important enough to block merging this PR.

@daviwil

This comment has been minimized.

Member

daviwil commented Jun 1, 2018

Excellent work @jasonrudolph! I'm going to quickly test this out on Windows and see if it has the desired effect.

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Jun 1, 2018

I chatted with @daviwil regarding the next steps for this pull request:

  • David is going to test on Windows
  • If all is well with testing, David will merge this pull request and then ship an Atom 1.28-beta hotfix next week

Note: We acknowledge that Atom doesn't automatically relaunch when you change this new config setting, but that's something that could be addressed in a follow-up pull request. 😅

@daviwil

This comment has been minimized.

Member

daviwil commented Jun 2, 2018

Tested this out on Windows, works great!

With Color Profile set to "Use color profile configured in the operating system" the colors are washed out:

1 28-os-profile

Once I change Color Profile to "Use sRGB color profile" and restart Atom, I see this:

1 28-srgb-profile

Thumbs up from me! I'll get this cherry-picked over to 1.28-releases on Monday and ship a hotfix after that.

@daviwil

daviwil approved these changes Jun 2, 2018

@simurai

simurai approved these changes Jun 4, 2018

Note: We acknowledge that Atom doesn't automatically relaunch when you change this new config setting, but that's something that could be addressed in a follow-up pull request.

Yeah, would be nice to be consistent with the custom title-bar setting, but not that high priority.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Jun 4, 2018

I implemented the restart prompt in another branch. Let me know if you want me to cherry-pick the commit(s) into this branch.


  1. 9b58621
  2. c80e701
  3. c7935c5
@daviwil

This comment has been minimized.

Member

daviwil commented Jun 4, 2018

Thanks @thomasjo! I'll go ahead and merge this PR now and then we can merge your improvement afterward.

@daviwil daviwil merged commit fceb6b0 into master Jun 4, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

daviwil added a commit that referenced this pull request Jun 4, 2018

daviwil added a commit that referenced this pull request Jun 4, 2018

Merge pull request #17380 from atom/fix-color-problems-using-electron…
…-2.0

Restore color rendering following Electron 2.0 update

daviwil added a commit that referenced this pull request Jun 4, 2018

daviwil added a commit that referenced this pull request Jun 4, 2018

@atomiks

This comment has been minimized.

atomiks commented Jun 10, 2018

Thanks for this setting! I actually love the oversaturated/vibrant syntax colors of the sRGB color profile, but prefer using the default Color LCD profile on my computer because otherwise images look oversaturated.

@atomiks

This comment has been minimized.

atomiks commented Jun 22, 2018

For some reason this isn't working for me under 1.28.0 on macOS 10.13.4. The OS color profile is "Color LCD" (default).

The Use sRGB color profile is the same as the default one, so the colors are desaturated now 😢 .

@jasonrudolph jasonrudolph referenced this pull request Jun 25, 2018

Closed

sRGB color profile does not work on macOS (1.28.0) #17564

1 of 1 task complete
@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Jun 25, 2018

The Use sRGB color profile is the same as the default one, so the colors are desaturated now

@atomiks: Thanks for reporting this. I see that you've opened #17564, so let's use that issue to discuss the behavior you're seeing.

@bittin bittin referenced this pull request Jul 25, 2018

Closed

1.28 releases #17733

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