982656 - 'Restore Defaults' should place all 3rd party widgets in the palette

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From Asa,

> Add-on toolbar buttons should not be allowed to claim to be "default"
> buttons and be restored to my toolbar when I tell Firefox to "Restore
> Defaults."  This button should do what it claims to do and what users
> will expect it to do. It should undo what everdamage has been done to
> the user's toolbar, whether self-inflicted or by way of a pushy add-on.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Discussion can be found here, https://mail.mozilla.org/pipermail/firefox-dev/2014-March/001472.html

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8389962 [details] [diff] [review]
Patch

Review of attachment 8389962 [details] [diff] [review]:
-----------------------------------------------------------------

I've got some questions with this patch, and I'm not 100% sure just my eyes should be on it. I think Gijs or Unfocused should look at it too.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1670,5 @@
>          let defaults = gAreas.get(aArea).get("defaultPlacements");
>          if (defaults) {
> +          for (let id of defaults) {
> +            let widget = gPalette.get(id);
> +            if (!aIsReset || !widget || widget.source == CustomizableUI.SOURCE_BUILTIN) {

Wait... why are we adding it if !widget? Not too clear what the logic is doing here. If this is doing what you wanted, some documentation to clear it up would probably be good.

@@ +1809,5 @@
>      this.notifyListeners("onWidgetCreated", widget.id);
>  
>      if (widget.defaultArea) {
>        let area = gAreas.get(widget.defaultArea);
> +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;

Wait... so if area.get("defaultCollapsed") returns true, then areaIsDefaultCollapsed is false?

That doesn't seem right...

@@ +1810,5 @@
>  
>      if (widget.defaultArea) {
>        let area = gAreas.get(widget.defaultArea);
> +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> +      if (areaIsDefaultCollapsed ||

So we will let add-ons default themselves to collapsed toolbars?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #3)
> 
> @@ +1809,5 @@
> >      this.notifyListeners("onWidgetCreated", widget.id);
> >  
> >      if (widget.defaultArea) {
> >        let area = gAreas.get(widget.defaultArea);
> > +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> 
> Wait... so if area.get("defaultCollapsed") returns true, then
> areaIsDefaultCollapsed is false?
> 
> That doesn't seem right...

If an area has defaultCollapsed==null, then we don't care about what the defaultPlacements are for that area since the area is not taken in to account for reset/inDefaultState calculations.

> @@ +1810,5 @@
> >  
> >      if (widget.defaultArea) {
> >        let area = gAreas.get(widget.defaultArea);
> > +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> > +      if (areaIsDefaultCollapsed ||
> 
> So we will let add-ons default themselves to collapsed toolbars?

Yes, this is necessary for an add-on that registers a toolbar and also populates that toolbar with some default items.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > 
> > @@ +1809,5 @@
> > >      this.notifyListeners("onWidgetCreated", widget.id);
> > >  
> > >      if (widget.defaultArea) {
> > >        let area = gAreas.get(widget.defaultArea);
> > > +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> > 
> > Wait... so if area.get("defaultCollapsed") returns true, then
> > areaIsDefaultCollapsed is false?
> > 
> > That doesn't seem right...
> 
> If an area has defaultCollapsed==null, then we don't care about what the
> defaultPlacements are for that area since the area is not taken in to
> account for reset/inDefaultState calculations.
> 
> > @@ +1810,5 @@
> > >  
> > >      if (widget.defaultArea) {
> > >        let area = gAreas.get(widget.defaultArea);
> > > +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> > > +      if (areaIsDefaultCollapsed ||
> > 
> > So we will let add-ons default themselves to collapsed toolbars?
> 
> Yes, this is necessary for an add-on that registers a toolbar and also
> populates that toolbar with some default items.

Ah, I see - so you're using the defaultCollapsed property as a way of determining whether or not the toolbar is provided from an add-on. I wonder if instead of doing this instead, an area can have a "source" property, mapped to SOURCE_BUILTIN or SOURCE_EXTERNAL (I know those are only for widgets right now, but we could extend their usage).

At the very least, if we went this route, I think we should rename the variable to isExternalToolbar or something.

Comment 6

[:Gijs (out for now; he/him)]

We should figure this out, but we discussed on IRC that we should probably deprio for 29

Comment 7

[Madhava Enros [:madhava]]

Some more context - this need a decision:

10:55 Gijs madhava: I think we first need to figure out what the desired end state is though. Both Blair and I think that we should provide add-ons with a way to say "insert this button here by default".
10:55 madhava is that the same as "insert it here on install?"
10:55 Gijs madhava: I guess that is a question.
10:55 Gijs s/a/the/
10:55 Gijs madhava: there's an argument that it should be limited to install, and an argument that it should not
10:56 Gijs madhava: for some context, add-on devs have complained since approximately forever that for whatever reason, their buttons get removed, and that's why they used to *always* insert into the navbar/add-on bar if they found their button wasn't in the document after the window had loaded
10:56 Gijs madhava: the current system basically says "that is evil, don't do it, use this property instead"
10:57 madhava I see
10:57 Gijs madhava: this way, if the user removes the button in customize mode, it gets removed and stays removed
10:57 madhava so we should tread lightly around changing the thing we've given them so that they don't behave badly
10:57 madhava I get it
10:57 Gijs madhava: but equally, using "restore defaults" will put it back so that it doesn't get "lost"
10:58 madhava I'm going to paste this in the bug
10:58 Gijs (although arguably, with the current customize mode, buttons are never as lost as they were, as it's so much more accessible)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

If and when this bug gets fixed, it should get QA attention as it will noticeable affect the behavior of how Restore Defaults works.

Comment 9

[Marco Mucci [:MarcoM]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> If and when this bug gets fixed, it should get QA attention as it will
> noticeable affect the behavior of how Restore Defaults works.

This was originally set as a UX bug by Madhava in the priority list and that's why I set the verification as [qa-].  I've sent a 'need info' to him to confirm if this is still the case.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Ok, the confusion stems from this starting out as a bug that has a patch and is in progress of making the change. I missed the redirect of it to a [ux] bug.

Comment 11

[Madhava Enros [:madhava]]

Jared - I set it as a UX bug because the last I saw there was some debate still about whether changing this behavior would just cause more add-ons authors to do even more to add their add-ons on every restart (Gijs was explaining this in the block in Comment 7. I.e. are we just going to make things worse?

Maybe we have an answer for this right now, but otherwise I think the next step here is to make a decision. What do y

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Now that it is easier for people to customize their browser, and thus harder for people to "lose" items, I think we can let "Restore Defaults" do the full job of restoring the toolbars and menus to their default state.

Comment 13

[Mike Hoye [:mhoye]]

This made it out of priority backlog unassigned; adding [diamond] status. Jaws, you feel like mentoring this one, should a contributor step up?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Not really. The patch attached here is pretty close to complete. We discussed this over Vidyo about the potential issue that Gijs brought up in comment #7 and decided to move forward with this patch. So this patch will probably need to be rebased and go through the review process. I don't know if that is something good for a new contributor to pick up?

Comment 15

[Mike Hoye [:mhoye]]

Nope, if you're going with that patch, that's fine. Thanks!

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (rebased)

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8433610 [details] [diff] [review]
Patch (rebased)

Review of attachment 8433610 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1930,5 @@
>      this.notifyListeners("onWidgetCreated", widget.id);
>  
>      if (widget.defaultArea) {
>        let area = gAreas.get(widget.defaultArea);
> +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;

I agree with Mike's comments here that we shouldn't use the defaultCollapsed property as a proxy for "is this an add-on area".

There's a list of builtin toolbars on CustomizableUI already. We could use that + it being a toolbar instead?

@@ +1932,5 @@
>      if (widget.defaultArea) {
>        let area = gAreas.get(widget.defaultArea);
> +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> +      if (areaIsDefaultCollapsed ||
> +          widget.source == CustomizableUI.SOURCE_BUILTIN) {

So, I'm not a fan of the SOURCE_BUILTIN checking as it is, because only things in CustomizableWidgets.jsm will be marked as builtin. There's no way for external consumers elsewhere in the fx tree to create 'builtin' items. That sets us up for bloating CustomizableWidgets.jsm, and not being able to keep each widget with the rest of its feature's implementation.

That said, I don't know how to fix this in a simple fashion. We could allow consumers to specify this, but the obvious problem with that is that add-ons might abuse it. We could potentially make AMO check for this? It's possibly worth consulting with Jorge or others on this. Do you have any other ideas on how we could correctly distinguish this?

::: browser/components/customizableui/test/browser_982656_restore_defaults_builtin_widgets.js
@@ +5,5 @@
> +"use strict";
> +
> +// Restoring default should not place addon widgets back in the toolbar
> +add_task(function() {
> +  ok(CustomizableUI.inDefaultState, "Default state to begin");

We used to do this in head.js but it got lost with the switch to add_task. It should already be the case everywhere... if you feel brave enough, please ensure this gets run after every test from head.js (which might be tricky because it needs to be after any cleanups added by the test itself...) - or file a followup for it. Having it in every test is a little redundant and makes us find the issue *after* it's happened (ie it's not this test's fault if it's not finding the browser in its default state).

@@ +7,5 @@
> +// Restoring default should not place addon widgets back in the toolbar
> +add_task(function() {
> +  ok(CustomizableUI.inDefaultState, "Default state to begin");
> +
> +  const kWidgetId = "add-on-widget";

This is left in gSeenWidgets afterwards; can you make the ID more unique so that we can be sure it doesn't cause subtle and hard-to-detect issues with later/other tests? Other tests all use some description of what the test is testing with the widget (or should, anyway).

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

(In reply to :Gijs Kruitbosch from comment #17)
> So, I'm not a fan of the SOURCE_BUILTIN checking as it is, because only
> things in CustomizableWidgets.jsm will be marked as builtin. There's no way
> for external consumers elsewhere in the fx tree to create 'builtin' items.
> That sets us up for bloating CustomizableWidgets.jsm, and not being able to
> keep each widget with the rest of its feature's implementation.
> 
> That said, I don't know how to fix this in a simple fashion. We could allow
> consumers to specify this, but the obvious problem with that is that add-ons
> might abuse it. We could potentially make AMO check for this? It's possibly
> worth consulting with Jorge or others on this. Do you have any other ideas
> on how we could correctly distinguish this?

Yeah, this is a difficult problem but one that I don't think needs to be addressed immediately. I would rather handle this in a follow-up.
 
> :::
> browser/components/customizableui/test/
> browser_982656_restore_defaults_builtin_widgets.js
> @@ +5,5 @@
> > +"use strict";
> > +
> > +// Restoring default should not place addon widgets back in the toolbar
> > +add_task(function() {
> > +  ok(CustomizableUI.inDefaultState, "Default state to begin");
> 
> We used to do this in head.js but it got lost with the switch to add_task.
> It should already be the case everywhere... if you feel brave enough, please
> ensure this gets run after every test from head.js (which might be tricky
> because it needs to be after any cleanups added by the test itself...) - or
> file a followup for it.

This again is something I'd like to handle in a follow-up, as the test cleanup functions that are run in a separate add_task may need some refactoring.

https://tbpl.mozilla.org/?tree=Try&rev=d42b786254cc

Comment 19

[:Gijs (out for now; he/him)]

So, I must have misunderstood something.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > 
> > @@ +1809,5 @@
> > >      this.notifyListeners("onWidgetCreated", widget.id);
> > >  
> > >      if (widget.defaultArea) {
> > >        let area = gAreas.get(widget.defaultArea);
> > > +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> > 
> > Wait... so if area.get("defaultCollapsed") returns true, then
> > areaIsDefaultCollapsed is false?
> > 
> > That doesn't seem right...
> 
> If an area has defaultCollapsed==null, then we don't care about what the
> defaultPlacements are for that area since the area is not taken in to
> account for reset/inDefaultState calculations.

I don't believe this is true, looking at the inDefaultState implementation. We do also return external toolbars to their defined defaultState. This will also mean that if an add-on, for instance, defined a toolbar with defaultPlacements: ["home-button"], the behaviour of reset() would be undefined (probably dependent on the registration order). That edgecase can be a followup, but I believe the current code you have is just strange:

+      if (CustomizableUI.isBuiltinToolbar(widget.defaultArea) &&
+          widget.source == CustomizableUI.SOURCE_BUILTIN) {
+        // < add to defaultPlacements >


So this essentially means that:

(1) add-ons can't add stuff to the defaultPlacements of the toolbars (OK)
(2) add-ons can't add stuff to the defaultPlacements of add-on-provided toolbars (maybe OK, but I don't think so...)
(3) nobody (incl. builtin widgets) can add stuff to the defaultPlacements of the menu panel (definitely not OK)

At the moment I guess there is redundancy in the sense that we define the defaultPlacements for the menupanel /anyway/, but I don't think the check as it is right now makes sense.

> 
> > @@ +1810,5 @@
> > >  
> > >      if (widget.defaultArea) {
> > >        let area = gAreas.get(widget.defaultArea);
> > > +      let areaIsDefaultCollapsed = area.get("defaultCollapsed") === null;
> > > +      if (areaIsDefaultCollapsed ||
> > 
> > So we will let add-ons default themselves to collapsed toolbars?
> 
> Yes, this is necessary for an add-on that registers a toolbar and also
> populates that toolbar with some default items.

Right, so this seems to imply you don't think (2) is OK either. :-)

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This should address the issues you brought up in the previous review pass. I was able to simplify the patch as it now only deals with building up the defaultset. I also added a test for the custom toolbar case.

While working on the test I noticed that we check for a null areaNode in CustomizeMode._getCustomizableChildForNode in one part but forget to check for null a couple lines later. I added a null check for this.

Comment 21

[:Gijs (out for now; he/him)]

Comment on attachment 8437242 [details] [diff] [review]
Patch v2

Review of attachment 8437242 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good apart from the nits below. Please file followups as previously discussed.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1930,3 @@
>        let area = gAreas.get(widget.defaultArea);
>        //XXXgijs this won't have any effect for legacy items. Sort of OK because
>        // consumers can modify currentset? Maybe?

Nit: I think you can just remove this comment.

@@ +1932,5 @@
>        // consumers can modify currentset? Maybe?
> +      if (widget.source == CustomizableUI.SOURCE_BUILTIN) {
> +        addToDefaultPlacements = true;
> +      } else if (!CustomizableUI.isBuiltinToolbar(widget.defaultArea) &&
> +                 !widget.defaultArea == CustomizableUI.AREA_PANEL) {

!foo == bar ? ITYM widget.defaultArea != CustomizableUI.AREA_PANEL maybe?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/e888096eee4b

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Filed bug 1023319 and bug 1023322 as follow-ups.

Comment 24

[Ed Morley [:emorley]]

Backed out for failures in browser_982656_restore_defaults_builtin_widgets.js:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=e888096eee4b&jobname=browser-chrome-1

remote:   https://hg.mozilla.org/integration/fx-team/rev/abe2531afc3d

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Relanded with test fixed, https://hg.mozilla.org/integration/fx-team/rev/d93b579522fd

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d93b579522fd

Comment 27

[Petruta Horea [:phorea], Desktop Test Engineering]

Tested 33.0a1 Nightly under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 32-bit.

I installed several add-ons (Adblock Plus, Firebug, Ghostery, Greasemonkey, LastPass, SessionManager..) in order to reproduce the issue.
In a Nightly build before the fix, after 'Restore Default', all the add-ons were moved to the palette except for Adblock Plus.
In Nightly 2014-06-11, Adblock Plus is moved to the palette too.
Is this the expected behavior? Are there any other add-ons I could use for tests?

Also, please see what I encountered while testing the customization option: this image  http://i.imgur.com/LExzbmH.png appeared for a second every time when exiting Customization. Unfortunately I couldn't find steps to reproduce and I couldn't see it again with the same profile, different builds / different profile, same build.
Maybe someone saw this too or has an idea of where those icons come from. I reproduced it under Win 7, Nightly 2014-06-11.

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #27)
> Tested 33.0a1 Nightly under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX
> 32-bit.
> 
> I installed several add-ons (Adblock Plus, Firebug, Ghostery, Greasemonkey,
> LastPass, SessionManager..) in order to reproduce the issue.
> In a Nightly build before the fix, after 'Restore Default', all the add-ons
> were moved to the palette except for Adblock Plus.
> In Nightly 2014-06-11, Adblock Plus is moved to the palette too.
> Is this the expected behavior? Are there any other add-ons I could use for
> tests?

Yes and no, that I know of, respectively.

> Also, please see what I encountered while testing the customization option:
> this image  http://i.imgur.com/LExzbmH.png appeared for a second every time
> when exiting Customization. Unfortunately I couldn't find steps to reproduce
> and I couldn't see it again with the same profile, different builds /
> different profile, same build.

Did you still have all the add-ons installed, and could you not reproduce without them? It sounds like the issue was introduced by one of the add-ons. If you can figure out steps to reproduce, please file a new bug.

Comment 29

[Petruta Horea [:phorea], Desktop Test Engineering]

Thanks, Gijs!

Jared, any other thoughts? If not, I'll mark this bug accordingly. Thank you!

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Nope, this is all good. Thanks!

Comment 31

[Petruta Horea [:phorea], Desktop Test Engineering]

Marking as verified based on comment 27. 

(In reply to :Gijs Kruitbosch from comment #28)
> Did you still have all the add-ons installed, and could you not reproduce
> without them? It sounds like the issue was introduced by one of the add-ons.
> If you can figure out steps to reproduce, please file a new bug.

Now I can't reproduce with or without add-ons. I'll add a new bug once I see it again.