Open Bug 1606067 Opened 4 years ago Updated 2 years ago

[macOS ]On OS dark theme, search term highlight color on Home preferences page sticks with the light one

Categories

(Core :: Layout: Text and Fonts, defect, P3)

All
macOS
defect

Tracking

()

Tracking Status
firefox-esr68 --- unaffected
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- fix-optional
firefox74 --- fix-optional

People

(Reporter: asoncutean, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Attached image screeenshot issue.png

[Affected versions]:

  • 71.0
  • 72.0b11
  • 73.0a1 (2019-12-26)

[Affected platforms]:

  • macOS 10.15

[Steps to reproduce]:

Preconditions:

  • The OS general theme is set on dark (System Preference - General - Dark)
  1. Go to about:preferences
  2. Type firefox inside the search input area
  3. Scroll down the page and observe the highlight for the searched term

[Expected result]:

  • The dropdown options from Home page are highlighted in blue

[Actual result]:

  • The dropdown options from Home page are highlighted in yellow (the same as when on light theme), and the contrast makes the text barely visible

[Regression range]:

  • I don’t think this is a regression, it probably started to reproduce once the macOS dark theme feature was introduced. I will provide more info asap.
Has Regression Range: --- → no
Has STR: --- → yes

Hello Anca - Does this only repro using 10.15? Thanks.

Flags: needinfo?(anca.soncutean)

(In reply to Marcia Knous [:marcia - needinfo? me] from comment #1)

Hello Anca - Does this only repro using 10.15? Thanks.

No, I was pressured by time when I logged this issue, but I can confirm now that is also reproducible on macOS 10.14, where the dark OS theme feature is available.

Flags: needinfo?(anca.soncutean)
Component: Theme → Find Backend
Product: Firefox → Core

:ntim, any idea what's going on here?

Component: Find Backend → Preferences
Flags: needinfo?(ntim.bugs)
Product: Core → Firefox

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

(In reply to :Gijs (he/him) from comment #3)

:ntim, any idea what's going on here?

Since this is only happening on macOS (I can't reproduce with the Windows dark mode), my guess is that there is some flawed logic in nsTextPaintStyle::GetHighlightColors.

What's supposed to happen is that the call to Selection::SetColors is supposed to set (yellow, currentColor) as (background, foreground) colors by default, and (blue, currentColor) when there isn't enough contrast.

The flawed logic is probably related to the fact nsTextPaintStyle::GetHighlightColors doesn't take in account the fact the macOS has different default selection colors logic: https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/layout/generic/nsTextFrame.cpp#4082-4084 .

Another possible flaw is the use of EnsureSufficientContrast within nsTextPaintStyle::GetHighlightColors, the problem with EnsureSufficentContrast is that it actually swaps the background/foreground values when the contrast is too low. In combination with the different default selection logic on macOS, this may be causing the contrast checks later on in nsTextPaintStyle::GetHighlightColors to be checking against the wrong values, causing the fallback colors not to be used. I don't know why this would only be happening for menulists though.

I can definitively confirm this is something wrong with the fallback color platform code as flipping the values here moves the issue to the light theme, but I can't check the everything else I said above without some time to debug (which I don't have atm).

Is this something you could look at/forward to the right person ?

Flags: needinfo?(ntim.bugs) → needinfo?(gijskruitbosch+bugs)

Based on hg log, this (see comment #5) was :timdream's work, but he's not around anymore. Looks like Masayuki and Olli reviewed - Masayuki, can you take a look and if not, forward to Olli? Thank you!

Component: Preferences → Layout: Text and Fonts
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(masayuki)
Product: Firefox → Core

Too late for a fix in 71 but we can still take a patch for 73/74.

Priority: -- → P3

This was addressed on Linux (Bug 1549282) by adding -moz-appearance:none to the affected items.

See Also: → 1549282

(In reply to Kestrel from comment #8)

This was addressed on Linux (Bug 1549282) by adding -moz-appearance:none to the affected items.

This is something I tried and it didn't fix it.

Sorry for the delay to reply. Unfortunately, I don't have much time to work on this bug for now.

Looks like that we hit this path:
https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/layout/generic/nsTextFrame.cpp#3889,3897-3898,3902,3907-3909

or

https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/layout/generic/nsTextFrame.cpp#3889,3894-3895,3897,3907-3909

Although I feel blue has better contrast than yellow with white text, but mSufficientContrast might be enough small value.

Anyway, it might be better to compare the luminosity difference with NS_SUFFICIENT_LUMINOSITY_DIFFERENCE rather than mSufficientContrast because mSufficientContrast is introduced for default selection colors won't be swapped in some Linux themes.

Flags: needinfo?(masayuki)

I narrowed down the possible regression window for this issue, at least from what is happening visually in Firefox to a relatively wide interval - https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=47048ef82b50e3130220d86b09f5addf0b58906e&tochange=b8d6494caa5707d7f52c64d5d2f66d0254e69e88 (between this dates, opening the about:preferences page triggers a crash).

Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.