Closed Bug 1486204 Opened 6 years ago Closed 6 years ago

Selection color not visible on dark background on macOS

Categories

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

63 Branch
x86_64
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 - wontfix
firefox65 + verified
firefox66 --- verified

People

(Reporter: ardacebi1, Assigned: ntim)

References

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

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.
Component: Untriaged → Address Bar
Keywords: 64bit
OS: Unspecified → Mac OS X
Priority: -- → P4
Hardware: Unspecified → x86_64
Summary: Highlighted text on URL bar not visible while using Dark Mode on macOS → Highlighted text on address bar not visible while using Dark Mode on macOS
Please do not prioritize tasks if you do not plan to work on them. Thanks!
Priority: P4 → --
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
There's a Mac-specific problem independently from the regression that is bug 1476524. See bug 1476524 comment 19.
Status: RESOLVED → REOPENED
Component: Address Bar → Widget: Cocoa
Depends on: 1476524
Ever confirmed: true
Keywords: 64bit
Product: Firefox → Core
Resolution: DUPLICATE → ---
I'm getting a bit lost in the various bugs/comments. Could you clarify what exactly needs to be fixed in widget:cocoa?
Flags: needinfo?(dao+bmo)
Noting that the screenshots were taken on macOS Mojave Beta.
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.
Flags: needinfo?(dao+bmo)
I see. Thanks for the clarifications!
Priority: -- → P2
No longer depends on: 1476524
See Also: → 1476524
Blocks: mojave
Summary: Highlighted text on address bar not visible while using Dark Mode on macOS → Highlighted text on address bar not visible while using Dark Mode on macOS mojave
Not really specific to Mojave.
No longer blocks: mojave
Summary: Highlighted text on address bar not visible while using Dark Mode on macOS mojave → Highlighted text on address bar not visible while using Dark Mode on macOS
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?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs) → needinfo?(dao+bmo)
QA Contact: mstange
(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.
Blocks: 1476524
Flags: needinfo?(dao+bmo)
Summary: Highlighted text on address bar not visible while using Dark Mode on macOS → Selection color not visible on dark background on macOS
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.
Flags: needinfo?(ntim.bugs)
(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 ?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
> (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.
Flags: needinfo?(ntim.bugs)
Great, thanks for clarifying.
Assignee: nobody → ntim.bugs
I'll file a new bug for the workaround.
Assignee: ntim.bugs → nobody
QA Contact: mstange
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?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
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
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
(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
Assignee: nobody → ntim.bugs
Status: REOPENED → ASSIGNED
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.
Attachment #9015814 - Attachment is obsolete: true
Attached image Screenshot of URL bar with patch applied (obsolete) —
Attachment #9015866 - Attachment mime type: text/plain → image/png
Attachment #9015867 - Attachment is patch: false
Attachment #9015867 - Attachment mime type: text/plain → image/png
Attachment #9015868 - Attachment description: Safari screenshot → Safari screenshot - urlbar
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.
Attachment #9015863 - Attachment description: Bug 1486204 - Adjust macOS selection background color for dark backgrounds. → Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text.
The latest revision solves problem 1 and 2.
Attachment #9015863 - Attachment description: Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. → Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r=emilio
Attachment #9015863 - Attachment description: Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r=emilio → Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text.
Component: Widget: Cocoa → Layout: Text and Fonts
Attachment #9015867 - Attachment is obsolete: true
Attachment #9015866 - Attachment is obsolete: true
Adding an additional reference that we should aim for.
Attachment #9015863 - Flags: review?(mstange)
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].
Flags: needinfo?(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.
Flags: needinfo?(mstange)
Next patch will mostly touch Widget: Cocoa
Component: Layout: Text and Fonts → Widget: Cocoa
Attachment #9015863 - Attachment description: Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. → Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r?dholbert
Attachment #9015863 - Attachment description: Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r?dholbert → Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r=mstange
Attachment #9015863 - Attachment description: Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r=mstange → Bug 1486204 - Make nsLookAndFeel.mm return transparent selection colors. r=mstange
Not a new issue and Fx64 is shipping tomorrow, so too late there. Happy to keep this on the radar for 65, though.
Attachment #9015863 - Attachment description: Bug 1486204 - Make nsLookAndFeel.mm return transparent selection colors. r=mstange → Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r=mstange
Attachment #9015863 - Flags: review?(mstange)
Attachment #9015863 - Attachment description: Bug 1486204 - Blend macOS selection background on top of frame background to improve contrast with text. r=mstange → Bug 1486204 - Make nsLookAndFeel.mm return transparent selection colors. r=mstange
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/77b1fad58845 Make nsLookAndFeel.mm return transparent selection colors. r=mstange
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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:
Attachment #9015863 - Flags: approval-mozilla-beta?
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.
Attachment #9015863 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Depends on: 1513401
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1516867

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

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

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

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

Flags: needinfo?(mstange)

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

Flags: needinfo?(mstange)

I filed bug 1547353

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: