1404724 - remove browser_style warnings

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Dietrich Ayala (:dietrich)]

I see this warning when I add a browser action with a popup:

addons.webextension.6dd02b7859466bc02ac7b8198e78936697df8a8d@temporary-addon	WARN	Please specify whether you want browser_style or not in your browser_action options.

It shouldn't warn by default. The default should be totes fine unless it causes an error state (in which case it should be an error, not a warning).

If the field isn't required, then the warning should either be removed. If it is required, the warning should be an error instead.

Comment 1

[Kris Maglione [:kmag]]

Blake asked for this to encourage developers to opt into browser styling. Although I agree it's annoying, and think we should only show the warning at install time (or the first time a popup opens after install), rather than nagging every time a popup opens.

Comment 2

[Dietrich Ayala (:dietrich)]

Hrm, while I understand the desire, this is not an effective pattern for developer behavior modification, just bad developer UX.

Nothing in the warning tells the developer why they might want to do it. It just makes it appear as if they did something wrong.

Documenting the hell out of it would be better. On MDN, it's not mentioned at all in the main browser action doc, and is only mentioned innocuously in the popup doc - no explanation of why a developer would want to choose it.

Making the warning happen on install is only slightly less bad, because if your dev workflow is using web-ext, you'll still see it every time.

Blake, can we remove this? I recommend that instead you add something to MDN that explains *why* developers might want to opt-in to browser default styling.

Comment 3

[Dietrich Ayala (:dietrich)]

Alternatively, if it actually *is* so important that we need to constantly remind developers about it forever, even after they've chosen not to enable it, it should actually just be on by default. And then developers can choose to enable it as they like.

Comment 4

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

Did I ask for it? I thought that was more something Kris wanted…

I'm happy removing the warning, and adding more documentation to MDN would also be nice.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Given that we document it defaulting to false we should not be warning about not including it.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1404724 - remove browser_style warning, fix default values,

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

mconca: 

browser_style defaulted to false for page and browserAction.  It defaulted to true for options ui (in about:addons) and the sidebar action.

The patch I'm attaching changes the default to false in all cases.

Verifying again, before landing, that this is what we want.  It will be a change for some extensions.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1404724 - remove browser_style warning, fix default values,

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

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8980760 [details]
Bug 1404724 - remove browser_style warning, fix default values,

https://reviewboard.mozilla.org/r/246930/#review253082

Changing the default without any notice to extension developers does not sound like the right thing.  Clearing the review flag for now until we get an opinion from product.

Comment 10

[Mike Conca [:mconca]]

I don't think we should change the default behavior of browser_style without giving developers some notice. Let's land the removal of the warning here, and file a separate bug to track and implement the setting of the default value.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1404724 - remove browser_style warning, fix default values,

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

Comment 12

[Shane Caraveo (:mixedpuppy)]

doc notes:

We need to update (or verify) page/browser/sidebar actions and options ui entries regarding browser_style.  This should note:

page/browser actions default browser_style to false
sidebar/options default browser_style to true

We should also note bug 1383910.  If browser_style is true, then text is not selectable in the UI unless overridden via css (see the bug for details)

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8981628 [details]
Bug 1404724 - remove browser_style warning, fix default values,

https://reviewboard.mozilla.org/r/247752/#review254148

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:464
(Diff revision 1)
>    addon.type = extension.type === "extension" ?
>                 "webextension" : `webextension-${extension.type}`;
>    addon.strictCompatibility = true;
>    addon.internalName = null;
>    addon.updateURL = bss.update_url;
>    addon.optionsBrowserStyle = true;

can remove this line now

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8981628 [details]
Bug 1404724 - remove browser_style warning, fix default values,

https://reviewboard.mozilla.org/r/247752/#review254148

> can remove this line now

I prefer to leave it since it is set/used in various other places.

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8981628 [details]
Bug 1404724 - remove browser_style warning, fix default values,

https://reviewboard.mozilla.org/r/247752/#review254148

> I prefer to leave it since it is set/used in various other places.

Ah, I read too quickly and didn't notice that the code below that sets the same property is inside a conditional block.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f5232092405
remove browser_style warning, fix default values, r=aswan

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7f5232092405

Comment 18

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 19

[Irene Smith]

In cases where the value defaults to false, the setting description includes:

Although this key defaults to false, it's recommended that you include it and set it to true in order to make your popups consistent with the look of the rest of the browser user interface.

Also, the existing documentation has an article about Browser Styles that includes this paragraph:

One of the challenges with this approach is styling the element in such a way that it fits in with the browser's own style. To help with this, the manifest.json keys include an extra optional property: browser_style. If this is included and set to true, then your document will get one or more extra stylesheets that will help make it look consistent with the browser's UI and with other extensions that use the browser_style property.

Do you feel that there should be more information somewhere or is this enough?

Comment 20

[Shane Caraveo (:mixedpuppy)]

browser style is quite broken in several ways, IMO we should not be recommending it, and if we do, the developer should at least be sure to test with multiple themes (built-in or from amo) to ensure the extension UI behaves in a way they understand.  We have bugs to address browser style, at which point maybe we can recommend it.

Comment 21

[Shane Caraveo (:mixedpuppy)]

And the other information is in comment 12, which is that browser_style is not always false by default.  The sidebar and the options (ie. extension options in about:addons) default this to true.  One primary gotcha that is not documented with this, text selection is disabled with browser_style=true.

Comment 22

[Irene Smith]

The default values are documented, but the thing about text selection being disabled seems not to be. To what does that refer? Text selection in the browser, in the extension? Also, the documentation already recommended setting browser_style to true. Should be add some sort of note suggesting that, for now, the property be set to false because there are some bugs?

Not picking nits here, but this sounds like a complicated issue and I want to get it right.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Yeah, sorry, it is a bit convoluted.  There are issues with it, so I think we should change the recommendation to include: if extensions want to use it, they should test against the various themes, etc. to ensure that the panel behaves as expected.

The text selection issue is in the extension panel if browser_style is true.  So by default, you cannot select text in an extension sidebar unless they override the css or set browser_style to false.  That is bug 1383910, and the workaround is in bug 1383910 comment 7.  The documentation for browser_style should state that text selection is disabled when true as that matches how the UI in the browser works.  Then include the work around.

There are other problems you can see in the list of bugs on bug 1458678

Comment 24

[Irene Smith]

(In reply to Shane Caraveo (:mixedpuppy) from comment #23)

I think I will create a related documentation bug for this. I think concerns about how all this works deserves to be its own documentation bug.

Comment 26

[Irene Smith]

I did create a separate issue, but I wanted to update this issue with the information I added to Browser Styles:

When considering using browser_style: true, you need to test your extension with various themes (built-in or from AMO) to make sure that the extension UI behaves the way you expect it to.

When browser_style: true is included in your web extension's manifest, text selection in your extension's UI is disabled except in input controls. If this will cause a problem, include browser_style:false instead.

Comment 27

[Shane Caraveo (:mixedpuppy)]

LGTM