1476524 - Address bar text selection colors are reversed in Dark theme

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P5)

Description

[Jens]

Attachment: awesomebar_before.png

mozregression says:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e01bc035fe389c36fcf95426c75a754b028f3581&tochange=b3b2ac62bbe0c726eb47ee5d2ce89ce332f82791

which is Bug 1475509

AR (what it looks like now in Nightly with Dark Theme, both on Windows and Linux):
awesomebar_before.png

I think the white background is too much into the eyes. Is this deliberately done?

Also when double clicking (doing a selection) in address bar, then it's also white.


ER (what it looked before):
awesomebar_after.png

Comment 1

[Jens]

Attachment: awesomebar_after.png

Comment 2

[Pascal Chevrel:pascalc]

Bug 1475509 was about improving the theme for Mac OSX Mojave, I don't think it should have impacted Windows and Linux themes.

Tim it seems your patch changed the behaviour on other platforms, could you have a look please? Thanks

Comment 3

[Tim Nguyen :ntim]

Dão, what do you think should be done here ?

Comment 4

[Jens]

Attachment: Ubuntu_after__incorrect.png

fwiw, here's what it currently looks in Ubuntu. (I use windows and ubuntu and imho on both it regressed, cannot say anything about macOS)

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #3)
> Dão, what do you think should be done here ?

Not sure. Back out your patch? Get UX input? FWIW, I'm pretty sure Gecko intentionally flips these colors on dark backgrounds to increase contrast.

Comment 6

[Tim Nguyen :ntim]

Hi Amy,

In bug 1475509, we removed the custom background/text color for selections to match the native highlight color, which made the highlight look like this on Windows/Linux.

How should the highlight look on the dark theme ?

Comment 7

[Amy Lee [:amylee] UX]

(In reply to Tim Nguyen :ntim from comment #6)
> Hi Amy,
> 
> In bug 1475509, we removed the custom background/text color for selections
> to match the native highlight color, which made the highlight look like this
> on Windows/Linux.
> 
> How should the highlight look on the dark theme ?

Hi, 

The highlight colour for text should match the highlight colour around the search box. "awesomebar_before.png" is correct. This colour rule should be applied to the Ubuntu version :-)

Comment 8

[Julien Cristau [:jcristau]]

P5, removing release tracking flag.

Comment 9

[Dão Gottwald [:dao]]

63 is affected nevertheless.

Comment 10

[Florens Verschelde :fvsch]

Attachment: macos-dark-theme-selected-url.png

Not sure if this is strictly the same issue, but on MacOS 10.13 the URL bar became very hard to read too when selected (white text on light blue background). Current stable (top left) is much more readable.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Florens Verschelde from comment #10)
> Created attachment 8996474 [details]
> macos-dark-theme-selected-url.png
> 
> Not sure if this is strictly the same issue, but on MacOS 10.13 the URL bar
> became very hard to read too when selected (white text on light blue
> background). Current stable (top left) is much more readable.

Tim, has bug 1475509 regressed this too?

Comment 12

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #11)
> (In reply to Florens Verschelde from comment #10)
> > Created attachment 8996474 [details]
> > macos-dark-theme-selected-url.png
> > 
> > Not sure if this is strictly the same issue, but on MacOS 10.13 the URL bar
> > became very hard to read too when selected (white text on light blue
> > background). Current stable (top left) is much more readable.
> 
> Tim, has bug 1475509 regressed this too?

Yep.

Comment 13

[Tim Nguyen :ntim]

Attachment: Bug 1476524 - Fix selection colors on dark theme.

Review commit: https://reviewboard.mozilla.org/r/260764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260764/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Tim said that the patch wasn't working but if the rules are moved to browser/themes/shared/compacttheme.inc.css it works... That's slightly weird so I'll take a look if time permits :).

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Tim found that this is because of the default XUL namespace in that sheet.

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8996687 [details]
Bug 1476524 - Fix selection colors on dark theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260764/diff/1-2/

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8996687 [details]
Bug 1476524 - Fix selection colors on dark theme.

https://reviewboard.mozilla.org/r/260764/#review268422

It's not quite clear to me that we should do this for all dark themes. Like I said Gecko intentionally adjusts the selection colors to increase contrast.

Have you figured out why this isn't working on Mac?

Comment 18

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #17)
> Comment on attachment 8996687 [details]
> Bug 1476524 - Fix selection colors on dark theme.
> 
> https://reviewboard.mozilla.org/r/260764/#review268422
> 
> It's not quite clear to me that we should do this for all dark themes. Like
> I said Gecko intentionally adjusts the selection colors to increase contrast.
> 
> Have you figured out why this isn't working on Mac?

I'm not sure. Markus, Stephen, do you know why the selection colors look like attachment 8996474 [details] on macOS ?

This code [0] is supposed to increase the contrast, but it seems like it is not the case on Mac.


[0]: https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/layout/generic/nsTextFrame.cpp#3839

Comment 19

[Florens Verschelde :fvsch]

Attachment: firefox-mac-selection-contrast.png

> Gecko intentionally adjusts the selection colors to increase contrast.

Can confirm that it’s not working on macOS 10.13 (neither in chrome nor content). No idea if it works in the Mojave (10.4) betas.

Nightly correctly picks up the system highlight color in 10.13. Tried with a different color (orange) and a custom one (dark purple), both were used as-is (immediately in the URL bar, and after restarting Nightly for content).

Attached is an example from web content with dark backgrounds, which compares what Chrome does to the current result in Firefox Nightly. Chrome seems to be tweaking the selection's background color to improve contrast, while Firefox doesn’t.

Comment 20

[Stephen A Pohl [:spohl]]

(In reply to Tim Nguyen :ntim from comment #18)
> (In reply to Dão Gottwald [::dao] from comment #17)
> > Comment on attachment 8996687 [details]
> > Bug 1476524 - Fix selection colors on dark theme.
> > 
> > https://reviewboard.mozilla.org/r/260764/#review268422
> > 
> > It's not quite clear to me that we should do this for all dark themes. Like
> > I said Gecko intentionally adjusts the selection colors to increase contrast.
> > 
> > Have you figured out why this isn't working on Mac?
> 
> I'm not sure. Markus, Stephen, do you know why the selection colors look
> like attachment 8996474 [details] on macOS ?

No, I wouldn't know.

Comment 21

[Markus Stange [:mstange]]

I don't know either without debugging.

Comment 22

[Jens]

Bug 1475509 causes these regressions on Windows and Linux and as pointed out in Comment 10 also MacOS. These regressions are very noticable because they affect the address bar highlighting. Due to autocompletion and auto-selection of the address bar I notice this bug like a dozen times during a normal 1h browsing session. When I regressed I almost immediately "eye-catched" this bug, the wrong selection colors make it hard to read and really look wrong.

As it looks all the NIs said they "do not know", Tim has unassigned. What now? 

Could Bug 1475509 be backed-out at least until it's known on how to advance here? Looks like it causes more harm to keep it in at the moment imho.

Comment 23

[Florens Verschelde :fvsch]

Agreed that at the very least, P5 is too low a priority.

For macOS users, it's a very visible bug:
- if you click the URL in the awesomebar, Firefox selets the whole URL (to make it easier to input a completely new one);
- and URL autocomplete uses the selection style for the autocompleted part.

Comment 24

[Dão Gottwald [:dao]]

Florens, since the macOS issue affects web content independently from bug 1475509, can you please file a separate bug on that?

Comment 26

[Tom S [:evilpie]]

This is pretty major regression, can this be addressed?

https://www.reddit.com/r/firefox/comments/9qv7zd/with_the_lastest_update_my_urls_are_getting/
https://www.reddit.com/r/firefox/comments/9lgbd1/unreadably_bright_text_highlight_in_default_dark/

Comment 27

[Tim Nguyen :ntim]

(In reply to Tom Schuster [:evilpie] from comment #26)
> This is pretty major regression, can this be addressed?

I'm fixing macOS in bug 1486204.

We use EnsureSufficientContrast [0] on Windows (attachment 8992879 [details])/Linux (attachment 8993066 [details]), which give the colors with the most contrast, but also not very eye-pleasing results.

I think it'd be reasonable to match Chrome or Edge behaviour for dark web content, or only match their behaviour for the default theme, and keep the old Firefox behaviour for high-contrast themes, not sure...

In any case, I don't have time to fix Win/Linux, but [0] should be a good starting point to change.

[0]: https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/layout/generic/nsTextFrame.cpp#3797

Comment 28

[Dão Gottwald [:dao]]

It's not clear to me that anything needs fixing on Windows and Linux, since the current behavior is intended by Gecko. We may want to change this behavior, but this doesn't seem like a pressing issue.

Comment 29

[Pascal Chevrel:pascalc]

Updating tracking flags as we get closer to the 64 release, fix-optional on 65 as this is a P5.

Comment 30

[Will]

This should not be left on the back burner, especially because it can't be hard to fix. More and more people are using Firefox dark theme on Windows

Comment 31

[armando_cgiraldi]

This would be a great QoL update. Could not be something revolutionary but certainly would impact the experience of everyone using dark mode. Native dark theme is a huge difference between chrome and firefox, seems like google is working on their dark mode, dont let they do it better to retain more users

Comment 33

[Will]

gentle ping (this one is surprisingly annoying)

Comment 34

[Will]

Firefox team, please fix this. It's been 2 years :( 2 years is just too long for something so highly visible (and dark mode is getting more and more popular)

Comment 35

[Florens Verschelde :fvsch]

Attachment: fx-74-macos-10.15-dark-mode.png

I'm getting an improved result on macOS 10.15 with Firefox 73 and 74 (Nightly).

The text selection color (for both chrome and content) is correctly inherited from system settings and seems to be applied with a partial opacity which gives good results on both light and dark backgrounds. (Don't know if it's just opacity or if there's some color math going on.)

For macOS, I'd consider this fixed.

Comment 36

[Will]

I believe the macOS version of this bug is this https://bugzilla.mozilla.org/show_bug.cgi?id=1486204

Comment 37

[Jens]

Attachment: Windows10 still incorrect

Interesting, after seeing Comment 35 by :fvsch I tested again on Ubuntu 19.10: There the issue is also fixed. That is, the background color is Ubuntu's organge (like the input border), so similar to how it's now on MacOS. Sorry, I have no screenshot at the moment.

Testing on Windows 10 it is however still the white background color, see attachment.

Comment 38

[Kestrel]

(In reply to Jens from comment #37)

I tested again on Ubuntu 19.10: There the issue is also fixed. That is, the background color is Ubuntu's orange

That is most likely because you are using a dark GTK theme with Firefox Default theme, if you switch to Firefox Dark theme (the title of this bug) you should see the same as in Windows.

Comment 39

[Jens]

Kestrel, you are right. My Gnome theme was set to Yaru-dark. With Yaru (light) the bug is still present in Ubuntu too.

Comment 40

[Will]

It would be nice to have this fixed in time for QuantumBar shipping to stable, it would make sense (I know they're technically separate, but still)

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1476524 - Don't require so much contrast for selection background-against-background checks. r=jfkthame

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48a96ab7e13d
Don't require so much contrast for selection background-against-background checks. r=jfkthame

Comment 43

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/48a96ab7e13d