Closed Bug 1460910 Opened 2 years ago Closed 2 years ago

Reconsider opening sidebar on install of an extension

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

See bug 1459007 for context.

Currently, if a WebExtension has a sidebar we always open it on the install of the WebExtension

I think we should reconsider this decision.

In the uBlock case, they simply have a sidebar for debugging/logging, yet it opens up to a normal user.

It's up to the WebExtension to make the sidebar discoverable, it shouldn't be done by Firefox.
Flags: needinfo?(mconca)
The issue is that sidebars can't be opened programmatically, they can only be opened by user action. The rationale behind opening them on install was so that the user understood the extension included a sidebar.  This works and is desired by the vast majority of sidebar extensions.

I guess I view uBlock as the exception to the general case, and making every extension with a sidebar figure out a way to make themselves discoverable seems to penalize nearly every sidebar extension in order to account for the exception (albeit one with a very large install base).

Perhaps an alternative would be to add a manifest key called "installed_open" that defaults to true when not included, but could be set to false for extensions like uBlock.
Flags: needinfo?(mconca) → needinfo?(mozilla)
> The issue is that sidebars can't be opened programmatically, they can only be opened by user action.

I thought we provided a way for an add-on to open a sidebar if a user explicitly chose an action within the add-on, but it looks like we don't.

So we force users to go through our sidebar UI to access any sidebar?

It seems like a better fix would be to allow add-ons to have a way to open the sidebar if there was a user action within the add-on. Then they would be responsible for surfacing their own sidebar.

> Perhaps an alternative would be to add a manifest key called "installed_open" that defaults to true when not included, but could be set to false for extensions like uBlock.

That might work as well...
Flags: needinfo?(mozilla)
> … shouldn't be done by Firefox.

+1

The degree of annoyance when undoing the intrusion in fifteen or windows can be surprisingly high. 

I'm tempted to repeat this text now in comments 4, 
5, 
6, 
7, 
8, 
9, 
10, 
11, 
12, 
13, 
14, 
15, 
16, 
17 
and 18, but I suspect that doing so would bring to mind a word less gentle than "surprising". So I'll
(In reply to Mike Kaply [:mkaply] from comment #2)
> > The issue is that sidebars can't be opened programmatically, they can only be opened by user action.
> 
> I thought we provided a way for an add-on to open a sidebar if a user
> explicitly chose an action within the add-on, but it looks like we don't.

We do.  sidebarAction.open() is available and can be used in response to a user clicking a browserAction, pageAction, context menu item, keyboard shortcut, or a bundled page.

The implication, though, is that every sidebar would need to include one of those items, when it may not make sense. There are a lot of sidebar extensions that are simply sidebars (they show Twitter, Facebook, chat, email, etc.) and want to simply be a sidebar selection inside of Firefox along with bookmarks, history, and sync'd tabs.

To be clear, I'm not against asking sidebar extensions to surface themselves, it's a reasonable request. It just seems like the current default behavior of showing the sidebar on install is what the majority of extensions want. So, to me, the problem is that the extensions that don't want this behavior have no choice, which is why offering an install-time choice to not be shown might be a better solution.
> So, to me, the problem is that the extensions that don't want this behavior have no choice, which is why offering an install-time choice to not be shown might be a better solution.

Makes perfect sense. I'll see if I can write a quick patch.
Assignee: nobody → mozilla
Priority: -- → P3
Comment on attachment 8977043 [details]
Bug 1460910 - Allow sidebar to be closed at install.

https://reviewboard.mozilla.org/r/245142/#review251098

::: browser/components/extensions/schemas/sidebar_action.json:38
(Diff revision 1)
> +              },
> +              "open_at_install": {
> +                "type": "boolean",
> +                "optional": true,
> +                "default": true,
> +                "description": "Whether or not the sidebar is opened at install"

", default is <code>true</code>", or something like that
Attachment #8977043 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8977043 [details]
Bug 1460910 - Allow sidebar to be closed at install.

https://reviewboard.mozilla.org/r/245142/#review251106

::: browser/components/extensions/parent/ext-sidebarAction.js:138
(Diff revision 1)
> -      if (install || SidebarUI.lastOpenedId == this.id) {
> +      if ((install || SidebarUI.lastOpenedId == this.id) &&
> +          this.extension.manifest.sidebar_action.open_at_install) {

This logic doesn't look right, shouldn't it be
`(install && open_at_install) || lastOpened == this`
Comment on attachment 8977043 [details]
Bug 1460910 - Allow sidebar to be closed at install.

https://reviewboard.mozilla.org/r/245142/#review251106

> This logic doesn't look right, shouldn't it be
> `(install && open_at_install) || lastOpened == this`

Yes. I misread that this was completely under ADDON_INSTALL.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/42abedd98732
Allow sidebar to be closed at install. r=mixedpuppy
mconca:

I ended up using open_at_install

Hope you don't mind.
Keywords: dev-doc-needed
(In reply to Mike Kaply [:mkaply] from comment #13)
> I ended up using open_at_install
> 
> Hope you don't mind.

What?!? And miss a prime opportunity to bikeshed the name of an optional keyword?!?  We'll need to carry on a public, back-and-forth discussion over the name for at least another handful of comments before I can be satisfied.

All kidding aside, mkaply, great job landing this. It'll be a nice enhancement for developers.
Backed out for failing browser chrome at  browser/components/extensions/test/browser/test-oop-extensions/browser_ext_sidebarAction.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=42abedd987321bd8d3cc9d531c575e31e4283f1c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179239582&repo=autoland&lineNumber=2829

Backout: https://hg.mozilla.org/integration/autoland/rev/17efe13d07fafbf1539f86f2046c946b67f3a787
Flags: needinfo?(mozilla)
I am curious why it opens it for every single browser window when installed. A reasonable thing would be to open the sidebar if the extension wants it displayed on install in the window that initiated the addon installation.
So addressing aswans last comment broke this (and I failed to retest).

The reason is because of the code here:

https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-sidebarAction.js#138

SidebarUI.lastOpenedId == this.id

Because this test is run repeatedly with the same extension, this value is true.

I'm wondering if we shoudl do something with lastOpenedId when an extension is installed.

Alternative is to use a different extensionID for the test.
Flags: needinfo?(mozilla)
Flags: needinfo?(mixedpuppy)
I've updated the patch to cover this case. During shutdown, if lastOpenedId is our ID, we should be resetting it.
(In reply to Mike Kaply [:mkaply] from comment #19)
> During shutdown, if lastOpenedId
> is our ID, we should be resetting it.

That doesn't sound right.  Isn't the point of that logic to restore the sidebar across browser sessions?
Did you mean "during uninstall" instead of "during shutdown"?
> Isn't the point of that logic to restore the sidebar across browser sessions?
Did you mean "during uninstall" instead of "during shutdown"?

I thought that at first too, but I see no place where lastOpenedId is preserved across shutdowns.

And during on Shutdown, we close our own sidebar.

I honestly don't understand the point of lastOpenedId, and this comment in browser-sidebar makes it even less clear:

https://searchfox.org/mozilla-central/source/browser/base/content/browser-sidebar.js#33

  // lastOpenedId is set in show() but unlike currentID it's not cleared out on hide
  // and isn't persisted across windows
mixedpuppy: aswan, mkaply: looks like it is fine to null lastOpenedId if the extension is being shutdown
mixedpuppy: lastOpenedId is used only to support SidebarUI.toggle
mixedpuppy: it is set during show to be the broadcaster id
mixedpuppy: the persistence (to reopen an open sidebar on startup) is handled in xul

additional note: lastOpenedId is also set on startup, based on the broadcaster id persisted via xul.
Flags: needinfo?(mixedpuppy)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/490a791ce6bd
Allow sidebar to be closed at install. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/490a791ce6bd
Status: NEW → RESOLVED
Closed: 2 years ago
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?(mozilla)
I don't think so. We have an automated test for this, so we should be good.
Flags: needinfo?(mozilla) → qe-verify-
Product: Toolkit → WebExtensions
Added open_at_install. The optional parameter has been added here:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/sidebar_action

and to the release notes.
Flags: needinfo?(mozilla)
Content looks good. Formatting is off though. It's not using a fixed width font.
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.