Implement `chrome_style` in options V2 API.

RESOLVED FIXED in Firefox 55

Status

P2
normal
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: bwinton, Assigned: mattw)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

unspecified
mozilla55
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

Attachments

(2 attachments, 1 obsolete attachment)

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

3 years 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)

Updated

3 years ago
Attachment #8756592 - Flags: ui-review?(shorlander)
(Reporter)

Updated

3 years ago
Keywords: dev-doc-needed

Updated

3 years 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

3 years 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

3 years 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

Comment 9

3 years ago
(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?
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

3 years ago
Assignee: bwinton → nobody
Status: ASSIGNED → NEW

Updated

3 years ago
Assignee: nobody → mwein
webextensions: + → ---
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3

Updated

2 years 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)
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)
(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)
(Assignee)

Updated

2 years ago
Attachment #8756592 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Priority: P3 → P2
Comment hidden (mozreview-request)

Comment 16

2 years 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

2 years 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

2 years ago
Attachment #8839774 - Flags: ui-review?(shorlander)
(Assignee)

Comment 19

2 years 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

2 years ago
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.
Comment hidden (mozreview-request)

Comment 25

2 years 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

2 years ago
Keywords: checkin-needed

Comment 27

2 years 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
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)

Updated

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

Comment 32

2 years 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

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

Updated

2 years ago
Depends on: 1354336

Comment 34

2 years 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)
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

2 years ago
lgtm, thanks
Flags: needinfo?(amckay)
Keywords: dev-doc-needed → dev-doc-complete

Comment 38

2 years ago
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/

Comment 39

2 years ago
(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 years 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 years ago
We should also default "browser_style" to false then. I will add that as part of bug 1354336.

Updated

a year ago
See Also: → bug 1430349

Updated

9 months ago
Product: Toolkit → WebExtensions
Depends on: 1466418
You need to log in before you can comment on or make changes to this bug.