909729 - Dragging bookmark items to navbar results in dis-functioning bookmark items

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike de Boer [:mikedeboer]]

STR:
1. Open Customization mode
2. Drag the toolbar items from the bookmarks toolbar to the navbar
3. Exit Customization mode
4. Click on a Bookmarks (smart) folder

Expected result:
The folder panels open, showing the bookmark items within

Actual result:
No folder panel opens.

Warning shown in the console:
JavaScript strict warning: chrome://browser/content/browser.xul, line 1: reference to undefined property this._placesView

Comment 1

[Mike de Boer [:mikedeboer]]

Marco, do you think this is related to bug 895938 at all and if a similar solution would be appropriate?

Comment 2

[Marco Bonardo [:mak]]

I don't think it's related explicitly to the change, could just be a missing piece of code handgling a special case. The code to touch is very likely the same.

Comment 3

[:Gijs (he/him)]

This is essentially the same as bug 890567, which shouldn't have been duped to bug 895938.

Comment 4

[Mike de Boer [:mikedeboer]]

Marco, any progress you can report here?

Comment 5

[:Gijs (he/him)]

Attachment: bookmark toolbar items don't work in the navbar,

I looked at this briefly. It's because the navbar's customization target isn't a toolbar. Detecting that it is seemed a bit tricky to me... I've added code that just walks the parentNode chain, which will work assuming people don't do really weird things like adding toolbars with a customizationTarget that's not a descendant of that toolbar... This also fixes bug 901418. Marco, does this look OK to you?

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 807132 [details] [diff] [review]
bookmark toolbar items don't work in the navbar,

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

::: browser/base/content/browser-places.js
@@ +964,5 @@
>      // there is no need to initialize the toolbar if customizing because
>      // init() will be called when the customization is done.  Finally the view
>      // is hidden if the element is not on a toolbar.
> +    let container = viewElt.parentNode.parentNode;
> +    if (!this._isInToolbar(container) || container.collapsed ||

thanks for looking into this.

Are you sure the container.collapsed works? it worked cause we set collapsed on toolbars, I don't think we set it on the customization target...
This also doesn't seem to cover the second part of the comment, that is "skip initialization during customization mode")
You may want something like let toolbar = this._getParentToolbar(viewElt); that returns either the toolbar or null, though you also need to add an additional check for whether we are in customization mode.

Comment 7

[:Gijs (he/him)]

Attachment: bookmark toolbar items don't work in the navbar,

That makes sense. Updated to look for the toolbar recursively instead. Note that the ._isCustomizing check was still there in the original patch. I now moved it about to make the if condition shorter.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 807705 [details] [diff] [review]
bookmark toolbar items don't work in the navbar,

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

::: browser/base/content/browser-places.js
@@ +963,5 @@
> +    // - not in a toolbar, or;
> +    // - the toolbar is collapsed, or;
> +    // - the toolbar is hidden some other way:
> +    // don't initialize.  Also, there is no need to initialize the toolbar if
> +    // customizing because this will happen when the customization is done.

comma after customizing and s/this/that/?

@@ +964,5 @@
> +    // - the toolbar is collapsed, or;
> +    // - the toolbar is hidden some other way:
> +    // don't initialize.  Also, there is no need to initialize the toolbar if
> +    // customizing because this will happen when the customization is done.
> +    let toolbar = this._getParentToolbar(viewElt.parentNode.parentNode);

may simplify by not providing .parentNode.parentNode, the only added cost is checking a couple localName, shouldn't matter.

Oh, I missed the _isCustomizing check in the middle of splinter, sorry

@@ +996,5 @@
>        PlacesCommandHook.showPlacesOrganizer("BookmarksToolbar");
>      }
> +  },
> +
> +  _getParentToolbar: function(node) {

since in Places we have 2 concept of nodes (dom nodes and places nodes) please use element or elt here

Comment 9

[:Gijs (he/him)]

Landed with nits fixed: https://hg.mozilla.org/projects/ux/rev/8d12999288a9

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/8d12999288a9