1449933 - Dark theme popups for WebExtensions are hard to read

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

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

Attachment: Example

Originally reported here: https://twitter.com/DavisMTL/status/979246175319506945

See attached screenshot.

This add-on is what's being used: https://addons.mozilla.org/en-US/firefox/addon/switch-container/?src=search

Comment 1

[Tim Nguyen :ntim]

The default background should probably be white, not an empty string: https://searchfox.org/mozilla-central/source/browser/components/extensions/ExtensionPopups.jsm#353

A testcase should also be added: browser/components/extensions/test/browser/browser_ext_popup_background.js

Comment 2

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #1)
> The default background should probably be white, not an empty string:
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> ExtensionPopups.jsm#353

You'd also have to set text and border colors so that this doesn't break with dark OS themes (notably the high contrast ones on Windows). Then again, I'm not sure we really want white popups there.

Comment 3

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #2)
> (In reply to Tim Nguyen :ntim from comment #1)
> > The default background should probably be white, not an empty string:
> > https://searchfox.org/mozilla-central/source/browser/components/extensions/
> > ExtensionPopups.jsm#353
> 
> You'd also have to set text and border colors so that this doesn't break
> with dark OS themes (notably the high contrast ones on Windows). Then again,
> I'm not sure we really want white popups there.

The content inside the extension popups frame decides what the text color is. I agree with the border color though.

It makes sense to use white, as this is the background color that's typically used for web content.

Comment 4

[Tim Nguyen :ntim]

Extensions developers just develop their popups assuming that the default background is white.

Comment 5

[Tim Nguyen :ntim]

If they would like to match the current theme, they can always opt-in using the browser.theme.getCurrent(); API.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review commit: https://reviewboard.mozilla.org/r/232966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232966/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238412


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/test/browser/browser_ext_popup_background.js:53
(Diff revision 1)
> -    },
> +      },
> -  });
> +    });
>  
> -  await extension.startup();
> +    await extension.startup();
>  
> -  async function testPanel(browser, standAlone) {
> +    async function testPanel(browser, standAlone) {

Error: Move function declaration to function body root. [eslint: no-inner-declarations]

::: browser/components/extensions/test/browser/browser_ext_popup_background.js:126
(Diff revision 1)
> -  }
> +    }
>  
> -  await extension.unload();
> +    await extension.unload();
> +  }
> +
>  });

Error: Block must not be padded by blank lines. [eslint: padded-blocks]

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/1-2/

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238434

::: browser/components/extensions/ExtensionPopups.jsm:354
(Diff revision 2)
>      if (background) {
>        this.panel.style.setProperty("--arrowpanel-background", background);
>      }

I believe you can remove the `if (background) {` check, now that there's always a default parameter.

::: browser/components/extensions/ExtensionPopups.jsm:357
(Diff revision 2)
> +    if (background == "#fff") {
> +      // Set usable default colors that work with the default background-color.
> +      this.panel.style.setProperty("--arrowpanel-color", "#000");
> +      this.panel.style.setProperty("--arrowpanel-border-color", "ThreeDShadow");
> +    }

I don't think the --arrowpanel-color is ever used anywhere in extension popups (since the popup is directly embed in a <xul:browser>).

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238434

> I believe you can remove the `if (background) {` check, now that there's always a default parameter.

The default paramenter won't be used if the function is called with an empty string (an otherwise falsy value).

> I don't think the --arrowpanel-color is ever used anywhere in extension popups (since the popup is directly embed in a <xul:browser>).

It makes sense to me to set the background and foreground colors together, but I can drop this line if you still don't think it should be here.

Comment 11

[Tim Nguyen :ntim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> > I don't think the --arrowpanel-color is ever used anywhere in extension popups (since the popup is directly embed in a <xul:browser>).
> 
> It makes sense to me to set the background and foreground colors together,
> but I can drop this line if you still don't think it should be here.

It doesn't make sense to simply set the foreground iff the background is #fff. Eg, if the background is #fefefe, the foreground wouldn't be set to black.

I would personally just drop it, because fixing this for all backgrounds is unnecessary complication :)

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Fair enough :)

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/2-3/

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238464

::: browser/components/extensions/ExtensionPopups.jsm:357
(Diff revision 3)
> -  setBackground(background = "") {
> +  setBackground(background = "#fff") {
> -    if (background) {
> -      this.panel.style.setProperty("--arrowpanel-background", background);
> +    this.panel.style.setProperty("--arrowpanel-background", background);
> +    if (background == "#fff") {
> +      // Set usable default colors that work with the default background-color.
> +      this.panel.style.setProperty("--arrowpanel-border-color", "ThreeDShadow");

You'll also need to undo this in https://searchfox.org/mozilla-central/source/browser/components/extensions/ExtensionPopups.jsm#126

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/3-4/

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238458

I applied the patch, set my theme to dark and installed the extension, the issue is not fixed.  This seems like an extension problem, it's not looking at the theme and changing its styles.  I'm also a little confused by the patch.  If no background color in the browser content is set we set the panel to black?  That seems backwards to the bug report (and the panel is already black).  I'm not sure what is being attempted here.

::: browser/components/extensions/ExtensionPopups.jsm:357
(Diff revision 2)
>  
> -  setBackground(background = "") {
> +  setBackground(background = "#fff") {
>      if (background) {
>        this.panel.style.setProperty("--arrowpanel-background", background);
>      }
> +    if (background == "#fff") {

What if the background is #ffe?  This doesn't seem like a good way to handle this.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> Comment on attachment 8964226 [details]
> Bug 1449933 - Webextension popups that don't define a background-color can
> be hard to read.
> 
> https://reviewboard.mozilla.org/r/232966/#review238458
> 
> I applied the patch, set my theme to dark and installed the extension, the
> issue is not fixed.  This seems like an extension problem, it's not looking
> at the theme and changing its styles.  I'm also a little confused by the
> patch.  If no background color in the browser content is set we set the
> panel to black?  That seems backwards to the bug report (and the panel is
> already black).  I'm not sure what is being attempted here.

I tested with the same extension. What platform are you testing on? This works for me on Windows 10. This patch covers the scenario where a theme developer creates a panel and sets a text color but assumes a white background. When no background color is supplied, the <browser> is transparent and shows the white background color of the panel. With the dark theme, the panel is now dark by default and the extension-author-defined black text appears on a dark background. In the absence of a defined background color, we will now default it to white so that the original assumptions will still hold true.

I don't see where we "set the panel to black"? The patch sets the background color (--arrowpanel-background) to white and also sets the border-color to a sensible default.

> ::: browser/components/extensions/ExtensionPopups.jsm:357
> (Diff revision 2)
> >  
> > -  setBackground(background = "") {
> > +  setBackground(background = "#fff") {
> >      if (background) {
> >        this.panel.style.setProperty("--arrowpanel-background", background);
> >      }
> > +    if (background == "#fff") {
> 
> What if the background is #ffe?  This doesn't seem like a good way to handle
> this.

See comment #2 and comment #3.

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)

> I don't see where we "set the panel to black"? The patch sets the background
> color (--arrowpanel-background) to white and also sets the border-color to a
> sensible default.

/me facepalm.  

Somehow I was not looking at the latest patch in the first place, secondarily I was reading the added --arrowpanel-color as --arrowpanel-background.

And...apparently I need to actually build to get changes in ExtensionsPopups to work.

Comment 19

[Shane Caraveo (:mixedpuppy)]

So what happens in the case where an extension gets the theme, changes it's styles to work with dark theme, but without setting a background color?  Do we end up with white text on white background in a dark theme?   

I'm also concerned that this probably sets background for a lot of extensions in any theme, where half the time leaving the backround as it was is the correct thing to do.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238472

Comment 21

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> So what happens in the case where an extension gets the theme, changes it's
> styles to work with dark theme, but without setting a background color?  Do
> we end up with white text on white background in a dark theme?   

The arrow panel theming feature is new enough that it's safe to assume nobody has done this. If somebody has done so, they probably follow changes on the beta channel, and would catch this change as well.

And people can always opt-in to follow the theme using the getCurrent() API.

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Tim Nguyen :ntim from comment #21)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> > So what happens in the case where an extension gets the theme, changes it's
> > styles to work with dark theme, but without setting a background color?  Do
> > we end up with white text on white background in a dark theme?   
> 
> The arrow panel theming feature is new enough that it's safe to assume
> nobody has done this. If somebody has done so, they probably follow changes
> on the beta channel, and would catch this change as well.
> 
> And people can always opt-in to follow the theme using the getCurrent() API.

Couldn't they have gotten text color of something other than the panel values? I've no idea if, or how many, extensions are monkeying with the theme api to blend in better (I do assume that number is very low).  

More to the point, we're doing something to attempt to force-fix extension styling issues and I'm not clear that we should be doing that.  It changes the default panel styling for (likely) the majority use-case making the extension panels almost always inconsistent with the rest of Firefox.

Comment 23

[Tim Nguyen :ntim]

> Couldn't they have gotten text color of something other than the panel values?

If that would be the case, they would also set their own custom background color, and this patch wouldn't change anything for their case.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238514

I didn't catch on to this being a regression. I suspect that most extensions don't bother much with theming, so this would indeed affect many.  As I understand, themes can now set the --arrowpanel-background, and there is no way to get the original default color so we can use that instead.  This also doesn't provide a way for an extension to opt-out and keep the transparency (it would have to set a color instead).  I don't know if that matters.  I'm not terribly happy with hard coding a color here, but I'm not sure of any better way to handle this, so kind of grudgingly ok with it.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review238518

::: browser/components/extensions/ExtensionPopups.jsm:354
(Diff revision 4)
>  
>      let event = new this.window.CustomEvent("WebExtPopupResized", {detail});
>      this.browser.dispatchEvent(event);
>    }
>  
> -  setBackground(background = "") {
> +  setBackground(background = "#fff") {

Please use a better comment with a reference to the regression.  Something like:

Bug 1449933 - Panels are now themed and the likelyhood is that most extensions wont work in dark themes.  If they have not set a background color, we force it to white to ensure visibility of the extension content.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

https://reviewboard.mozilla.org/r/232966/#review239608

::: browser/components/extensions/ExtensionPopups.jsm:358
(Diff revision 4)
> -  setBackground(background = "") {
> +  setBackground(background = "#fff") {
> -    if (background) {
> -      this.panel.style.setProperty("--arrowpanel-background", background);
> +    this.panel.style.setProperty("--arrowpanel-background", background);
> +    if (background == "#fff") {
> +      // Set usable default colors that work with the default background-color.
> +      this.panel.style.setProperty("--arrowpanel-border-color", "ThreeDShadow");

This doesn't seem to make much sense. ThreeDShadow might well be white or close to that depending on the OS theme. If you hardcode the background, you'll probably want to hardcode the border color too.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/4-5/

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/5-6/

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/6-7/

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8964226 [details]
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232966/diff/7-8/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88657b4a0dd8
Webextension popups that don't define a background-color can be hard to read. r=mixedpuppy

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/88657b4a0dd8

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

The bug that regressed this only landed on 61 so this bug does not affect 60. I'll fix the tracking flags. Also I confirmed via local build of Beta.

Comment 36

[Yuri Khan]

So what happens when the author has already opted in to the Right Thing by setting "browser_style": true and applying all the necessary browser-style classes? Do they still have to jump through the hoops querying the browser for the theme colors?

Comment 37

[marius.santa]

Can you please answer https://bugzilla.mozilla.org/show_bug.cgi?id=1449933#c36 , so that I can than verify it. Ty.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Yuri Khan from comment #36)
> So what happens when the author has already opted in to the Right Thing by
> setting "browser_style": true and applying all the necessary browser-style
> classes? Do they still have to jump through the hoops querying the browser
> for the theme colors?

Yes, extension developers will need to use the https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/getCurrent API to get the colors and apply them to their popup.

Comment 39

[Tim Nguyen :ntim]

(In reply to (slow to respond 4/23 to 4/27) Jared Wein [:jaws] (please needinfo? me) from comment #38)
> (In reply to Yuri Khan from comment #36)
> > So what happens when the author has already opted in to the Right Thing by
> > setting "browser_style": true and applying all the necessary browser-style
> > classes? Do they still have to jump through the hoops querying the browser
> > for the theme colors?
> 
> Yes, extension developers will need to use the
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/
> getCurrent API to get the colors and apply them to their popup.

I filed bug 1455960 to make sure that won't be the case in the future.

Comment 40

[Yuri Khan]

(In reply to (slow to respond 4/23 to 4/27) Jared Wein [:jaws] (please needinfo? me) from comment #38)
> Yes, extension developers will need to use the
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/
> getCurrent API to get the colors and apply them to their popup.

That is unfortunate. As long as hand-coding a popup style is considerably easier than doing a browser-styled popup, extension authors will do that, leading to a proliferation of extension popups, all styled differently and mostly favoring light themes.

(In reply to Tim Nguyen :ntim from comment #39)
> I filed bug 1455960 to make sure that won't be the case in the future.

Thank you and I hope it happens soon.

Comment 41

[marius.santa]

Attachment: dark theme.gif

Tested and reproduced issue on Firefox 61.0a1 (20180329222832).
Retested and verified in Firefox 62.0a1 (20180508100105) on Windows 10 x64 and MacOS 10.13.3.