Closed Bug 1449933 Opened 5 years ago Closed 5 years ago

Dark theme popups for WebExtensions are hard to read

Categories

(WebExtensions :: Frontend, defect, P2)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 verified, firefox62 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- verified
firefox62 --- verified

People

(Reporter: mconley, Assigned: jaws, Mentored)

References

(Regressed 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

Example
11.96 KB, image/jpeg
Details
Bug 1449933 - Webextension popups that don't define a background-color can be hard to read.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
dark theme.gif
144.79 KB, image/gif
Details
Attached image 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
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
Blocks: 1408121
No longer blocks: 1417883
Mentor: ntim.bugs
Component: Theme → WebExtensions: Frontend
Product: Firefox → Toolkit
(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.
(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.
Extensions developers just develop their popups assuming that the default background is white.
If they would like to match the current theme, they can always opt-in using the browser.theme.getCurrent(); API.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
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 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 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.
(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 :)
Fair enough :)
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 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.
Attachment #8964226 - Flags: review?(mixedpuppy) → review-
(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.
(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.
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 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
Attachment #8964226 - Flags: review- → review?(mixedpuppy)
(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.
(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.
> 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 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.
Attachment #8964226 - Flags: review?(mixedpuppy) → review+
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 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.
Priority: -- → P2
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88657b4a0dd8
Webextension popups that don't define a background-color can be hard to read. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/88657b4a0dd8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(jaws)
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.
status-firefox60: affected → unaffected
Flags: needinfo?(jaws)
Depends on: 1453803
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?
Can you please answer https://bugzilla.mozilla.org/show_bug.cgi?id=1449933#c36 , so that I can than verify it. Ty.
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
(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.
(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.
Depends on: 1459066
Attached image 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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
Regressions: 1540529
You need to log in before you can comment on or make changes to this bug.