Dietrich Ayala (:dietrich) Reporter
	Description • 7 years ago

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.

	Kris Maglione [:kmag]
	Comment 1 • 7 years ago

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.

	Dietrich Ayala (:dietrich) Reporter
	Comment 2 • 7 years ago

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.

	Dietrich Ayala (:dietrich) Reporter
	Comment 3 • 7 years ago

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.

	Blake Winton (:bwinton) (:☕️)
	Comment 4 • 7 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 5 • 6 years ago

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

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 7 • 6 years ago

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.

	Andrew Swan [:aswan]
	Comment 9 • 6 years ago
mozreview-review

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.

	Mike Conca [:mconca]
	Comment 10 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 12 • 6 years ago

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)

	Andrew Swan [:aswan]
	Comment 13 • 6 years ago
mozreview-review

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

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 14 • 6 years ago
mozreview-review-reply

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.

	Andrew Swan [:aswan]
	Comment 15 • 6 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 16 • 6 years ago

Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7f5232092405
remove browser_style warning, fix default values, r=aswan

	Narcis Beleuzu [:NarcisB]
	Comment 17 • 6 years ago
bugherder

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

	marius.santa
	Comment 18 • 6 years ago

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.

	Irene Smith
	Comment 19 • 6 years ago

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?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 20 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 21 • 6 years ago

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.

	Irene Smith
	Comment 22 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 23 • 6 years ago

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

	Irene Smith
	Comment 24 • 6 years ago

(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.