876926 - CustomizeMode.jsm should unwrap/re-wrap widgets based on an event sent from CustomizableUI

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

In CustomizeMode.jsm, we need to unwrap widgets whenever they're moved, so that CustomizableUI can safely interact with them. However, it makes assumptions as to when it needs to do that - naming, assuming widgets need unwrapped/re-wrapped only for an action performed by CustomizeMode.jsm - which is wrong. Any code can call CustomizableUI's APIs when one of the browser windows is in customize mode, two windows can be in customize mode, or an add-on can be added/removed.

So, CustomizeMode.jsm needs to listen for events for when to unwrap/re-wrap widgets. We have onWidgetMoved/onWidgetAdded/onWidgetRemoved events - CustomizableUI will need to also send events indicating that these are about to happen (ie, onWidgetMoving/onWidgetAdding/onWidgetRemoving), and CustomizeMode needs to unwrap when these events are received, and re-wrap when receiving the existing events.

Additionally, with bug 870011 with have Reset to Defaults working - which also assumes a reset will only happen via that instance of the UI. So that also needs to be triggered off some event (though, maybe it will just work with the events mentioned above).

Comment 1

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

Not taking this for Australis:M7.

Comment 3

[Justin Dolske [:Dolske]]

Related to bug 885575?

Comment 4

[Justin Dolske [:Dolske]]

I'm unclear what the impact of not doing this is. Sounds like something that is unlikely to happen frequently?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Justin Dolske [:Dolske] from comment #4)
> I'm unclear what the impact of not doing this is. Sounds like something that
> is unlikely to happen frequently?

Shouldn't happen frequently, no. But if an add-on uses the CustomizableUI API while the browser is in customization mode (even if its just because the add-on was updated), the whole customization mode will break... potentially along with parts of the toolbars. So, uh, kinda serious side affects.

Comment 6

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #5)
> (In reply to Justin Dolske [:Dolske] from comment #4)
> > I'm unclear what the impact of not doing this is. Sounds like something that
> > is unlikely to happen frequently?
> 
> Shouldn't happen frequently, no. But if an add-on uses the CustomizableUI
> API while the browser is in customization mode (even if its just because the
> add-on was updated), the whole customization mode will break... potentially
> along with parts of the toolbars. So, uh, kinda serious side affects.

Yes. Going to be bold and go for P3 instead, also because this'll involve some core changes that we're going to need to test well, so doing it at the last minute would be very suboptimal.

(In reply to Justin Dolske [:Dolske] from comment #3)
> Related to bug 885575?

Yes.

Comment 7

[:Gijs (he/him)]

I'll poke at this tomorrow.

Comment 8

[:Gijs (he/him)]

Attachment: CustomizeMode.jsm should unwrap/re-wrap widgets on an event-based basis,

This, essentially. The palette-specific drag code and our position adjusting stuff needed to be taught about all this. Otherwise this works. I'll file a bug on splitting off the positioning stuff into a singleton module tonight/tomorrow, as per some discussion on IRC.

Comment 9

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

Comment on attachment 796779 [details] [diff] [review]
CustomizeMode.jsm should unwrap/re-wrap widgets on an event-based basis,

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

Can bug 885575 be resolved after this is fixed or is there more work to do?

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1053,5 @@
>  
> +CustomizeMode.prototype.onWidgetAdded =
> +CustomizeMode.prototype.onWidgetMoved =
> +CustomizeMode.prototype.onWidgetRemoved =
> +CustomizeMode.prototype._onUIChange;

I don't think I've seen this style elsewhere. We should either just place CustomizeMode.prototype._onUIChange on each line separately, or indent all lines except the first one.

@@ +1058,5 @@
> +
> +CustomizeMode.prototype.onWidgetBeforeAdding =
> +CustomizeMode.prototype.onWidgetBeforeRemoving =
> +CustomizeMode.prototype.onWidgetBeforeMoving =
> +function _cmBeforeNodeTouch(aWidgetId, aTarget) {

Nit: The cm prefix isn't necessary here since all of these functions are limited to the CustomizeMode.jsm.

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #9)
> Comment on attachment 796779 [details] [diff] [review]
> CustomizeMode.jsm should unwrap/re-wrap widgets on an event-based basis,
> 
> Review of attachment 796779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can bug 885575 be resolved after this is fixed or is there more work to do?

That bug doesn't have a description, so I'm not 100% sure. :-)
Taken literally, we'd need to teach CustomizableUI about toolbarpaletteitems and such. Which would be sadfaces. If that's not actually what that bug wants to be doing, then I think it can be resolved.

Note that a case that we weren't addressing before and aren't addressing now is what happens when an add-on adds (creates) a widget when in customize mode. We should probably add it to the visiblePalette, but that should be a separate bug IMO (we can do that from an onWidgetCreated handler).


Separately, when trying out this patch some more, I realized it's broken sometimes when dragging to/from the panel. Turns out that the culprit for 'sometimes' is here: http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableUI.jsm?mark=606-615#606 . I'm inclined to just remove the specialcasing and never to reinsert back into the palette, but I'll check again whether that's a good idea when I wake up tomorrow.

Comment 11

[:Gijs (he/him)]

Attachment: Fix placement id error, and accidental advantage had in toolbar.xml,

Right. So this wasn't nearly enough, it turned out, so I split up my patches. This is the first. It addresses a small bug in getWidgetIdsInArea, and two tiny style/hardening issues.

Comment 12

[:Gijs (he/him)]

Attachment: Use CustomizableUI events to wrap items for CustomizeMode,

This is the core patch. It does require (the functional parts of) the previous patch in order to work.

Comment 13

[:Gijs (he/him)]

Attachment: Tests for CustomizableUI-event-based wrapping behaviour,

Then I wrote tests. They all pass with these patches, as do all the other CustomizableUI tests.

Comment 14

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

Comment on attachment 797204 [details] [diff] [review]
Fix placement id error, and accidental advantage had in toolbar.xml,

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

r+ with the following issue addressed.

::: browser/components/customizableui/content/toolbar.xml
@@ +173,5 @@
>            // Get a list of items only in the new list
>            let newIds = [id for (id of newVal) if (oldIds.indexOf(id) == -1)];
>            CustomizableUI.beginBatchUpdate();
>            for (let newId of newIds) {
> +            let oldIds = CustomizableUI.getWidgetIdsInArea(this.id);

I would prefer that we not introduce a new scoped variable for this. Just continue to use the previously defined oldIds variable (IOW, remove the 'let ' here).

Comment 15

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

Comment on attachment 797205 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +612,5 @@
> +    if (aTarget.ownerDocument.defaultView != this.window) {
> +      return;
> +    }
> +    afterNodeTouch.call(this, aWidgetId, aTarget);
> +    // If an API-based widget is removed while customizing, append it to the palette.

How can a widget be removed in such a way that it is not already destined for the palette?

@@ +1078,5 @@
>    }
>  };
>  
> +function beforeNodeTouch(aWidgetId, aTarget) {
> +  if (aTarget.ownerDocument.defaultView != this.window) {

When will the target's window not equal this.window? (same question for the usage below)

Comment 16

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #15)
> Comment on attachment 797205 [details] [diff] [review]
> Use CustomizableUI events to wrap items for CustomizeMode,
> 
> Review of attachment 797205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +612,5 @@
> > +    if (aTarget.ownerDocument.defaultView != this.window) {
> > +      return;
> > +    }
> > +    afterNodeTouch.call(this, aWidgetId, aTarget);
> > +    // If an API-based widget is removed while customizing, append it to the palette.
> 
> How can a widget be removed in such a way that it is not already destined
> for the palette?

STR:

1. Open a second window.
2. Open customize mode in one window
3. In your first window, open the panel.
4. Right click on a widget and click "Remove from Menu"

Alternative step 4: have an add-on (install/uninstall) or other code call removeWidgetFromArea.

Per the link in comment 10, API widgets (and special widgets, which is OK) don't get put in the palette automatically when removed, so we have to do it 'manually' when in customize mode.

Thinking more about this, just like for the onWidgetCreated call, we should probably also handle onWidgetDestroyed in Customize Mode, and remove the (now empty) wrapper.


> @@ +1078,5 @@
> >    }
> >  };
> >  
> > +function beforeNodeTouch(aWidgetId, aTarget) {
> > +  if (aTarget.ownerDocument.defaultView != this.window) {
> 
> When will the target's window not equal this.window? (same question for the
> usage below)

These will get called for each areaNode. So, if you open customize mode in one window, and have a second (or nth) window, all these functions will be called for all those windows.

Comment 17

[:Gijs (he/him)]

Comment on attachment 797204 [details] [diff] [review]
Fix placement id error, and accidental advantage had in toolbar.xml,

Pushed this bit already: https://hg.mozilla.org/projects/ux/rev/761ef871425b

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 797204 [details] [diff] [review]
Fix placement id error, and accidental advantage had in toolbar.xml,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1960,5 @@
>        throw new Error("Area not yet restored");
>      }
>  
> +    // We need to clone this, as we don't want to let consumers muck with placements
> +    return [].concat(gPlacements.get(aArea));

Late driveby nit: Now days we can more elegantly clone arrays, using the spread operator:
  [...myArray];

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 797205 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +564,5 @@
>  
>        let container = areaNode.customizationTarget;
>        let [provider, widgetNode] = this.getWidgetNode(aWidgetId, window);
>  
> +      this.notifyListeners("onWidgetBeforeAdding", aWidgetId, container);

Why do we need to send this event individually for every window? Doesn't seem like the listeners really need to care about that level of detail (all windows should be the same anyway, right?), but it means a lot more events and needing to filter them.

ie, currently, the order of events is:
* onWidgetBeforeAdding
* [add widget node]
* onWidgetAfterAdding
* onWidgetBeforeAdding
* [add widget node]
* onWidgetAfterAdding
* onWidgetBeforeAdding
* [add widget node]
* onWidgetAfterAdding

Instead of:
* onWidgetBeforeAdding
* [add widget node]
* [add widget node]
* [add widget node]
* onWidgetAfterAdding


Also, I think these events should be passing the same parameters as the existing events (onWidgetAdded, etc).

Comment 20

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #19)
> Comment on attachment 797205 [details] [diff] [review]
> Use CustomizableUI events to wrap items for CustomizeMode,
> 
> Review of attachment 797205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +564,5 @@
> >  
> >        let container = areaNode.customizationTarget;
> >        let [provider, widgetNode] = this.getWidgetNode(aWidgetId, window);
> >  
> > +      this.notifyListeners("onWidgetBeforeAdding", aWidgetId, container);
> 
> Why do we need to send this event individually for every window? Doesn't
> seem like the listeners really need to care about that level of detail (all
> windows should be the same anyway, right?)

No, they aren't. This is also exactly why. One window can be in customization mode, others aren't (by definition, there are only 0-1 customizing windows). Then there are widgets which appear in some windows, but not others (showInPrivateBrowsing). Toolbar overflow is different in different windows, which can have different sizes.

Comment 21

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

Comment on attachment 797205 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +582,5 @@
>        if (area.get("type") == CustomizableUI.TYPE_TOOLBAR) {
>          areaNode.setAttribute("currentset", areaNode.currentSet);
>        }
> +
> +      this.notifyListeners("onWidgetAfterAdding", aWidgetId, container, widgetNode);

I agree with Blair that it would be nice if the these events all passed the same parameters.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1084,5 @@
> +  }
> +  if (aTarget.id == CustomizableUI.AREA_PANEL) {
> +    this._removePanelCustomizationPlaceholders();
> +  }
> +  this._unwrapItemsInArea(this.visiblePalette);

"// TODO(bug 885575): Remove once CustomizeUI can handle moving wrapped widgets." should be placed before this line and before the similar lines in afterNodeTouch.

Comment 22

[:Gijs (he/him)]

Attachment: Use CustomizableUI events to wrap items for CustomizeMode,

This uses just two events, and only unwraps the required items. Still passes the same tests. I think this is probably our best bet. I don't think moving the wrapping/unwrapping logic that is now in this patch in CustomizeMode.jsm to CustomizableUI.jsm is good for separation of concerns etc. (in effect, I think that we should fix this bug and then wontfix bug 885575). Does that sound agreeable?

Comment 23

[:Gijs (he/him)]

Attachment: Tests for CustomizableUI-event-based wrapping behaviour,

The test does need to add the security-related prefs though.

Comment 24

[:Gijs (he/him)]

Comment on attachment 804292 [details] [diff] [review]
Tests for CustomizableUI-event-based wrapping behaviour,

Carrying over r+

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 804291 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,

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

+1 to comment 22

Comment 26

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/80958a628e14
https://hg.mozilla.org/projects/ux/rev/d07c3d3fc083

Jared, per comment 22 / comment 25, and this patch, are you OK wontfixing bug 885575?

Comment 27

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

(In reply to :Gijs Kruitbosch from comment #26)
> https://hg.mozilla.org/projects/ux/rev/80958a628e14
> https://hg.mozilla.org/projects/ux/rev/d07c3d3fc083
> 
> Jared, per comment 22 / comment 25, and this patch, are you OK wontfixing
> bug 885575?

Yep, just closed it out.

Comment 28

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/761ef871425b
https://hg.mozilla.org/mozilla-central/rev/d07c3d3fc083
https://hg.mozilla.org/mozilla-central/rev/80958a628e14