964204 - Customization Mode: Glitch when moving items in the toolbar or the panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

When grabbing an item in the toolbar and moving it slightly, the location bar resizes to take up the space of the moved item and then animates back to its desired position. This makes the experience of reordering items in the toolbar feel glitchy.
There's a similar issue in the panel, where the drop indicators are doing the same thing.

Here are some screen captures showing the problem:
Toolbar: http://cl.ly/253H10192G0a
Panel: http://cl.ly/150U3f3W1112

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Oh, and the location bar also jumps once an item is dropped back into the toolbar (also visible in the video)

Comment 2

[:Gijs (he/him)]

Attachment: fix drag glitches in Australis panel and palette,

This fixes things in the panel/palette, and fixes another issue where dragging from the palette to the panel or vice versa 'snapped' the first time we make space, instead of smoothly animating.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8365868 [details] [diff] [review]
fix drag glitches in Australis panel and palette,

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

This looks good :)

As a side note, dragging items has become a bit choppy; I suspect we're doing a bit too many calculation in JS-land whilst dragging an item. Perhaps it's a good idea to run a profile at some point?

::: browser/components/customizableui/src/DragPositionManager.jsm
@@ +125,5 @@
>     * way. We go through all the children, and shift them based on the position
>     * they would have if we had inserted something before aBefore. We use CSS
>     * transforms for this, which are CSS transitioned.
>     */
> +  insertPlaceholder: function(aContainer, aBefore, aWide, aSize, aIsFromArea) {

nit: the name is a bit non-descriptive... perhaps `aIsFromCurrentArea` or `aIsOnCurrentArea`, depending on which describes this scenario best?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8365868 [details] [diff] [review]
fix drag glitches in Australis panel and palette,

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Comment on attachment 8365868 [details] [diff] [review]
> fix drag glitches in Australis panel and palette,
> 
> Review of attachment 8365868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good :)
> 
> As a side note, dragging items has become a bit choppy; I suspect we're
> doing a bit too many calculation in JS-land whilst dragging an item. Perhaps
> it's a good idea to run a profile at some point?
> 
> ::: browser/components/customizableui/src/DragPositionManager.jsm
> @@ +125,5 @@
> >     * way. We go through all the children, and shift them based on the position
> >     * they would have if we had inserted something before aBefore. We use CSS
> >     * transforms for this, which are CSS transitioned.
> >     */
> > +  insertPlaceholder: function(aContainer, aBefore, aWide, aSize, aIsFromArea) {
> 
> nit: the name is a bit non-descriptive... perhaps `aIsFromCurrentArea` or
> `aIsOnCurrentArea`, depending on which describes this scenario best?

Went with aIsFromThisArea - the dragpositionmanager is per-area. :-)

remote:   https://hg.mozilla.org/integration/fx-team/rev/cdef896777c5

Comment 5

[:Gijs (he/him)]

Leaving open for the toolbar issue, although I could swear that was already filed...

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[:Gijs (he/him)]

I'm not working on the toolbar issue at the moment, if anyone else wants to pick this up. It'll likely need similar work to the panel (ie a notransition attribute to stop it transitioning the first time we make space somewhere in the bar when dragging an item from that area itself, and likewise when an item is dropped)

Comment 8

[:Gijs (he/him)]

Attachment: Australis - glitch when dragging in toolbars,

As far as I can tell, this significantly improves the situation. HOowever, it could probably benefit from someone else looking at the outcome.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #8)
> Created attachment 8369435 [details] [diff] [review]
> Australis - glitch when dragging in toolbars,
> 
> As far as I can tell, this significantly improves the situation. HOowever,
> it could probably benefit from someone else looking at the outcome.

It does improve the situation!
Unfortunately it still seems to have two issues:
1) When dragging an item, the item to the right jumps into the empty space
2) If an item is placed between the location bar an the search field, picking up that item causes the old jumpy behavior (or something that look similar)

Comment 10

[:Gijs (he/him)]

Attachment: Australis - glitch when dragging in toolbars,

This should be better as regards both issues.

Comment 11

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

Comment on attachment 8369478 [details] [diff] [review]
Australis - glitch when dragging in toolbars,

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1453,4 @@
>          aItem.style[prop] = width + 'px';
>          aItem.style.removeProperty(otherProp);
> +        if (makeSpaceImmediately) {
> +          aItem.getBoundingClientRect();

Do you think a comment should be added to say that this is needed to force a layout flush? It may not be obvious to newcomers or people that aren't familiar with the code.

Comment 12

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #11)
> Comment on attachment 8369478 [details] [diff] [review]
> Australis - glitch when dragging in toolbars,
> 
> Review of attachment 8369478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +1453,4 @@
> >          aItem.style[prop] = width + 'px';
> >          aItem.style.removeProperty(otherProp);
> > +        if (makeSpaceImmediately) {
> > +          aItem.getBoundingClientRect();
> 
> Do you think a comment should be added to say that this is needed to force a
> layout flush? It may not be obvious to newcomers or people that aren't
> familiar with the code.

With comments in both places,

remote:   https://hg.mozilla.org/integration/fx-team/rev/d33aff1eaaed

Comment 13

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Excellent, that is so much better!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Gary [:streetwolf52]]

FYI.  Even with this patch the same thing happens with the search bar on the toolbar.

Comment 16

[:Gijs (he/him)]

(In reply to Gary [:streetwolf] from comment #15)
> FYI.  Even with this patch the same thing happens with the search bar on the
> toolbar.

We really need more information than that to go on. I just tried picking up the search bar from the navigation bar, and it works for me (ie, other items don't move). If you can consistently reproduce this, could you please file a new bug (CC me) with more details, such as what OS, what build exactly (there aren't nightlies/aurora builds with this change yet) and what executing this command in the browser console returns:

CustomizableUI.getWidgetIdsInArea("nav-bar");

Thanks!

Comment 17

[Gary [:streetwolf52]]

To be honest I'm not really sure what this patch fixes.  I'm on inbound using Windows 8.1 so I have this patch.  When I am in customization mode and I grab the download arrow for example and move it to the left or right both my location bar and search bar will move a little left or right.  Is this normal behavior?

Comment 18

[:Gijs (he/him)]

(In reply to Gary [:streetwolf] from comment #17)
> To be honest I'm not really sure what this patch fixes.  I'm on inbound
> using Windows 8.1 so I have this patch.  When I am in customization mode and
> I grab the download arrow for example and move it to the left or right both
> my location bar and search bar will move a little left or right.  Is this
> normal behavior?

So, previously, if your download arrow was before the home button, picking up the download arrow would have gotten the home button to abruptly appear underneath it and then animate back to the right. That should be fixed. I can't tell from your comment, and you didn't give any of the info I specifically asked for. "inbound" only got builds that have this patch about an hour and a half ago, are you on those builds? ( https://hg.mozilla.org/integration/mozilla-inbound/rev/ad62bf4d62b4 )


Depending on what you mean by "a little", maybe you're seeing bug 924446?

Comment 19

[Gary [:streetwolf52]]

Yes it looks like my problem might be 924446 not this one.  Sorry.

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.3; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0

Reproduced the issue on 2014-01-28 Nightly. 
Confirming the fix on Firefox 29 beta 6 (build ID: 20140407135746).