1225633 - Better default styles for WebExtension popups.

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

Attachment: The WebExtension and History panels.

With browserActions and pageActions, it's getting much easier for add-ons to open panels.  We would like those panels to look as much like the native Firefox panels as possible.  I think we can add a stylesheet that will get us much of the way there, and add-on authors are free to reset the styles if they don't like the ones we offer.

(Probably not blocking, but something we'ld like to have.)

Comment 1

[Kris Maglione [:kmag]]

Blake, what do we still need to do for this? I know the pageAction popups still don't match the styling of the browserAction ones, but is there anything else?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

There's a bunch of stuff.  Making ul elements look like the list in the history panel and making the font sans-serif are the first two.  I'ld also like to have footer elements be grey (also as in the history panel), and stuff like that.  (I was planning on taking this one, but was working on the Style Guide while waiting for the new CustomizableUI Panel code to land…  :)

Comment 3

[Kris Maglione [:kmag]]

Ah, so you want to style the contents of the browser?

Interesting...

OK. I guess I'll let you worry about it then. Thanks :)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

The way I figure it, stuff like http://meyerweb.com/eric/tools/css/reset/ is there for add-ons that want to style their own content specifically.  And if they don't care, then this way at least they'll look more firefox-y.  ;)

Comment 5

[Kris Maglione [:kmag]]

Yeah, fair enough. Our UA stylesheets are already different enough from Chrome's, in any case.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Okay, I've got it looking like this so far.  Obviously a WIP, but starting to get there…
https://www.dropbox.com/s/xgq9tggfgtbffl0/Screenshot%202016-02-22%2015.17.20.png?dl=0
https://www.dropbox.com/s/7py159b14o68zvy/Screenshot%202016-02-22%2015.19.12.png?dl=0

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: Work in progress…

Any suggestions on how to make it better would be very welcome!

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8722133 [details] [diff] [review]
Work in progress…

(Looping in shorlander for feedback…)

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: Better wip patch.

Comment 10

[Kris Maglione [:kmag]]

This looks nice.

I don't really have any comments relevant to these popups, but there's also some related functionality for the options API:

https://developer.chrome.com/extensions/optionsV2

They actually have a manifest property to turn the styles on and off[1], so it might be a good idea to do that here.

Somewhat more importantly, it would be nice to have a similar (possibly the same?) stylesheet for that API.

[1] They call it "chrome_style", and disable it by default. I added a "browser_style" alias, and was planning to enable it by default, in bug 1250784.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8722676 [details] [diff] [review]
Better wip patch.

Review of attachment 8722676 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-utils.js
@@ +204,5 @@
>          // dynamically, probably in response to MozScrolledAreaChanged events.
>          this.window.setTimeout(() => this.resizeBrowser(), 0);
> +        let winUtils = this.browser.contentWindow
> +          .QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +        winUtils.loadSheetUsingURIString("chrome://browser/content/extension.css", winUtils.AGENT_SHEET);

It would be nice to use `addSheet` rather than `loadSheet*` for this, so we don't force a synchronous load every time.

It would also be good to do this as early as possible in the load cycle. Definitely before the load event.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

(In reply to Kris Maglione [:kmag] from comment #11)
> > +        winUtils.loadSheetUsingURIString("chrome://browser/content/extension.css", winUtils.AGENT_SHEET);
> It would be nice to use `addSheet` rather than `loadSheet*` for this, so we
> don't force a synchronous load every time.

Fixed!

> It would also be good to do this as early as possible in the load cycle.
> Definitely before the load event.

I tried putting the code earlier, but this was the first point it seemed to work.  :(
Was there any particular event you were thinking of that I should add a handler for?

(Also, thanks for the pre-review!  :D  )

Comment 13

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> > It would also be good to do this as early as possible in the load cycle.
> > Definitely before the load event.
> 
> I tried putting the code earlier, but this was the first point it seemed to
> work.  :(
> Was there any particular event you were thinking of that I should add a
> handler for?

I think DOMWindowCreated should work.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> > Was there any particular event you were thinking of that I should add a
> > handler for?
> I think DOMWindowCreated should work.

Fixed!  :)

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: MozReview Request: Fix build errors, again.

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

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8725725 [details]
MozReview Request: Fix build errors, again.

https://reviewboard.mozilla.org/r/37607/#review34191

Looks good to me, aside from a couple of nits.

I think we also need tests for this, though. It should probably be enough to just add a test that opens a panel and checks the computed style of one of the elements that the stylesheet changes.

::: browser/components/extensions/ext-utils.js:185
(Diff revision 1)
> +          .QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

Please use the same indentation as the `.getService` continuation above, or line the '.' up with the one on the previous line.

::: browser/components/extensions/ext-utils.js:186
(Diff revision 1)
> +        let styleSheetURI = Services.io.newURI("chrome://browser/content/extension.css",

Please either line all arguments up with the opening `(`, or move them all to the next line.

::: browser/components/extensions/ext-utils.js:188
(Diff revision 1)
> +        let styleSheet = styleSheetService.preloadSheet(styleSheetURI,

We need to cache this result, or we still wind up synchronously loading the sheet every time a popup opens.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8725725 [details]
MozReview Request: Fix build errors, again.

(Redirecting to Markus, who might have a little more time to review stuff…)

Comment 18

[Stephen Horlander [:shorlander]]

https://reviewboard.mozilla.org/r/37607/#review35969

Looks pretty good, found a few probems. Attaching an image.

Comment 19

[Stephen Horlander [:shorlander]]

Attachment: WebExtensions - Review Comments - 01

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Okay, I'll fix those up in the next patch (to be submitted on Monday)…  Thanks!  :D

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8725725 [details]
MozReview Request: Fix build errors, again.

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

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Attachment: MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

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

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8725725 [details]
MozReview Request: Fix build errors, again.

(Whoops, ignore this unless you're building on Mac OS X with autoconf213 from homebrew…)

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

Shorlander: Preview at https://www.dropbox.com/s/fvevrsm8tzhky2y/Screenshot%202016-03-14%2011.27.42.png?dl=0

Kris: For some reason, despite the changes to scriptPage, the dump on line 21 prints out "<head><meta charset="utf-8"><script src="popup-a.js"></script></head>", and no body element.  If I remove the meta tag, that gets reflected in the dump, but still no body element.  Any idea what's going on there?  Or any other suggestions for how I could go about testing this?

Thanks!

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

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

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Okay, this seemed like a slightly hacky way to test the styles, but it works, and the flow of all the events was kinda tricky to follow, so this seemed like the simplest change…

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

https://reviewboard.mozilla.org/r/39771/#review36545

Looks good, aside from a few nits. Thanks!

::: browser/components/extensions/ext-utils.js:150
(Diff revision 2)
> +  let styleSheetURI = Services.io.newURI("chrome://browser/content/extension.css",
> +                                         null, null);
> +  let styleSheet = styleSheetService.preloadSheet(styleSheetURI,
> +      styleSheetService.AGENT_SHEET);
> +  styleSheetURI = Services.io.newURI("chrome://browser/content/extension-mac.css",
> +      null, null);

Please align with opening paren. Same for other calls.

::: browser/components/extensions/ext-utils.js:151
(Diff revision 2)
> +                                         null, null);
> +  let styleSheet = styleSheetService.preloadSheet(styleSheetURI,
> +      styleSheetService.AGENT_SHEET);
> +  styleSheetURI = Services.io.newURI("chrome://browser/content/extension-mac.css",
> +      null, null);
> +  let macStyleSheet = styleSheetService.preloadSheet(styleSheetURI,

I don't think we need to load this if we're not on OS-X.

::: browser/components/extensions/ext-utils.js:152
(Diff revision 2)
> +  let styleSheet = styleSheetService.preloadSheet(styleSheetURI,
> +      styleSheetService.AGENT_SHEET);
> +  styleSheetURI = Services.io.newURI("chrome://browser/content/extension-mac.css",
> +      null, null);
> +  let macStyleSheet = styleSheetService.preloadSheet(styleSheetURI,
> +      styleSheetService.AGENT_SHEET);

Maybe just return an array, and append this one to it if we're on OS-X, rather than checking at load-time?

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:21
(Diff revision 2)
>      },
>  
>      files: {
>        "popup-a.html": scriptPage("popup-a.js"),
>        "popup-a.js": function() {
> +        setTimeout(() => {

Use a "load" event listener instead.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:22
(Diff revision 2)
>  
>      files: {
>        "popup-a.html": scriptPage("popup-a.js"),
>        "popup-a.js": function() {
> +        setTimeout(() => {
> +          if (window.getComputedStyle(document.body).getPropertyValue("background-color") == "rgb(252, 252, 252)") {

`window.getComputedStyle(document.body).backgroundColor` works too.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:25
(Diff revision 2)
>        "popup-a.js": function() {
> +        setTimeout(() => {
> +          if (window.getComputedStyle(document.body).getPropertyValue("background-color") == "rgb(252, 252, 252)") {
> -        browser.runtime.sendMessage("from-popup-a");
> +            browser.runtime.sendMessage("from-popup-a");
> +          } else {
> +            browser.runtime.sendMessage("popup-a-failed-style-check");

We need to handle this message, and fail if we get it.

Comment 28

[Blake Winton (:bwinton) (:☕️)]

(In reply to Kris Maglione [:kmag] from comment #27)
> > +  styleSheetURI = Services.io.newURI("chrome://browser/content/extension-mac.css",
> > +      null, null);
> Please align with opening paren. Same for other calls.

Fixed.  (I think it's kinda ugly, but I changed it anyways.  ;)

> ::: browser/components/extensions/ext-utils.js:151
> > +  let macStyleSheet = styleSheetService.preloadSheet(styleSheetURI,
> I don't think we need to load this if we're not on OS-X.
> > +  let macStyleSheet = styleSheetService.preloadSheet(styleSheetURI,
> > +      styleSheetService.AGENT_SHEET);
> Maybe just return an array, and append this one to it if we're on OS-X,
> rather than checking at load-time?

Fixed!  (Good idea!)

> > +        setTimeout(() => {
> Use a "load" event listener instead.

Fixed!

> > +          if (window.getComputedStyle(document.body).getPropertyValue("background-color") == "rgb(252, 252, 252)") {
> `window.getComputedStyle(document.body).backgroundColor` works too.

Fixed!

> > +            browser.runtime.sendMessage("popup-a-failed-style-check");
> We need to handle this message, and fail if we get it.

So, I could do that, but we'll be expecting a popup at https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_browserAction_popup.js#77 and so that'll already fail, and if that didn't catch it for some reason, then then else at https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_browserAction_popup.js#80 would catch it and fail…  Did you still want me to add it?

Comment 29

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

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

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

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

Comment 31

[Stephen Horlander [:shorlander]]

Comment on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

LGTM!

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f41a072c0ca

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f41a072c0ca

Comment 34

[Vasilica Tamas, QA [:vtamas]]

Could you please provide me the webextension that appears in screenshots in order to manually verify this bug?

Comment 35

[Blake Winton (:bwinton) (:☕️)]

Yes, sorry about that!  I believe it's https://www.dropbox.com/s/kw8zz32feop4r17/ui-playground.xpi?dl=0

Comment 36

[Vasilica Tamas, QA [:vtamas]]

Thanks Blake!
I was able to reproduce this issue on Firefox 45.0a1 (2015-11-17) under Ubuntu 14.04 32-bit.

Verified on Firefox 48.0a1 (2016-03-28) and the panel looks as expected under Mac OS X 10.11.2, but I noticed a scrollbar at the bottom of the panel using Windows 10 64-bit and Ubuntu 14.04 32-bit:   http://i.imgur.com/6YtUIsr.jpg ,  http://i.imgur.com/wKsKhLi.png

Should I file a new bug for this issue?

Comment 37

[Blake Winton (:bwinton) (:☕️)]

Yeah, that sounds buggable.  ;)
Thanks!

Comment 38

[Vasilica Tamas, QA [:vtamas]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #37)
> Yeah, that sounds buggable.  ;)
> Thanks!

Filed Bug 1260727.
I am marking this bug as Verified since the other issue is tracked separately.