880382 - When customizing, dragging wide widgets in the panel should cause panel to break by rows

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

See:  http://cl.ly/image/1A2F420F272b

The middle result is what we should get when dragging a wide widget. The image on the right of the screenshot should not be possible.

Comment 1

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

Sounds good, thank you for filing this!

Comment 2

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

I implemented a solution to this that adjusts where the wide widgets are dropped, but it breaks if user removes/moves/adds widgets to the panel.

I'm thinking that the way we should handle it is:
1) Store how many narrow buttons precede each wide widget.
2) When we build the panel, keep placing narrow buttons until we have reached enough to place a wide widget.
3) If we run out of narrow widgets, place the rest of the wide widgets.

I'm not sure how much of the storage will have to change to support this, but I'm starting to look in to it.

Comment 3

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

Attachment: WIP Patch

This is what I've got so far. What do you think about the approach? I think it's pretty nice that the wide widgets can do this to take care of it themselves basically. 

The problem with this patch, and you'll see it if you run the test upon exiting customization mode, is that we currently only unwrap the dragged item and the destination item. When we need to move other items in the panel, since they're not unwrapped, we end up pulling the unwrapped item out of the wrapper and things start to fall apart.

Comment 4

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

Comment on attachment 762853 [details] [diff] [review]
WIP Patch

Mike and I talked offline about this and I'm going to try something different here that might make this quite a bit simpler :-)

Comment 5

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

Attachment: Patch (part 1)

This patch correctly handles dragging wide widgets in all variations that I could think of for the panel. It also handles modifications of narrow widgets that will affect the placement of the wide widgets, as well as additions and removals to the panel.

Comment 6

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

Attachment: Patch (part 2)

This patch refactors _onDragDrop to remove all of the duplication that we are doing when we are about to return from the function. By moving all of the plumbing out to a separate function, we can handle all of the setup before calling that function, and then all of the cleanup afterwards.

When looking at the diff you'll see that I was able to clean up _onDragDrop quite a bit.

Comment 7

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

Hey Jared, I asked this in IRC, but you may have lost your connection before you saw it:

Why do we insert the wide item above a row of 1,2,3 when dropped on 1, but below when dropped on 2 or 3?

Comment 8

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

(In reply to Mike Conley (:mconley) from comment #7)
> Hey Jared, I asked this in IRC, but you may have lost your connection before
> you saw it:
> 
> Why do we insert the wide item above a row of 1,2,3 when dropped on 1, but
> below when dropped on 2 or 3?

This is simply because we usually insert before the item that it is dropped on. I agree that this would be confusing to users. I can fix that in the next revision of this patch if that's all that needs fixing.

Comment 9

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

Comment on attachment 763131 [details] [diff] [review]
Patch (part 1)

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

Thanks for your patience while I grokked this patch. :) This looks mostly OK. I mean, I think it's a bit of a hacky solution, but it's probably our best option given our time constraints, and it'll do until we come up with something better.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +32,5 @@
>      }
>    }
>  }
>  
> +function getPanelChildForNode(aNode) {

We haven't been doing a great job of documentation the functions we've been writing. Maybe now's a good time to start. Can you please write documentation for this function and adjustPosition?

@@ +63,5 @@
> +        nextElement.classList.contains(kWidePanelItemClass)) {
> +      return;
> +    }
> +
> +    let position = [].indexOf.call(aNode.parentNode.children, aNode);

Clever - but I think this might be clearer:

Array.indexOf(aNode.parentNode.children, aNode);

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Great tests - but you're breaking the 80 char limit in a few places.

Comment 10

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

Comment on attachment 763132 [details] [diff] [review]
Patch (part 2)

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

I'm also OK with this - though, behaviour-wise, remember I mentioned that dropping a wide widget onto 1,2,3 goes above when dropped on 1, but below when dropped on 2 or 3...we should fix that. So r-'ing just on that.

Comment 11

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

Removing the items from M7 that do not block us from landing on m-c.

Comment 12

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

Attachment: Patch v2

Alright, this gets the behavior as we want.

There is one behavior that is not properly handled with this patch, but I'd like to land it as is since the case doesn't break the panel. I've covered the case by a todo in the tests. It just deals with dropping a narrow widget on top of a wide widget.

Comment 13

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

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

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

I think I'm happy with this. Like I mention in the review, I think this is a reasonable stop-gap until we can get CustomizableUI to handle wrapped widgets, which should make this all so much simpler.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +38,5 @@
> +// buttons from being placed in a row by themselves.
> +function adjustPosition(aNode) {
> +  // TODO: Merge this constant with the one in CustomizeMode.jsm,
> +  //       maybe just use a pref for this.
> +  const kColumnsInMenuPanel = 3;

Can you file a bug to merge these constants please?

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +505,5 @@
>        }
>      }.bind(this));
>    },
>  
> +  // TODO: Remove once CustomizeUI can handle moving wrapped widgets.

Can you file a bug for this please?

@@ +776,5 @@
>  
> +    this._setDragActive(this._dragOverItem, false);
> +    this._removePanelCustomizationPlaceholders();
> +
> +    this._unwrapToolbarItemsSync();

Let's put a comment in here as well that this total unwrapping should be removed once CustomizableUI can handle moving wrapped widgets.

Performance-wise, this is a slight nightmare, as replaceChild (called when wrapping or unwrapping) causes a sync reflow. Can't wait until CustomizableUI can handle wrapped stuff.

@@ +1026,2 @@
>      let visibleChildren = contents.querySelectorAll("toolbarpaletteitem:not([hidden])");
> +    // TODO: Still doesn't handle an empty whole when there is a wide widget located

"empty whole"? :)

@@ +1065,5 @@
>    return place;
>  }
>  
>  function __dumpDragData(aEvent, caller) {
> +  return;

Is __dumpDragData still useful? If so, we should remove this early return. If not, we should remove it (or file a bug to remove it). But having this trailing dead code is no good.

Comment 14

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

https://hg.mozilla.org/projects/ux/rev/48334123e0c7

Filed bug 885574, bug 885575, bug 885578, and bug 885579 for follow-ups.

Comment 15

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

This got backed out due to test timeout perma-oranges:

See: https://tbpl.mozilla.org/?tree=UX&rev=48334123e0c7

Back out: https://hg.mozilla.org/projects/ux/rev/f9405d636cbf

Comment 16

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

Relanded with a longer timeout, https://hg.mozilla.org/projects/ux/rev/49e160b46dad

On a side note, we really need to fix our test harness in head.js. Our simple test runner doesn't let TestRunner know when the subtests begin and complete, so TestRunner._currentTestStartTime is never updated. Without fixing this, legitimate tests that just have lots of subtests will begin to fail once someone adds one-too-many subtests.

I looked in to fixing this a simple way by just resetting TestRunner._currentTestStartTime before |yield test.run();| but TestRunner is not defined in that scope (maybe it uses a different name or there is a different way to access it?).

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/49e160b46dad