Closed Bug 855295 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Hmm, pretty sure this is a new bug (ie, due to work done since November) - which should make it easier to track down.
Assignee: nobody → mconley
Attached patch Patch v1 (obsolete) — Splinter Review
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?
Attachment #730777 - Flags: feedback?(bmcbride)
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).
Attachment #730777 - Flags: feedback?(bmcbride) → feedback-
Comment on attachment 730777 [details] [diff] [review]
Patch v1

Ok, good to know - thanks Blair.
Attachment #730777 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #731970 - Flags: review?(bmcbride)
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.
Attachment #731970 - Flags: review?(bmcbride) → review+
(Whats with people not setting the status these days?)
Status: NEW → ASSIGNED
(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. :)
And thanks for the review!
All fixed up.
Attachment #731970 - Attachment is obsolete: true
Attachment #733332 - Flags: review+
Whiteboard: [fixed in jamun]
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...
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.
Attachment #733499 - Flags: review?(bmcbride)
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.)
Attachment #733499 - Flags: review?(bmcbride) → review+
Landed in UX as

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

Follow-up:

https://hg.mozilla.org/projects/ux/rev/105d1330ec49
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]
https://hg.mozilla.org/mozilla-central/rev/344dfeecfd71
https://hg.mozilla.org/mozilla-central/rev/0a7b55de0e88
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.