1771792 - Select dropdowns have white font on bright background on macOS when a dark theme is enabled

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Attachment: Screenshot of the issue

STR:

1. In a recent Firefox Nightly, enable the built-in Dark Theme in about:addons
2. Visit the test case, and open the dropdown

ER:

Light text on a dark background.

AR:

Light text on a light background. See screenshot.

Used mozregression to find the regressor:

13:05.10 INFO: No more integration revisions, bisection finished.
13:05.10 INFO: Last good revision: e06cf44e3b0a181e0fc8bbf348d5f9afda0a86f3
13:05.10 INFO: First bad revision: c8d9ed133a435e0bdda96c27a3175436192cb510
13:05.10 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e06cf44e3b0a181e0fc8bbf348d5f9afda0a86f3&tochange=c8d9ed133a435e0bdda96c27a3175436192cb510

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Testcase

Comment 2

[Mike Conley (:mconley) (:⚙️)]

[Tracking Requested - why for this release]:

Tracking requested because this is a pretty serious readability / accessibility issue for a standard form element when using the browser dark theme.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:glandium, since you are the author of the regressor, bug 1696504, could you take a look?
For more information, please visit auto_nag documentation.

Comment 4

[Markus Stange [:mstange]]

Attachment: same issue for toolbar dropdown menus

We have the same issue for toolbar button dropdown menus.

<select> menus and toolbar button dropdown menus are the only cases we have left where we have "native-looking" menus that are actually non-native.

Comment 5

[Markus Stange [:mstange]]

Attachment: same screenshot with NSVisualEffectViewAllowsVibrancyWorkaround set to true

Executing the following on the Terminal fixes it:

defaults write -g NSVisualEffectViewAllowsVibrancyWorkaround -bool YES

I found this string by running nm AppKit | grep DefaultValueFunction, as described in our SDK docs.

So now we need to find out what "workaround" this is referring to and what we're doing wrong so that we needed this workaround.

Comment 6

[Markus Stange [:mstange]]

Oh, it's probably bug 1736694.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This also "fixes" it fwiw:

diff --git a/widget/cocoa/VibrancyManager.mm b/widget/cocoa/VibrancyManager.mm
--- a/widget/cocoa/VibrancyManager.mm
+++ b/widget/cocoa/VibrancyManager.mm
@@ -33,16 +33,17 @@ static NSAppearance* AppearanceForVibran
         // This must always be dark (dark aqua), regardless of window appearance.
         return [NSAppearance appearanceNamed:NSAppearanceNameDarkAqua];
       case VibrancyType::TOOLTIP:
       case VibrancyType::MENU:
       case VibrancyType::HIGHLIGHTED_MENUITEM:
       case VibrancyType::SOURCE_LIST:
       case VibrancyType::SOURCE_LIST_SELECTION:
       case VibrancyType::ACTIVE_SOURCE_LIST_SELECTION:
+        return [NSAppearance appearanceNamed:NSAppearanceNameDarkAqua];
         // Inherit the appearance from the window. If the window is using Dark Mode, the vibrancy
         // will automatically be dark, too. This is available starting with macOS 10.14.
         return nil;
     }
   }

   // For 10.13 and below, a vibrant appearance name must be used. There is no system dark mode and
   // no automatic adaptation to the window; all windows are light.

Not sure if that rings any bells.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1771792 - Don't do vibrancy for non-native menus. r=mstange

This seems to effectively be the old SDK behavior, and fixes contrast
of dark popups on light backgrounds.

Version check might not be wanted / necessary, but I have no way to
test so I haven't touched old macOS versions without dark mode support.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1771792 - Remove some dead vibrancy code. r=mstange

Drive-by cleanup.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1a1d3acbd3c
Don't do vibrancy for non-native menus. r=mstange

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68b233bf0519
Remove some dead vibrancy code. r=mstange

Comment 12

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing xpcshell failures on test_css-properties-db.js

Backout link : https://hg.mozilla.org/integration/autoland/rev/e6d5520e1597430acb8acca8551364a5ec4b7f04

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=usercancel%2Cretry%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=68b233bf0519e773df31019cef59cb5f92b7fa98&selectedTaskRun=ahqll3ueQ1WQN5xXyoFCSw.0

Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=379680699&repo=autoland&lineNumber=2588

Failure line:
TEST-UNEXPECTED-FAIL | devtools/shared/tests/xpcshell/test_css-properties-db.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | devtools/shared/tests/xpcshell/test_css-properties-db.js | run_test - [run_test : 73] The static database and platform do not match for

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a8d99446c2f
Remove some dead vibrancy code. r=mstange

Comment 14

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/f1a1d3acbd3c
https://hg.mozilla.org/mozilla-central/rev/7a8d99446c2f

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9278840 [details]
Bug 1771792 - Don't do vibrancy for non-native menus. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: comment 0
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): One-liner that affects just menus that aren't native (selects, long-press on back button, etc), and restores previous behavior before the SDK update.
• String changes made/needed: none
• Is Android affected?: No

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9278840 [details]
Bug 1771792 - Don't do vibrancy for non-native menus. r=mstange

Low risk, visible impact, approved for 102 beta 3, thanks.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/13b6b864d211

Comment 18

[Monica Chiorean]

I could not reproduce the issue on builds without the fix on Mac 10.13 using multiple nightly builds 102.0a1(20220526213638). Can you please mention what Mac OS were you using?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

I'm on macOS Monterey 12.1. Thankfully, I can confirm that the latest build of Nightly no longer has the issue for me.

Comment 20

[Peter Magyari (Desktop QA)]

I wasn't able to reproduce the issue on MacOS 12 or MacOS 12.3.1 on my end so I cannot verify the fix myself, but since Mike already confirmed it I'm removing the qe-verify+ flag.