1486204 - Selection color not visible on dark background on macOS

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Arda Çebi]

Attachment: Comparison of the highlighted text on the URL bar on Dark and Light Mode.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180824100112

Steps to reproduce:

I enabled dark mode from settings on macOS and Firefox successfully changed it's theme to a dark mode following the system's setting.


Actual results:

This resulted in an issue where the highlighted text on the URL bar to be not visible. (See screenshot for details)


Expected results:

The highlighted link text should've been visible to the user just like light mode.

Comment 1

[Andre Klapper]

Please do not prioritize tasks if you do not plan to work on them. Thanks!

Comment 3

[Dão Gottwald [:dao]]

There's a Mac-specific problem independently from the regression that is bug 1476524. See bug 1476524 comment 19.

Comment 4

[Stephen A Pohl [:spohl]]

I'm getting a bit lost in the various bugs/comments. Could you clarify what exactly needs to be fixed in widget:cocoa?

Comment 5

[Arda Çebi]

Noting that the screenshots were taken on macOS Mojave Beta.

Comment 6

[Arda Çebi]

This is exactly what I experience: https://bug1476524.bmoattachments.org/attachment.cgi?id=8996474

(Screenshot from https://bugzilla.mozilla.org/show_bug.cgi?id=1476524)

Comment 7

[Stephen A Pohl [:spohl]]

Thanks, I understand that this affects macOS. The question for :dao was regarding the fact that this bug was moved to the widget:cocoa component. I would like to understand what kind of platform code is suspected to be at issue here, rather than front-end JavaScript/CSS code.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Stephen A Pohl [:spohl] from comment #4)
> I'm getting a bit lost in the various bugs/comments. Could you clarify what
> exactly needs to be fixed in widget:cocoa?

I wouldn't know, but this bug appears to be Mac-specific.

(In reply to Stephen A Pohl [:spohl] from comment #7)
> Thanks, I understand that this affects macOS. The question for :dao was
> regarding the fact that this bug was moved to the widget:cocoa component. I
> would like to understand what kind of platform code is suspected to be at
> issue here,

Probably a good starting point: https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/layout/generic/nsTextFrame.cpp#3839

> rather than front-end JavaScript/CSS code.

Apparently this affects web content, see attachment 8997710 [details]. No front-end code involved.

Comment 9

[Stephen A Pohl [:spohl]]

I see. Thanks for the clarifications!

Comment 12

[Tim Nguyen :ntim]

Not really specific to Mojave.

Comment 15

[Shakeel Osmani]

I can also confirm one more thing, when Firefox used in private browsing mode, the text highlight works, if that helps

Comment 16

[Jens]

Issue starts to hit main users:
https://www.reddit.com/r/firefox/comments/9lgbd1/unreadably_bright_text_highlight_in_default_dark/

As said in the "See also" bug 1476524, this is not only MacOS, but affects all Dark theme users (also Windows and Linux). Not sure why bug 1476524 is P5 whereas this here is P2. Both bugs have the same regression source and where introduced with bug 1475509.

Comment 17

[Stephen A Pohl [:spohl]]

:ntim, can you give a summary of what was done in bug 1475509 and why? There was no bug description to the bug, which makes it difficult to track down what the problem was and why the fix was what it was. It seems to cause more fallout than anticipated. Can bug 1475509 be backed out until these regressions have been addressed?

Comment 18

[Tim Nguyen :ntim]

(In reply to Stephen A Pohl [:spohl] from comment #17)
> :ntim, can you give a summary of what was done in bug 1475509 and why? There
> was no bug description to the bug, which makes it difficult to track down
> what the problem was and why the fix was what it was.

The selection background was not native for all 3 platforms and therefor did not match MacOS’ chosen selection color.

The visibility issue here is a more general MacOS-only issue also affecting web content, see attachment 8997710 [details].
Other platforms have some code to handle this visibility problem: https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/layout/generic/nsTextFrame.cpp#3839

An ideal fix would be a Safari like behavior, that adapts the selection background when needed.

>  It seems to cause more fallout than anticipated. Can bug 1475509 be backed out until these regressions have been addressed?

Bug 1476524 is a different issue, also related to the platform code, and is acceptable, since the text selection is still readable.

Backing out is an option, but I’d rather land the workaround from bug 1476524, since it respects the native colors. Dão, what do you think?

This bug needs to be fixed either way for the web content.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #18)
> Bug 1476524 is a different issue, also related to the platform code, and is
> acceptable, since the text selection is still readable.

Right.

> Backing out is an option, but I’d rather land the workaround from bug
> 1476524, since it respects the native colors. Dão, what do you think?
> 
> This bug needs to be fixed either way for the web content.

I don't think we need to back out nor work around bug 1476524. We should just fix this bug.

Comment 20

[Stephen A Pohl [:spohl]]

This seems like a rather important bug to fix. However, I'm lacking the bandwidth or background at the moment to knock this one out. Since bug 1475509 has at least partially exacerbated this, :ntim, can you please take this on? Backing out until the platform side has the required changes still seems like a viable option to me.

It would be great if we could coordinate platform changes along with frontend changes in the future to avoid this type of problem.

Comment 21

[Dão Gottwald [:dao]]

(In reply to Stephen A Pohl [:spohl] from comment #20)
> Backing out until the platform side has the required changes still
> seems like a viable option to me.

Backing out that patch would do nothing for web content, and I think we can live with this bug being exposed in the address bar for the time being.

Comment 22

[Tim Nguyen :ntim]

I don't have anything to add. I can back this out from beta if that gives slightly more margin ?

Comment 23

[Stephen A Pohl [:spohl]]

(In reply to Dão Gottwald [::dao] from comment #21)
> (In reply to Stephen A Pohl [:spohl] from comment #20)
> > Backing out until the platform side has the required changes still
> > seems like a viable option to me.
> 
> Backing out that patch would do nothing for web content, and I think we can
> live with this bug being exposed in the address bar for the time being.

How long does "for the time being" mean? Running into this issue in browser chrome is arguably significantly more noticeable than in web content and I can't make a prediction when I would get around to fixing this on the platform side (this bug).

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #22)
> I don't have anything to add. I can back this out from beta if that gives
> slightly more margin ?

Usually, when a change like yours causes unexpected fallout, it is usually the developer's responsibility to either fix the fallout or coordinate a fix with the developers working on the affected area of the code base. Hence I was asking if you could take on this bug and get it fixed. I also stated that I would not be able to get to this bug anytime soon if you expected someone on the platform integration team (me) to take this on, so we should evaluate our options given the current situation.

Comment 24

[Tim Nguyen :ntim]

> (In reply to Tim Nguyen :ntim (please use needinfo?) from comment #22)
> > I don't have anything to add. I can back this out from beta if that gives
> > slightly more margin ?
> 
> Usually, when a change like yours causes unexpected fallout, it is usually
> the developer's responsibility to either fix the fallout or coordinate a fix
> with the developers working on the affected area of the code base.

That's what I've done. I've suggested some possible paths in comment 18 and my preferred workaround (the patch from bug 1476524).  Dão is going to be the person reviewing a potential workaround, so there's not much I can add to the conversation without his opinion.

Comment 25

[Stephen A Pohl [:spohl]]

Great, thanks for clarifying.

Comment 26

[Tim Nguyen :ntim]

I'll file a new bug for the workaround.

Comment 27

[Stephen A Pohl [:spohl]]

Tim, did you see Dao's comment 19? Are you disagreeing with him and stating that you believe that a workaround is the way to go here?

Comment 28

[Tim Nguyen :ntim]

(In reply to Stephen A Pohl [:spohl] from comment #27)
> Are you disagreeing with him and stating that you believe that a workaround is the way to go here?

No, I'm leaving the decision to him, which is why I haven't filed a bug yet.

Comment 29

[Stephen A Pohl [:spohl]]

Dao's comments seemed pretty clear to me. Dao, could you let Tim know exactly how he should proceed here so we all have clarity? Thank you

Comment 30

[Shakeel Osmani]

Let me also add the screenshots do not exactly represent what I face, for me I do not even see a faint selection highlight, the only thing I can do is go to private browsing to make it work. let me attach my screenshots.

https://imgur.com/a/7C0gv5s

Comment 31

[Tim Nguyen :ntim]

(In reply to Shakeel Osmani from comment #30)
> Let me also add the screenshots do not exactly represent what I face, for me
> I do not even see a faint selection highlight, the only thing I can do is go
> to private browsing to make it work. let me attach my screenshots.
> 
> https://imgur.com/a/7C0gv5s

This seems like an unrelated issue to this one.

Comment 32

[Dão Gottwald [:dao]]

(In reply to Stephen A Pohl [:spohl] from comment #29)
> Dao's comments seemed pretty clear to me. Dao, could you let Tim know
> exactly how he should proceed here so we all have clarity? Thank you

It's not necessarily Tim's responsibility, and I don't know more than what I said in comment 8. Again, Tim's patch did not cause the issue, it merely exposed it in one more place.

Maybe a good next step would be to see if the underlying issue is a regression and if so, when it regressed.

Comment 33

[Shakeel Osmani]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #31)
> (In reply to Shakeel Osmani from comment #30)
> > Let me also add the screenshots do not exactly represent what I face, for me
> > I do not even see a faint selection highlight, the only thing I can do is go
> > to private browsing to make it work. let me attach my screenshots.
> > 
> > https://imgur.com/a/7C0gv5s
> 
> This seems like an unrelated issue to this one.

I had opened it as a separate issue and it was closed saying duplicate. The issue I reported is a major impediment to using Firefox as your day to day browser. A fix should be really applied as soon as possible. I disabled all add-ons to no avail

Comment 34

[Shakeel Osmani]

Let me also add this update, the address bar highlight works fine on both dark theme (firefox) and light theme(firefox) on default theme (firefox) you do not see any highlight, for the time being my work around is to choose either of the two other modes.

Comment 36

[Tim Nguyen :ntim]

The reason macOS doesn't adjust the colors for dark backgrounds is that we intentionally set it not to: [0]. It sort of makes sense not to invert the selection colors on macOS, but I think it would be possible to apply a simple scheme like reducing the opacity of the background-color when on a dark background.

I'll try to provide a patch that does just that this week.

[0]: https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/layout/generic/nsTextFrame.cpp#4118-4124

Comment 37

[Tim Nguyen :ntim]

Attachment: Adjust macOS selection background color for dark backgrounds. r=

Comment 38

[Tim Nguyen :ntim]

Since my full build gives me a fully black window on Mojave, I can't actually test this locally.

I pushed it to try instead, and going to download the artifacts later: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d324ac6330dd642263bb826de9b02166a8016bf7

Comment 39

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #38)
> Since my full build gives me a fully black window on Mojave, I can't
> actually test this locally.
> 
> I pushed it to try instead, and going to download the artifacts later:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d324ac6330dd642263bb826de9b02166a8016bf7

This doesn't actually work. I'll see if I have time to fix my build.

Comment 40

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #39)
> This doesn't actually work. I'll see if I have time to fix my build.

Ah, I got the contrast check the wrong way around 😅, flipping the > sign to < in the check works.

Comment 41

[Tim Nguyen :ntim]

Attachment: Bug 1486204 - Make nsLookAndFeel.mm return transparent selection colors. r=mstange

Comment 42

[Tim Nguyen :ntim]

Attachment: Screenshot of URL bar with patch applied

Comment 43

[Tim Nguyen :ntim]

Attachment: Web content screenshot with patch applied

Comment 44

[Tim Nguyen :ntim]

Attachment: Safari screenshot - urlbar

Comment 45

[Tim Nguyen :ntim]

Attachment: Safari screenshot - web content

Comment 46

[Tim Nguyen :ntim]

Here's how the patch works currently: It checks whether the default selection background has sufficient contrast with the text, and if that is the case, it does nothing. If it is not the case, it will blend the selection background at 50% opacity with the actual text frame background.


This works OK for most cases, but there are some problems still:
1) When one part of the selection has sufficient contrast with the text (before color blending), and the other part doesn't, then both parts will have different selection backgrounds (see attachment 9015867 [details])

2) Since the patch does blending, the selection color after blending with ends up less saturated than the original one

Problem 1 can be solved by blending all the time regardless of the contrast.

Problem 2 can be solved by augmenting the saturation before blending.

Comment 47

[Tim Nguyen :ntim]

The latest revision solves problem 1 and 2.

Comment 48

[Aaron Benson]

Attachment: macOS Safari with color accents

Adding an additional reference that we should aim for.

Comment 52

[Tim Nguyen :ntim]

Replying to the Phabricator comment from mstange here:

> Safari uses the same, slightly transparent, selection background color as Firefox in web content

Firefox does not use a "slightly transparent" color, Firefox selection color is a light fully opaque color, which is the main cause of this bug.

> We're trying to solve a very restricted case: selections in a dark URL bar. 

No, we're also trying to solve the same issue for web content: attachment 8997710 [details] (left is safari, right is Firefox).

Given that last point, affecting selection everywhere seems like the way to go. Only changing the text selection based on the contrast produces strange results: see attachment 9015867 [details].

Comment 53

[Markus Stange [:mstange]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #52)
> Replying to the Phabricator comment from mstange here:
> 
> > Safari uses the same, slightly transparent, selection background color as Firefox in web content
> 
> Firefox does not use a "slightly transparent" color, Firefox selection color
> is a light fully opaque color, which is the main cause of this bug.

You're right. I must have gotten confused during testing.

I verified that macOS actually gives us an opaque color here, and not the transparent color that Safari and Chrome use.
I looked at Chrome's code for this and found the Color::BlendWithWhite method, which takes an opaque color as input and tries to find a translucent color of 60%-80% opacity which looks the same when blended with white. This code was last touched substantially in this Webkit changeset from June 2006: https://trac.webkit.org/changeset/14827/webkit

Could we do the same in the macOS LookAndFeel implementation that obtains this color? Having a slightly transparent selection background, in a way which is consistent with other browsers, seems preferable over having an opaque color that is blended with an element's CSS background-color.

Comment 54

[Tim Nguyen :ntim]

Next patch will mostly touch Widget: Cocoa

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Not a new issue and Fx64 is shipping tomorrow, so too late there. Happy to keep this on the radar for 65, though.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77b1fad58845
Make nsLookAndFeel.mm return transparent selection colors. r=mstange

Comment 58

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/77b1fad58845

Comment 59

[Tim Nguyen :ntim]

Comment on attachment 9015863 [details]
Bug 1486204 - Make nsLookAndFeel.mm return transparent selection colors. r=mstange

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: 1475509 (for the urlbar, since web content has been affected for a long time)

User impact if declined: attachment 9003983 [details]

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Select some text in the dark theme url bar or from any dark web page.

List of other uplifts needed: n/a

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Fairly self-contained Cocoa change. Although the selection color now looks slightly more faint for graphite (although still reasonably visible) against the light theme, that's a reasonable tradeoff IMO since this bug improves every other configuration (non-graphite, on dark & light themes + graphite, on dark themes).

String changes made/needed:

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9015863 [details]
Bug 1486204 - Make nsLookAndFeel.mm return transparent selection colors. r=mstange

[Triage Comment]
Fixes text highlighting issues exacerbated by bug 1475509. It's early in the Beta cycle, so approving for 65.0b4.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/28756c341f02

Comment 62

[Gabi Cheta [:Gabi] Desktop QA]

Reproduced the issue on Nightly 66.0a1 (2018-12-02)on macOS 10.10.6.
Verified that the issue is fixed in beta 65.0b4 and Nightly 66.0a1 (2018-18-12) on macOS 10.10.6, macOS 10.11, macOS 10.12.6, macOS 10.13 and macOS 10.14.

Comment 64

[perumeni]

This patch makes selection color oversaturated when gfx.color_management.mode = 1

https://i.imgur.com/gLJCd1Q.png

Comment 65

[perumeni]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #61)

https://hg.mozilla.org/releases/mozilla-beta/rev/28756c341f02

It seems alpha channel then lost (I think here https://dxr.mozilla.org/mozilla-central/rev/7e40e33da3da2640e965a153254594a234231f76/widget/nsXPLookAndFeel.cpp#889-896) which gives result from previous comment.

Comment 66

[Tim Nguyen :ntim]

(In reply to perumeni from comment #65)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #61)

https://hg.mozilla.org/releases/mozilla-beta/rev/28756c341f02

It seems alpha channel then lost (I think here https://dxr.mozilla.org/mozilla-central/rev/7e40e33da3da2640e965a153254594a234231f76/widget/nsXPLookAndFeel.cpp#889-896) which gives result from previous comment.

Thanks for figuring this out!

Markus, Is this something we should fix ? I'm not sure what gfx.color_management.mode = 1 does, but it does look broken with it flipped on.

Comment 67

[Markus Stange [:mstange]]

Yes, perumeni identified exactly the right place. That code should be fixed to apply the original alpha to the color-corrected color.

Comment 68

[Tim Nguyen :ntim]

I filed bug 1547353