1247045 - system addon widgets are not "default" in CUI

Status: NEW

(Firefox :: Toolbars and Customization, defect)

Description

[Shane Caraveo (:mixedpuppy)]

As a system addon, it should be placed into the default position defined by the widget.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Because creating a widget via api does not make it a "default" button in CUI, a couple issues happen:

- customization reset moves the button to the palette
- the customization reset button is enabled first time you enter customization mode because the pocket button is not default, but present in the UI.

Comment 3

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Hi Gijs, from IRC 

> Standard8: basically, I would propose to just fix the CustomizableUI API and not do any system add-on checks, as system add-ons are going to be the only non-webextension add-ons soon
>but it sounds like that would require rewriting the add-on code to not use webextensions to add the button

Can you add any more detail here (or should we file another bug)?

Comment 4

[:Gijs (he/him)]

(In reply to Cory Price [:ckprice] from comment #3)
> Hi Gijs, from IRC 
> 
> > Standard8: basically, I would propose to just fix the CustomizableUI API and not do any system add-on checks, as system add-ons are going to be the only non-webextension add-ons soon
> >but it sounds like that would require rewriting the add-on code to not use webextensions to add the button
> 
> Can you add any more detail here (or should we file another bug)?

Well, the basic idea is, I think, that soon we control all the direct callers of CustomizableUI, so we can just allow them to say they should modify the default state.

If we also want to do this from system add-ons using webextensions, we can add system add-on checks to the webextension layer that either provide a different API or allow magical options to existing APIs that trigger the same code in CustomizableUI.

I think given the limited impact it might make sense to just liberalize the CUI API today, and do the webextensions stuff in a separate followup if/where necessary.

If we want to use this with pageshot on an ASAP basis, we should probably either prioritize that webextensions work or this:

> >but it sounds like that would require rewriting the add-on code to not use webextensions to add the button

ie update pageshot to use the non-webextension way of adding its toolbar button.

Comment 5

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to :Gijs from comment #4)
> If we want to use this with pageshot on an ASAP basis, we should probably
> either prioritize that webextensions work or this:
We do want to use screenshots ASAP (targeting release in 54, would like to be in Beta now).

> 
> > but it sounds like that would require rewriting the add-on code to not use webextensions to add the button
> 
> ie update pageshot to use the non-webextension way of adding its toolbar
> button.

NI ian - what is the LOE here?

Comment 6

[Ian Bicking (:ianbicking)]

Changing the button to be implemented in bootstrap.js and not in the WebExtension would be a substantial change; in theory bootstrap can't initiate communication with the WebExtension (maybe there's a hacky way to do this, I'm not sure), and we manage the button state in the WebExtension.

Comment 7

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Gijs: what is involved in making the work in comment 4 a priority? IIUC this is not constrained just to Screenshots, but any system add-on which is a web extension.

Comment 8

[:Gijs (he/him)]

(In reply to Cory Price [:ckprice] from comment #7)
> Gijs: what is involved in making the work in comment 4 a priority?

I don't know. I'm not on the webextensions team and can't speak for their availability. You could ask Andy McKay.

> IIUC this
> is not constrained just to Screenshots, but any system add-on which is a web
> extension.

It's a problem for any system add-on, full stop. Fixing the non-webextension side of things is just the first thing that needs to happen.

Comment 9

[John Gruen [:jgruen]]

As far as I'm concerned, this is not a blocker for Screenshots in 54. That having been said, if we're using SAOs as a shipping method for features in future, we might consider looping in Andy's team to help prioritize.

Comment 10

[Mark Banner (:standard8)]

(In reply to [:jgruen] from comment #9)
> As far as I'm concerned, this is not a blocker for Screenshots in 54. That
> having been said, if we're using SAOs as a shipping method for features in
> future, we might consider looping in Andy's team to help prioritize.

So just to clarify in case folks don't realise. There are two things that this affects:

1) The Restore Defaults button in the Customisable toolbars won't work properly. Screenshots (or any other WebExtension SAO) will get moved into the pallet if the user selects to Restore Defaults.
2) We can't enable Screenshots in at least the browser_BrowserUITelemetry_defaults.js test until we fix this (bug 1358097, although I guess we could turn it off for just that test if we really have to).

Comment 11

[:Gijs (he/him)]

(In reply to [:jgruen] from comment #9)
> As far as I'm concerned, this is not a blocker for Screenshots in 54. That
> having been said, if we're using SAOs as a shipping method for features in
> future, we might consider looping in Andy's team to help prioritize.

The current state of things means "restore defaults" in customize mode is always enabled (on a clean profile with screenshots enabled) because there's nothing deciding the order screenshots and pocket go into the navbar. This in turn impacts telemetry on customization and might impact migration of such customizations for 57, etc. This is also why we're basically flying blind in terms of automated testing, because we have to disable screenshots because this customization stuff causes all these test failures.

Solving this before non-WE add-ons went on the chopping block was difficult. Now that that's been decided, solving this is straightforward, but like any work, non-0 effort.

We can still decide to ship Screenshots in 54 without fixing this, despite the downsides, I just wish we didn't keep punting on fixing this infrastructure problem. We punted for pocket, for hello, for the report site button (though we don't ship that outside of nightly) and now maybe for screenshots. We need (either before or after we ship screenshots) to stop punting and fix this properly.

Comment 12

[John Gruen [:jgruen]]

Attachment: Screen Shot 2017-04-25 at 7.28.36 PM.png

Comment 13

[John Gruen [:jgruen]]

(In reply to :Gijs from comment #11)


> The current state of things means "restore defaults" in customize mode is
> always enabled (on a clean profile with screenshots enabled) because there's
> nothing deciding the order screenshots and pocket go into the navbar.

Thanks for the clarification Gijs! I'm not able to repro the described behavior, though maybe the UI state is a mismatch with Telemetry in this case? See the screenshot in comment 12.

Comment 14

[:Gijs (he/him)]

(In reply to [:jgruen] from comment #13)
> (In reply to :Gijs from comment #11)
> 
> 
> > The current state of things means "restore defaults" in customize mode is
> > always enabled (on a clean profile with screenshots enabled) because there's
> > nothing deciding the order screenshots and pocket go into the navbar.
> 
> Thanks for the clarification Gijs! I'm not able to repro the described
> behavior, though maybe the UI state is a mismatch with Telemetry in this
> case? See the screenshot in comment 12.

The screenshot shows the screenshot button in the "additional tools" section rather than in the nav-bar. My understanding is that the button gets added to the toolbar by default when the add-on is disabled? So this looks like it's the result after you select 'restore defaults'.

The expected state is that, when started on a clean profile in the default configuration (which, soon, is going to include screenshots), the button isn't enabled/clickable. It becomes enabled when you customize something, and if you click it, the default (initial) state is restored.

Buttons we add via system add-ons should be treated like buttons added from any other part of Firefox. They should be considered part of the 'default' set, and they shouldn't trigger this button being enabled, and if you make some other customization that enables the button, the system-add-on buttons should stay in their usual places (not get moved to the palette ("additional tools")).

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs from comment #14)
> My understanding is that the button gets added
> to the toolbar by default when the add-on is disabled? So this looks like
> it's the result after you select 'restore defaults'.

Err, when the add-on is *enabled*. D'oh.

Comment 16

[Andy McKay]

If people think its worth it, I would have no problem is someone wrote a WebExtension API for that. We'd limit access to down to add-ons signed using the internal mozilla key. This will require bug 1280235 to be done first.

Comment 17

[Wil Clouser [:clouserw]]

I've talked to andym, mossop, and jgruen (Screenshots Product) and we all agree this is a good bug to fix soon, but it shouldn't block screenshots launching.