1744749 - Poor contrast in extension popup with color-scheme dark meta tag

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[wofwca]

Attachment: background.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.45 Safari/537.36

Steps to reproduce:

1. Set the browser theme to a dark one (about:addons).
2. Make an extension with a popup with no CSS and with <meta name="color-scheme" content="dark light">
3. Install the extension
4. Open the popup

Actual results:

The background of the popup is light, but the text is also light.

Expected results:

The background is dark and the text is light.

Comment 1

[wofwca]

Attachment: gecko-addon-popup-background.zip

Here's a test add-on

Comment 2

[wofwca]

May be a regression caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1525107 (or related stuff, like https://bugzilla.mozilla.org/show_bug.cgi?id=1738616). Prior to these changes (e.g. see 95.0) popups used to be always fully light-themed, regardless of the browser theme.
Similar bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1701792
https://bugzilla.mozilla.org/show_bug.cgi?id=1739042

Comment 3

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Theme' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 4

[Hani Yacoub, Desktop QA]

Managed to reproduce this issue on macOS 11.6, Windows 10 x64 and on Ubuntu 20.04 x64 on Firefox Nightly 97.0a1 and on Firefox 96.0b2.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1744749 - Clean-up IsTransparentContainerElement. r=#layout-reviewers

No behavior change (you can't have an in-process frame container and a
BrowserChild at the same time), but avoids some needless refcounting
overhead and makes it simpler to follow.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1744749 - Add an API to get the real canvas background color. r=#layout-reviewers

Extensions are using getComputedStyle(body).backgroundColor, which is
wrong at multiple levels.

The one that matters for this bug is that it is not color-scheme aware.

Depends on D133770

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1744749 - Silence a non-fatal assertion that can happen when creating webext panels.

This is drive-by but I hit this assert consistently when running
browser/components/extensions/test/browser/browser_ext_popup_background.js
locally.

The assert was wrong when we're recreating the widget, it is expected that
we've already constructed frames for its contents.

Depends on D133771

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1744749 - Use windowUtils.canvasBackgroundColor to get the web extension background. r=#extension-reviewers

Make popup browsers non-transparent so that we can get a meaningful
canvas background-color from layout. I don't think it matters since
popups are not meaningfully transparent since bug 1449933.

Depends on D133772

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/126d38bfcf30
Clean-up IsTransparentContainerElement. r=layout-reviewers,boris

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce9483d8d18f
Add an API to get the real canvas background color. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/f02163d0a5e8
Silence a non-fatal assertion that can happen when creating webext panels. r=layout-reviewers,boris
https://hg.mozilla.org/integration/autoland/rev/b015d5dbcd90
Use windowUtils.canvasBackgroundColor to get the web extension background. r=extension-reviewers,robwu

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/126d38bfcf30
https://hg.mozilla.org/mozilla-central/rev/ce9483d8d18f
https://hg.mozilla.org/mozilla-central/rev/f02163d0a5e8
https://hg.mozilla.org/mozilla-central/rev/b015d5dbcd90

Comment 12

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9255306 [details]
Bug 1744749 - Use windowUtils.canvasBackgroundColor to get the web extension background. r=#extension-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Low contrast in some webextension popups, see 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: Install the extension in comment 1.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It changes the way we use to get the popup background in a way that shouldn't effectively change behavior. The only place that would change behavior is if the popup has mismatched backgrounds set on both the <body> and the <html> element (in which case, before the patch, we chose the former, and now the later).

But in that (unlikely) case the worst case scenario is that the popup background is the root color, which shouldn't cause contrast issue (cosmetic issue at most).

• String changes made/needed: none

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9255303 [details]
Bug 1744749 - Clean-up IsTransparentContainerElement. r=#layout-reviewers

Given that we passed the early stage of beta, i'm going to let this ride in 97 instead of taking it as an uplift right now. Feel free to NI me if you feel strongly opposed.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

(In reply to Dianna Smith [:diannaS] from comment #14)

Given that we passed the early stage of beta, i'm going to let this ride in 97 instead of taking it as an uplift right now. Feel free to NI me if you feel strongly opposed.

Given the feature that causes the regression was enabled in 96 I think it'd be worth fixing it in beta. I think "Medium" might have been an overstatement for the amount of risk this poses, in practice any issue that the patch could cause would at the very best be purely cosmetic and way less severe than the result without the patch.

Comment 16

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 97.0a1 (2021-12-20) on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9255303 [details]
Bug 1744749 - Clean-up IsTransparentContainerElement. r=#layout-reviewers

:emilio ok thank you for the explanation. I see that QA verified it as well.

Approved for 96.0b8

Comment 18

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e93ab853c264
https://hg.mozilla.org/releases/mozilla-beta/rev/82ed4d9f0a5c
https://hg.mozilla.org/releases/mozilla-beta/rev/c78e4c21d841
https://hg.mozilla.org/releases/mozilla-beta/rev/85010d5acf5a

Comment 19

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 96.0b8 on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.