D11262: KDE logout screen background color fix

[Radek Hušek]

Pitel created this revision.
Pitel added reviewers: mart, Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
Pitel requested review of this revision.

REVISION SUMMARY
The patch D5036 <https://phabricator.kde.org/D5036> did calculate wrong whether the background color is light or not which resulted in always black background. The problem was that color reported by plasma is in range 0.0 -- 1.0 not 0 -- 255. Now the background is black only when button background color is dark and otherwise it respects button background color.

FIXES 382264

REPOSITORY
R120 Plasma Workspace

BRANCH
logoutColorFix

REVISION DETAIL
https://phabricator.kde.org/D11262

AFFECTED FILES
lookandfeel/contents/logout/Logout.qml

To: Pitel, mart, #plasma
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Michail Vourlakos]

mvourlakos added a comment.


For Latte I am using a different way to determine Luminance..

https://en.wikipedia.org/wiki/Relative_luminance

something like property real luminance = 0.2126*r + 0.7152*g + 0.0722*b

values above 0.65 threshold I consider them light and lower I consider them dark...

In the internet some are proposing 0.7 as threshold...

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik added a comment.


In color picker I use:

// formula for luminance according to https://www.w3.org/TR/2008/REC-WCAG20-20081211/#relativeluminancedef

var a = [color.r, color.g, color.b].map(function (v) {
return (v <= 0.03928) ? v / 12.92 :
Math.pow( ((v + 0.055) / 1.055), 2.4 );
});

return a[0] * 0.2126 + a[1] * 0.7152 + a[2] * 0.0722;

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Michail Vourlakos]

mvourlakos added a comment.


@broulik do you know why this part is needed?

var a = [color.r, color.g, color.b].map(function (v) {
return (v <= 0.03928) ? v / 12.92 :
Math.pow( ((v + 0.055) / 1.055), 2.4 );
});

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik added a comment.


See https://www.w3.org/TR/2008/REC-WCAG20-20081211/#relativeluminancedef

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Andres Betts]

abetts added a comment.


What will it look like now? Screenshot?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


With dark button background as it used to F5750710: Screenshot_20180312_151959.png <https://phabricator.kde.org/F5750710>
but if you choose some crazy combination (like black text on green background) it will also work (without this patch the background here would be still black)F5750713: Screenshot_20180312_152132.png <https://phabricator.kde.org/F5750713>

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Andres Betts]

abetts added a comment.


Seems pretty good!

+1

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Nathaniel Graham]

ngraham added a comment.

Post by Radek Hušek
With dark button background as it used to F5750710: Screenshot_20180312_151959.png <https://phabricator.kde.org/F5750710>
but if you choose some crazy combination (like black text on green background) it will also work (without this patch the background here would be still black)F5750713: Screenshot_20180312_152132.png <https://phabricator.kde.org/F5750713>

Would you be interested in producing similar patches for the Lock and Login screens, which suffer from the same issue?

- Login screen: https://bugs.kde.org/show_bug.cgi?id=388622
- Lock screen: https://bugs.kde.org/show_bug.cgi?id=369676

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.

Post by Nathaniel Graham
Would you be interested in producing similar patches for the Lock and Login screens, which suffer from the same issue?
- Login screen: https://bugs.kde.org/show_bug.cgi?id=388622
- Lock screen: https://bugs.kde.org/show_bug.cgi?id=369676

Those two seem to be a bit more complicated because the logout screen covers the desktop by some color overlay so we know how the background looks. But with login & lock screen there is only picture on the background and no color overlay.

Adding color overlays on them seems unnatural and ugly. Other thing I tried is to add Glow effect. It solves readability problem but I do not like how it looks and documentation says it also needs OpenGL.

Using background dependent color would require to calculate an "average color" of an image in qml and I do not see how to do that. So the only solution I see right know is to add config options to select text color for lock & login screens.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

Logout.qml:87
+ function isLightColor(color) {
+ return Math.max(color.r, color.g, color.b) > 0.5
}

the method lattes is using 0.2126*r + 0.7152*g + 0.0722*b is more correct
(a simple one would be convertin to gray scale with qGray, but unfortunately is C++ only)

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Fredrik Höglund]

fredrik added inline comments.

INLINE COMMENTS

mart wrote in Logout.qml:87
the method lattes is using 0.2126*r + 0.7152*g + 0.0722*b is more correct
(a simple one would be convertin to gray scale with qGray, but unfortunately is C++ only)

qGray() uses an integer approximation that is only correct for CRT monitors.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


I see that relative luminance formula is more correct but I would like to point out that we do not need here to calculate precise luminance. The point of the `?:` statement is to collapse colors close to back to real black because gray overlay with 50 % opacity does not look good. So the current formula looks good enough for me and I would even think about moving the threshold down a bit to 0.4 or 0.3. (Generally not collapsing to black is the save way, it may not look so good but the text will be still easy to read.)

For reference a screenshot with gray background (similar to default breeze) without collapsing it to black: F5752690: Screenshot_20180313_175108.png <https://phabricator.kde.org/F5752690>

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Marco Martin]

mart added a comment.

Post by Radek Hušek
I see that relative luminance formula is more correct but I would like to point out that we do not need here to calculate precise luminance. The point of the `?:` statement is to collapse colors close to back to real black because gray overlay with 50 % opacity does not look good. So the current formula looks good enough for me and I would even think about moving the threshold down a bit to 0.4 or 0.3. (Generally not collapsing to black is the save way, it may not look so good but the text will be still easy to read.)

yep, gray background definitely doesn't look good.
can you try with the luminance formula and put a screenshot on how it looks?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


I finally got to do some testing and it is not looking good for luminance formula `0.2126*r + 0.7152*g + 0.0722*b`.

Screenshot with background color `#5500FF` (blueish, button background on picture), you can see that black text is readable on it, but it has luminance only 0.14 so in overlay it is replaced by black.
F5769603: Screenshot_20180327_174715.png <https://phabricator.kde.org/F5769603>

So I would rather stay conservative, and used formula `max(r, g, b)`. I am still not sure whether the threshold should be `0.5` or a bit lower...

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Marco Martin]

mart accepted this revision.
mart added a comment.
This revision is now accepted and ready to land.


let's go for it

REPOSITORY
R120 Plasma Workspace

BRANCH
logoutColorFix

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:af2a74341d5f: KDE logout screen background color fix (authored by Pitel).

REPOSITORY
R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D11262?vs=29324&id=30937

REVISION DETAIL
https://phabricator.kde.org/D11262

AFFECTED FILES
lookandfeel/contents/logout/Logout.qml

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Christoph Feck]

cfeck added a comment.

Post by Radek Hušek
I finally got to do some testing and it is not looking good for luminance formula `0.2126*r + 0.7152*g + 0.0722*b`.
Screenshot with background color `#5500FF` (blueish, button background on picture), you can see that black text is readable on it, but it has luminance only 0.14 so in overlay it is replaced by black.
F5769603: Screenshot_20180327_174715.png <https://phabricator.kde.org/F5769603>
So I would rather stay conservative, and used formula `max(r, g, b)`. I am still not sure whether the threshold should be `0.5` or a bit lower...

You can adjust the coefficients. Please check color (0, 0, 128), which is - according to the committed way - considered a light color.

Maybe try something like (r+2*g+b) > 0.8.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


I guess I should have renamed it from `isLightColor` to something like `isNotAlmostBlack`... The point was to replace almost black colors with real black because it looks better with opacity. IMHO `(0,0,128)`, which is deep blue, is far enough from black to not mess with it because font colors readable on deep blue background might not be readable on black one.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Fabian Vogt]

fvogt added a comment.


This seems to be a bugfix - `Plasma/5.12` branch as well?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


It is meant as one. I am still kinda new here, so I am not exactly sure how to that (without confusing Phabricator).

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Fabian Vogt]

fvogt added a comment.

Post by Radek Hušek
It is meant as one. I am still kinda new here, so I am not exactly sure how to that (without confusing Phabricator).

Cherry-pick the commit into the destination branch, merge into master and push both.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Nathaniel Graham]

ngraham added a comment.


Since it was already committed to the `master` branch, wouldn't just need to be cherry-picked into the `Plasma/5.12` branch at this point?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Fabian Vogt]

fvogt added a comment.

Post by Nathaniel Graham
Since it was already committed to the `master` branch, wouldn't just need to be cherry-picked into the `Plasma/5.12` branch at this point?

That's exactly what I wrote?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Nathaniel Graham]

ngraham added a comment.


I meant, we wouldn't need to subsequently merge it into master because in this case, it's already in master.

But yes, for the future, committing to the branch and merging to master is the preferred approach. You can actually use `arc` for this, for future reference: https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Ray Green]

ragreen added a comment.


5.12.4

Way too dark. A guy posted a 5.8 vs 5.12 on Neon's G+.
F5778120: Screenshot_20180330_182831.png <https://phabricator.kde.org/F5778120>

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Nathaniel Graham]

ngraham added a comment.


Ray, this fix hasn't made it into a release yet; it was just committed this morning. So we wouldn't expect someone using 5.12.4 to benefit from the fix yet.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Ray Green]

ragreen added a comment.


Okay, Nate. It's good to know there's hope.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Ray Green]

ragreen added a comment.


Will this also fix the aliasing in the logout screen?

https://plus.google.com/communities/105101838887387505413

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


@ragreen I am sorry to disappoint but this patch does not remove the (usually black) half-opaque overlay the G+ post refers to. It only changes its color to button background color in the case that button background color is too far from black to make sure the text is readable. I guess the readability of text was the reason to introduce the overlay in the first place because only blurring the background (like 5.8 seems to do) is not enough in general.

I am not sure what artifacts is he referring to (but I am usually blind to this kind of issues)...

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.


It also should be in `Plasma/5.12` now.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Ray Green]

ragreen added a comment.


@Pitel I'm sorry to say this but they had nailed it in 5.8, then they ruined it in 5.12.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Nathaniel Graham]

ngraham added a comment.


@ragreen, this isn't the right place for that kind of discussion. Phabricator Differential is used for patch and code review, not commentary on larger design decisions. If you'd like to start a conversation about whether the current UX is a regression (and I would agree with you, FYI), it should be done in an applicable Bugzilla ticket or Phabricator Task, such as T7682 <https://phabricator.kde.org/T7682>.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Ray Green]

ragreen added a comment.


@ngraham You're right. Thanks for the heads-up.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Radek Hušek]

Pitel added a comment.

Post by Radek Hušek
I am not sure what artifacts is he referring to (but I am usually blind to this kind of issues)...

I'm not.

I did not mean that I want to ignore them, I just need them pointed out for me to start seeing them.

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Christoph Feck]

cfeck added a comment.

Post by Radek Hušek
I guess I should have renamed it from `isLightColor` to something like `isNotAlmostBlack`... The point was to replace almost black colors with real black because it looks better with opacity. IMHO `(0,0,128)`, which is deep blue, is far enough from black to not mess with it because font colors readable on deep blue background might not be readable on black one.

Ah okey. I had the impression this was used to decide if light or dark text needs to be used. I wouldn't want to see black text on dark blue buttons :)

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

[Richard Addison]

raddison added a comment.


It looks like that in Kubuntu 18.04 ...
F5866833: current.png <https://phabricator.kde.org/F5866833>

Seems okay to me. It's not dark, it's fairly transparent and it doesn't have any artifacts. Can anyone confirm it with a screenshot?

REPOSITORY
R120 Plasma Workspace

REVISION DETAIL
https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: raddison, ragreen, fvogt, cfeck, fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart