Closed Bug 1404724 Opened 2 years ago Closed Last year

remove browser_style warnings

Categories

(WebExtensions :: Frontend, enhancement, P5)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: dietrich, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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.
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.
Component: WebExtensions: General → WebExtensions: Frontend
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.
Flags: needinfo?(bwinton)
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.
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.
Flags: needinfo?(bwinton)
Priority: -- → P5
Blocks: 1458678
Given that we document it defaulting to false we should not be warning about not including it.
Summary: browser action with popup warns about browser_style by default → remove browser_style warnings
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.
Flags: needinfo?(mconca)
Attachment #8980759 - Attachment is obsolete: true
Attachment #8980759 - Flags: review?(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.
Attachment #8980760 - Flags: review?(aswan)
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.
Flags: needinfo?(mconca)
Attachment #8980760 - Attachment is obsolete: true
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)
Keywords: dev-doc-needed
Assignee: nobody → mixedpuppy
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
Attachment #8981628 - Flags: review?(aswan) → review+
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 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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7f5232092405
remove browser_style warning, fix default values, r=aswan
https://hg.mozilla.org/mozilla-central/rev/7f5232092405
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Product: Toolkit → WebExtensions
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?
Flags: needinfo?(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.
Flags: needinfo?(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.
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.
Flags: needinfo?(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
Flags: needinfo?(mixedpuppy)
(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.
Duplicate of this bug: 1444408

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.

Flags: needinfo?(mixedpuppy)

LGTM

Flags: needinfo?(mixedpuppy)
You need to log in before you can comment on or make changes to this bug.