Implement `chrome_style` in options V2 API.

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P2
normal
RESOLVED FIXED
a year ago
15 days ago

People

(Reporter: bwinton, Assigned: mattw)

Tracking

({dev-doc-complete})

unspecified
mozilla55
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
So, I think we want to keep the name "chrome_style", since by setting that, the add-on author has indicated that they want their settings to look like they fit in with the browser chrome, and I feel we should honour that preference.

I wouldn't be averse to adding "browser_style" as well, for people who want to look good in Firefox, but get the default styles in Chrome, if that's something you think add-on authors would be interested in…
I'd rather we name it "browser_style" and have it default to `true`.
(Reporter)

Comment 2

a year ago
Even after the problems we ran into with popups?

(I suggest implementing "chrome_style" because I'm interested in getting exiting add-ons looking nice in Firefox.  I have no problems with also implementing "browser_style", but after the popup stuff, I don't think defaulting to "true" is the best idea…  Perhaps a warning if it's not specified, like the browser_action/page_action's "browser_style"?)
I think we're probably less likely to run into problems here. Either way, I'd rather default to true and wait to see if there are problems than assume there will be.

But we can still emit a warning if it's not supplied so people have some idea of what to do if they run into trouble.
(Reporter)

Comment 4

a year ago
Created attachment 8756592 [details]
MozReview Request: Bug 1275287 - Use better styling for the options pages.  ui-r=shorlander, r=kmag.

Review commit: https://reviewboard.mozilla.org/r/55270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55270/
Attachment #8756592 - Flags: review?(kmaglione+bmo)
(Reporter)

Updated

a year ago
Attachment #8756592 - Flags: ui-review?(shorlander)
(Reporter)

Updated

a year ago
Keywords: dev-doc-needed

Updated

a year ago
webextensions: --- → +
Whiteboard: triaged
Comment on attachment 8756592 [details]
MozReview Request: Bug 1275287 - Use better styling for the options pages.  ui-r=shorlander, r=kmag.

https://reviewboard.mozilla.org/r/55270/#review52068

::: toolkit/mozapps/extensions/content/extensions.js:82
(Diff revision 1)
>  XPCOMUtils.defineLazyGetter(gStrings, "appVersion", function() {
>    return Services.appinfo.version;
>  });
>  
> +var global = {};
> +XPCOMUtils.defineLazyGetter(global, "stylesheets", () => {

Please use `this` rather than `global`. And we should use a more specific name, like `inlineOptionsStylesheets`.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:907
(Diff revision 1)
>    addon.internalName = null;
>    addon.updateURL = bss.update_url;
>    addon.updateKey = null;
>    addon.optionsURL = null;
>    addon.optionsType = null;
> +  addon.optionsBrowserStyle = true;

I'm a bit worried about adding this to the add-ons DB before bug 1272446 is fixed. It might be safer to just add an observer and inject the stylesheets from webextension code.

Also, this name is a bit confusing. Maybe something like `injectOptionsCSS`?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:919
(Diff revision 1)
>      else
>        addon.optionsType = AddonManager.OPTIONS_TYPE_INLINE_BROWSER;
> +
> +    if (manifest.options_ui.browser_style === null)
> +      logger.warn("Please specify whether you want browser_style " +
> +                                 "or not in your options_ui options.");

This check/warning should really happen on the webextensions side. Ideally in the schema, but a fallback in Extension.jsm would be fine.

::: toolkit/themes/shared/extensions/browser-style.css:1
(Diff revision 1)
> +/* stylelint-disable property-no-vendor-prefix */

Can you share a screenshot of this?

::: toolkit/themes/shared/mozapps.inc.mn:23
(Diff revision 1)
>    skin/classic/mozapps/plugins/pluginProblem.css             (../../shared/plugins/pluginProblem.css)
>    skin/classic/mozapps/aboutNetworking.css                   (../../shared/aboutNetworking.css)
>  #ifndef ANDROID
>    skin/classic/mozapps/aboutProfiles.css                     (../../shared/aboutProfiles.css)
> +  skin/classic/mozapps/extensions/browser-style.css                     (../../shared/extensions/browser-style.css)
> +  skin/classic/mozapps/extensions/browser-style-mac.css                 (../../shared/extensions/browser-style-mac.css)

Please remove extra whitespace.
Attachment #8756592 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 6

a year ago
https://reviewboard.mozilla.org/r/55270/#review52068

> Please use `this` rather than `global`. And we should use a more specific name, like `inlineOptionsStylesheets`.

Switched to this.inlineOptionsStylesheet.

> I'm a bit worried about adding this to the add-ons DB before bug 1272446 is fixed. It might be safer to just add an observer and inject the stylesheets from webextension code.
> 
> Also, this name is a bit confusing. Maybe something like `injectOptionsCSS`?

Yeah, I thought it would mirror optionsType and optionsURL, but I've changed it to injectOptionsCSS.
Should we get this bug to block on bug 1272446?  Do we think that will be fixed in time?
(If not, can you give me a hint about where the observer should be added?)

> This check/warning should really happen on the webextensions side. Ideally in the schema, but a fallback in Extension.jsm would be fine.

So, I've put it in the schema, but can I choose the message, or do we feel the default message of `Reading manifest: Error processing options_ui: Property "browser_style" is required` is fine?

> Please remove extra whitespace.

So, I ended up switching to use chrome://mozapps/skin/extensions/extensions.css (the default add-on manager stylesheet), so that the preferences look more like the rest of the page, and so this code is gone.  :)
https://reviewboard.mozilla.org/r/55270/#review52068

> Yeah, I thought it would mirror optionsType and optionsURL, but I've changed it to injectOptionsCSS.
> Should we get this bug to block on bug 1272446?  Do we think that will be fixed in time?
> (If not, can you give me a hint about where the observer should be added?)

Once bug 1276025 lands, you can probably do something using the code in Extension.jsm that detects inline options browsers.

> So, I've put it in the schema, but can I choose the message, or do we feel the default message of `Reading manifest: Error processing options_ui: Property "browser_style" is required` is fine?

Yeah, I think it's fine.
Depends on: 1276025
(Reporter)

Comment 8

a year ago
So, I've been playing around with this a bit, but I think we really need bug 418833 fixed before we can unleash this on unsuspecting add-on authors…  :(
Depends on: 418833
(In reply to Blake Winton (:bwinton) (:☕️) (PTO 'til London.  Find me there for quick answers!) from comment #8)
> So, I've been playing around with this a bit, but I think we really need bug
> 418833 fixed before we can unleash this on unsuspecting add-on authors…  :(

That bug has been around a long time, do you think its likely change?
(Reporter)

Comment 10

a year ago
I have hope, now that it's blocking something more important than WebCompat, that maybe someone will pick it up…  (Although any advocacy efforts you could lend would be much appreciated. :)

Updated

a year ago
Assignee: bwinton → nobody
Status: ASSIGNED → NEW

Updated

10 months ago
Assignee: nobody → mwein
webextensions: + → ---
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3

Updated

8 months ago
webextensions: --- → ?
(In reply to Blake Winton (:bwinton) (:☕️) from comment #8)
> So, I've been playing around with this a bit, but I think we really need bug
> 418833 fixed before we can unleash this on unsuspecting add-on authors…  :(

Now that bug 418833 is fixed would it be safe to default this to true?
Flags: needinfo?(bwinton)
(Reporter)

Comment 12

6 months ago
Once we make better in-content stylesheets that don't hide radio buttons and replace them with images, yes.  :)
Flags: needinfo?(bwinton)
(In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> Once we make better in-content stylesheets that don't hide radio buttons and
> replace them with images, yes.  :)

Can we make this depends on that? ;)

So I am a little confused about the ui-review on this is for?
Flags: needinfo?(bwinton)
(Reporter)

Comment 14

6 months ago
(In reply to Stephen Horlander [:shorlander] from comment #13)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> > Once we make better in-content stylesheets that don't hide radio buttons and
> > replace them with images, yes.  :)
> Can we make this depends on that? ;)

Sure!  I believe Matt is working on that.  Or at least talking to people who might work on that?

> So I am a little confused about the ui-review on this is for?

Given the date of the patch, feel free to clear that out.  I'm hoping it's obsolete now, and Matt will post a better one.
Flags: needinfo?(bwinton)
Attachment #8756592 - Flags: ui-review?(shorlander)
(Assignee)

Updated

6 months ago
Attachment #8756592 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Priority: P3 → P2
Comment hidden (mozreview-request)

Comment 16

6 months ago
mozreview-review
Comment on attachment 8839774 [details]
Bug 1275287 - Use the same browser_style styling for options that we use for popups

https://reviewboard.mozilla.org/r/114340/#review116052

Is there any test for this?

::: toolkit/mozapps/extensions/content/extensions.js:3647
(Diff revision 1)
>        mm.addMessageListener("Extension:BrowserResized", messageListener);
> -      mm.sendAsyncMessage("Extension:InitBrowser", {fixedWidth: true});
>  
> +      let browserOptions = {
> +        fixedWidth: true,
> +        inline: true,

Shouldn't this be isInline?
(Assignee)

Comment 17

6 months ago
mozreview-review-reply
Comment on attachment 8839774 [details]
Bug 1275287 - Use the same browser_style styling for options that we use for popups

https://reviewboard.mozilla.org/r/114340/#review116052

Not at the moment. What sort of tests do you think we should have for this?

I'll also request a ui-review for this from shorlander.

> Shouldn't this be isInline?

Oops, yes, thank you
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8839774 - Flags: ui-review?(shorlander)
(Assignee)

Comment 19

6 months ago
shorlander, I've never requested a ui-review before, so please let me know what sort of information you need from me in order to be able to review this and I'll do my best to provide it for you.
(In reply to Matthew Wein [:mattw] from comment #17)

> Not at the moment. What sort of tests do you think we should have for this?

You should be able to load the page that shows the inline browser, then check that the stylesheets were applied.  Maybe checking for a style on an element in the frame would work, similar to this:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_browserAction_popup.js#31
(In reply to Matthew Wein [:mattw] from comment #19)
> shorlander, I've never requested a ui-review before, so please let me know
> what sort of information you need from me in order to be able to review this
> and I'll do my best to provide it for you.

Hi! Is there a try-build I could use? What exactly needs review?
(Assignee)

Comment 22

6 months ago
Created attachment 8843331 [details]
example_options_ui-1.0.xpi

The XPI I've attached provides an example of how the options would look for a WebExtension using chrome_style. It includes examples for buttons, input fields, radio groups, checkboxes, and navigation.
Thanks, looks good to me.
Comment hidden (mozreview-request)

Comment 25

5 months ago
mozreview-review
Comment on attachment 8839774 [details]
Bug 1275287 - Use the same browser_style styling for options that we use for popups

https://reviewboard.mozilla.org/r/114340/#review121634

r+ pending my comment on the test.

::: browser/components/extensions/test/browser/browser_ext_optionsPage_browser_style.js:59
(Diff revision 3)
> +      "options.js": optionsScript,
> +    },
> +    background: backgroundScript,
> +  });
> +
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");

This looks unecessary.  Remove or comment.
Attachment #8839774 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 27

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eead99c3e41a
Use the same browser_style styling for options that we use for popups r=mixedpuppy
Keywords: checkin-needed

Comment 28

5 months ago
sorry had to back this out for mochitest failures in browser_ext_optionsPage_browser_style.js like https://treeherder.mozilla.org/logviewer.html#?job_id=83519395&repo=autoland&lineNumber=3970

https://hg.mozilla.org/integration/autoland/rev/6343f0613cb5
Flags: needinfo?(mwein)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56fbfd6577ab22bd50c5c5330b3719b517f0c23a
(Assignee)

Updated

5 months ago
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 32

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bcf6f4ded64
Use the same browser_style styling for options that we use for popups r=mixedpuppy
Keywords: checkin-needed

Comment 33

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4bcf6f4ded64
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Depends on: 1354336

Comment 34

4 months ago
Comment on attachment 8839774 [details]
Bug 1275287 - Use the same browser_style styling for options that we use for popups

This has landed, so removing the shorlander review flag so it won't show up in my queries.
Attachment #8839774 - Flags: ui-review?(shorlander)

Updated

3 months ago
Duplicate of this bug: 1364350
I've updated: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/options_ui
I've also opened a PR to update the compat data: https://github.com/mdn/browser-compat-data/pull/214

Let me know if this covers it.
Flags: needinfo?(amckay)

Comment 37

3 months ago
lgtm, thanks
Flags: needinfo?(amckay)
Keywords: dev-doc-needed → dev-doc-complete
Looks like this change totally broken styling of WebExtensions options’ pages — overall styling looks like if the page was in non-standard (Quirks) rendering mode (though `document.compatMode` returns `CSS1Compat`), and checkboxes are not displayed at all.

Please see the comparison screenshot:

    http://tanalin.com/_experimentz/bugs/fx/2017/options-styling-broken/screenshot.png

The screenshot demonstrates how my SmartUpscale [1] extension’s options page (displayed inline in the properties page of the extension in Firefox) looks in two versions of Firefox Nightly:

    * 2017-03-14-03-02-15-mozilla-central-l10n — not affected;
    * 2017-03-15-03-02-15-mozilla-central-l10n — affected.

Setting `options_ui → browser_style` manifest option to `false` does “fix” the issue, but many WebExtensions-based extensions don’t have the `browser_style` option specified in their manifest and so will certainly be broken by default.

It probably makes sense (if the change makes sense at all in its current state) in terms of compatibility with existing extensions, to make this change opt-IN instead of the current opt-OUT. Thanks.

[1] https://addons.mozilla.org/firefox/addon/smart-upscale/
(In reply to comment #38)
> overall styling looks like if the page was in non-standard (Quirks) rendering mode

I should correct myself: I actually meant that UI of the extension’s options page looks similar to UI of an old Windows application with no support for Windows XP+ visual styles. I have no idea why I mentioned Quirks mode. Anyway, that’s clear from the screenshot. Thanks.

Comment 40

2 months ago
I have noticed that FF55 messes with CSS on the option pages.

There is a disparity with Chrome here.


> chrome_style (boolean) - optional
> If true, a Chrome user agent stylesheet will be applied to your options page. The default value is false, but we recommend you enable it for a consistent UI with Chrome.

The disparity is that in Chrome it defaults to false while in FF it defaults to true thus forcing developers to add "browser_style": false to every manifest.json (for the time being) in order to maintain the applied CSS.
(Assignee)

Comment 41

2 months ago
We should also default "browser_style" to false then. I will add that as part of bug 1354336.
You need to log in before you can comment on or make changes to this bug.