855295 - Nav bar is not preserving order of customized items on session restore

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

STR:

1) Start customizing
2) Drag the "Cut" item from the palette onto the nav bar so that it immediately to the right of the search input
3) Click "Done"
4) Restart the browser

What happens:

The cut item is still in the nav bar, but the ordering is wrong - it's now on the left of the search input.

Reproducible on all platforms.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hmm, pretty sure this is a new bug (ie, due to work done since November) - which should make it easier to track down.

Comment 2

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

Attachment: Patch v1

It looks like when the nav-bar's state is re-loaded, it goes through each of the items in the nav-bar's currentset (since it has a legacy property), and adds each ID in there to the "placements" array for that area.

The problem is that some items in currentset are not being found. findWidgetInToolbox, for example, will not find the unified back-forward button (since it's not already in the customizationtarget, and is not in the palette).

These items, though "missing" (according to CustomizableUI), still definitely existed in the UI. They just stay put because they're not inside the customizationtarget.

These items, however, do *not* get removed from the placements array. That's why nav-bar is having a hard time remembering positions - when you insert something into position 1 for example, it's assuming you've jammed something after the unified-back-forward button, as opposed to after the search input (which by default starts in position 0 in the nav-bar's customizationtarget).

So what this patch does is cause buildArea to return an accurate placements array once the building of the area is completed. buildArea knows which widgets it can't find, and just does not include them in its returned list.

Blair - am I being shortsighted with this solution? Is there a good reason to keep missing widgets in the placements array?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 730777 [details] [diff] [review]
Patch v1

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

(In reply to Mike Conley (:mconley) from comment #2)
> Blair - am I being shortsighted with this solution? Is there a good reason
> to keep missing widgets in the placements array?

Ah, yes, keeping non-existent widgets in there was intentional (sorry, I should have documented that - it's non-obvious). The idea is that you should have able to have an add-on be uninstalled or disabled (either manually, or via blocklisting/incompatibility), then sometime later the add-on can be reinstalled/re-enabled and it's widget would automatically be restored to whatever position you originally put it in. Obviously, leads to the placements growing over time with non-existent widgets - but it's a small overhead that I don't think we should worry about.

And, of course, we need to somehow deal with this when customizing... but I'm not sure what's the best approach for doing that. One option would be to have CustomizeMode keep track of the relative position in the actual UI where a widget is being placed (which doesn't include missing placements) - and from that work out the absolute position in the list that does contain non-existent widgets (perhaps via an API like CustomizableUI.addWidgetToAreaRelative).

Maybe we should have an API for purging specific non-existent widgets from the placements - we'd be able to permanently remove old built-in widgets that way, via a hard-coded list of IDs we know won't exist (and aren't from any add-on). However, that would break the UI of old builds if we're still persisting currentset (this patch would cause the same breakage, FWIW).

Comment 4

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

Comment on attachment 730777 [details] [diff] [review]
Patch v1

Ok, good to know - thanks Blair.

Comment 5

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

Attachment: Patch v2

Ok, this might be a little better.

We can assume that the widget that the user is dropping on is visible, and in a customizable area, so really all we have to do is make sure that CustomizableUI.onWidgetMoved and CustomizableUI.onWidgetAdded know to map the position that they're passed to the ID of the node they're supposed to insert before.

This means also temporarily unwrapping the target nodes when moving or adding widgets.

Can you think of any edge-cases I'm missing here?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

Well that works nicely. (Sorry for the delay, got side-tracked catching up on all the work that happened in CustomizeMode.)

One thing I think needs fixed up though, r+ with that.

::: browser/modules/CustomizableUI.jsm
@@ +1452,5 @@
>    },
>    reset: function() {
>      CustomizableUIInternal.reset();
> +  },
> +  positionOf: function(aArea, aWidgetId) {

This is basically already implemented as CustomizableUIInternal.getPlacementOfWidget() - should just expose that in the public API instead. As a bonus, it also gives a more accurate description of the placement of the widget, since it includes the area.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(Whats with people not setting the status these days?)

Comment 8

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

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #7)
> (Whats with people not setting the status these days?)

Ack, sorry - I'm horrible with that flag. I'll be more vigilant. :)

Comment 9

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

And thanks for the review!

Comment 10

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

Attachment: Patch v2.1 (r+'d by Unfocused)

All fixed up.

Comment 11

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

Ugh, this patch regressed us - we can no longer drag items into the palette.

We should probably get some regression tests written at some point, hence my filing of bug 858196 (though I'm open to other suggestions on where to put such tests).

Anyhow, working on a follow-up...

Comment 12

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

Attachment: Only check for position after we're ruled out the dragging-to-palette case.

We were bailing out if the targetNode's position wasn't found - but that's expected if we're dragging into the palette.

We shouldn't care about position unless we've ruled out the dragging-to-palette case.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 733499 [details] [diff] [review]
Only check for position after we're ruled out the dragging-to-palette case.

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

(Can we put links to the changeset when landing on Jamun? I find it useful.)

Comment 14

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

Landed in jamun as:

https://hg.mozilla.org/projects/jamun/rev/17e87306d983

Follow-up:

https://hg.mozilla.org/projects/jamun/rev/105d1330ec49

Comment 15

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

Landed in UX as

https://hg.mozilla.org/projects/ux/rev/17e87306d983

Follow-up:

https://hg.mozilla.org/projects/ux/rev/105d1330ec49

Comment 16

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/344dfeecfd71
https://hg.mozilla.org/mozilla-central/rev/0a7b55de0e88