927717 - CustomizeMode _onDragOver does not handle the empty toolbar case

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Aris, the fellow working on the add-ons that restores the add-on bar (along with numerous other things[1]), reported this to me - apparently, dragging to and from his add-on bar is no longer working.

He sent me a test version of his add-on, and I can confirm that it's busted - this appears to be fallout from bug 923439, which while making it easier to add things to the ends of toolbars, also made the assumption that toolbars will have items in them when dragging stuff over them.

Patch coming up.

Comment 1

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

Attachment: Patch v1

Comment 2

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

[1] in comment 0 should have pointed to: http://aris-at-mozilla.blogspot.ca/

Comment 3

[:Gijs (he/him)]

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

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +796,5 @@
>      let dragOverItem, dragValue;
>      if (targetNode == targetArea.customizationTarget) {
> +      // We'll assume if the user is dragging directly over the target, that
> +      // they're attempting to append a child to that target.
> +      dragOverItem = targetNode.lastChild || targetNode;

This is the right idea. Unfortunately, this will put a border-width on the container node and set a dragover attribute and such when _setDragActive gets called further down. We probably don't want to do that, which presumably means making the call to _setDragActive conditional. Other than that, we should really add a test for this so we don't accidentally regress it again. :-(

Comment 4

[Aris]

Thanks Mike for opening this bug and also for the tip with the empty toolbars. I now know how to add a workaround until this issue is solved.

Comment 5

[:Gijs (he/him)]

Attachment: "CustomizeMode _onDragOver does not handle the empty toolbar case" []

So I corrected the patch and wrote a test, but I hit a snag. Which is that our existing way of appending toolbars never worked correctly. I fixed that, but that then caused a separate bug to show up. See the todo(). Which is that if you add a toolbar with a default placement that has no nodes in it, it 'doesn't work'. This is because buildArea doesn't get run for the toolbar: the XBL notices 0 nodes in the toolbar when its constructor runs, and because _init runs immediately afterwards, there are still 0 childNodes, and it therefore calls registerToolbar with aIsOverlayed == false. Which means buildArea doesn't run, the toolbar stays empty instead of being populated with its default children, and the check in that function fails.

It doesn't fail at the moment because the lack of build nodes (the toolbars are hidden in the toolbarset, and therefore don't get XBL-bound, and don't call registerToolbar) means we don't correct the placement array. When the toolbars *are* visible and call registerToolbar, we remove all the items in the placement array that aren't in the toolbar, which means the array is empty and doesn't match the default placements which has 3 items.

I think this usecase should in principle work - if you add a toolbar, buildArea should ensure that the placements match reality, usually by adding nodes to the toolbar or by changing the placements. Does that make sense, or do you disagree?

(for now, I think we should land this fix and file a followup for this new issue)

Comment 6

[:Gijs (he/him)]

Attachment: CustomizeMode _onDragOver does not handle the empty toolbar case,

After discussion with Mike on IRC, this seemed like a worthwhile course to pursue. All tests pass - just need to doublecheck that this won't affect tpaint/ts_paint too much...

Comment 7

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

Comment on attachment 819006 [details] [diff] [review]
CustomizeMode _onDragOver does not handle the empty toolbar case,

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

Ok, yeah, let's try this, and keep an eye on the ts_paint / tpaint numbers. Thanks Gijs!

::: browser/components/customizableui/test/browser.ini
@@ +23,5 @@
>  run-if = os == "mac"
>  
>  [browser_914863_disabled_help_quit_buttons.js]
>  [browser_918049_skipintoolbarset_dnd.js]
> +[browser_927717_customize_drag_empty_toolbar.js]

At one point, I think these tests were ordered by their bug #'s... looking at lines 17-19, I guess that isn't necessarily the case...

I'd put this new line at the bottom of the list. And if you're feeling extra awesome, re-order the tests on lines 17-19 (and maybe further up too) so that they too are ordered by bug number.

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/e20188deb51d

Comment 9

[:Gijs (he/him)]

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