Closed Bug 1275287 Opened 8 years ago Closed 7 years ago

Implement `chrome_style` in options V2 API.

Categories

(WebExtensions :: Frontend, defect, P2)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
webextensions ?

People

(Reporter: bwinton, Assigned: mattw)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(2 files, 1 obsolete file)

Bug 1275287 - Use the same browser_style styling for options that we use for popups
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
example_options_ui-1.0.xpi
1.56 KB, application/x-xpinstall
Details 
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`.
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.
Attachment #8756592 - Flags: ui-review?(shorlander)
Keywords: dev-doc-needed
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)
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
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?
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. :)
Assignee: bwinton → nobody
Status: ASSIGNED → NEW
Assignee: nobody → mwein
webextensions: + → ---
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
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)
Attachment #8756592 - Flags: ui-review?(shorlander)
Attachment #8756592 - Attachment is obsolete: true
Priority: P3 → P2
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?
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
Attachment #8839774 - Flags: ui-review?(shorlander)
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?
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 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+
Keywords: checkin-needed
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)
Flags: needinfo?(mwein)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/4bcf6f4ded64
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1354336
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)
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)
lgtm, thanks
Flags: needinfo?(amckay)
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.
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.
We should also default "browser_style" to false then. I will add that as part of bug 1354336.
See Also: → 1430349
Product: Toolkit → WebExtensions
Depends on: 1466418
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: