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.

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

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

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

Noting that the screenshots were taken on macOS Mojave Beta.

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)

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.

(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.

I see. Thanks for the clarifications!

Not really specific to Mojave.

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

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.

: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?

(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.

(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.

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.

(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.

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

(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.

> (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.

Great, thanks for clarifying.

I'll file a new bug for the workaround.

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?

(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.

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

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

(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.

(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.

(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

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.

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

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

(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.

(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.

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.

The latest revision solves problem 1 and 2.

Adding an additional reference that we should aim for.

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].

(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.

Next patch will mostly touch Widget: Cocoa

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

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/77b1fad58845 Make nsLookAndFeel.mm return transparent selection colors. r=mstange

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 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.

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.