Closed Bug 1225633 Opened 5 years ago Closed 5 years ago

Better default styles for WebExtension popups.

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 verified)

VERIFIED FIXED
mozilla48
Tracking Status
firefox48 --- verified

People

(Reporter: bwinton, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webextension-polish][browserAction][pageAction])

Attachments

(3 files, 3 obsolete files)

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.)
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?
Flags: needinfo?(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…  :)
Flags: needinfo?(bwinton)
Ah, so you want to style the contents of the browser?

Interesting...

OK. I guess I'll let you worry about it then. Thanks :)
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.  ;)
Yeah, fair enough. Our UA stylesheets are already different enough from Chrome's, in any case.
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
Assignee: nobody → bwinton
Attached patch Work in progress… (obsolete) — Splinter Review
Any suggestions on how to make it better would be very welcome!
Comment on attachment 8722133 [details] [diff] [review]
Work in progress…

(Looping in shorlander for feedback…)
Attachment #8722133 - Flags: feedback?(shorlander)
Attached patch Better wip patch. (obsolete) — Splinter Review
Attachment #8722133 - Attachment is obsolete: true
Attachment #8722133 - Flags: feedback?(shorlander)
Attachment #8722676 - Flags: feedback?(shorlander)
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 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.
(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  )
(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.
(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!  :)
Attachment #8722676 - Flags: feedback?(shorlander)
Attachment #8722676 - Attachment is obsolete: true
Attachment #8725725 - Flags: ui-review?(shorlander)
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.
Attachment #8725725 - Flags: review?(kmaglione+bmo)
Comment on attachment 8725725 [details]
MozReview Request: Fix build errors, again.

(Redirecting to Markus, who might have a little more time to review stuff…)
Attachment #8725725 - Flags: ui-review?(shorlander) → ui-review?(mjaritz)
Okay, I'll fix those up in the next patch (to be submitted on Monday)…  Thanks!  :D
Flags: needinfo?(bwinton)
Attachment #8725725 - Attachment description: MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag → MozReview Request: Fix build errors, again.
Attachment #8725725 - Flags: review?(kmaglione+bmo)
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 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…)
Attachment #8725725 - Attachment is obsolete: true
Attachment #8725725 - Flags: ui-review?(mjaritz)
Attachment #8725725 - Flags: review?(kmaglione+bmo)
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!
Attachment #8730243 - Flags: ui-review?(shorlander)
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/
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 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.
Attachment #8730243 - Flags: review?(kmaglione+bmo) → review+
(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 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 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 on attachment 8730243 [details]
MozReview Request: Bug 1225633 - Add better default styles to WebExtension panels. ui-r=shorlander, r=kmag

LGTM!
Attachment #8730243 - Flags: ui-review?(shorlander) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/4f41a072c0ca
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Could you please provide me the webextension that appears in screenshots in order to manually verify this bug?
Flags: needinfo?(bwinton)
Yes, sorry about that!  I believe it's https://www.dropbox.com/s/kw8zz32feop4r17/ui-playground.xpi?dl=0
Flags: needinfo?(bwinton)
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?
Flags: needinfo?(bwinton)
Yeah, that sounds buggable.  ;)
Thanks!
Flags: needinfo?(bwinton)
Depends on: 1260727
(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.
Status: RESOLVED → VERIFIED
Blocks: 1263709
Depends on: 1269081
Depends on: 1269334
Depends on: 1269336
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.