Closed Bug 964204 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29

People

(Reporter: phlsa, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 files, 1 obsolete file)

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
Oh, and the location bar also jumps once an item is dropped back into the toolbar (also visible in the video)
Version: 28 Branch → Trunk
Whiteboard: [Australis:P?] → [Australis:P3]
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.
Attachment #8365868 - Flags: review?(mdeboer)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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?
Attachment #8365868 - Flags: review?(mdeboer) → review+
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
Attachment #8365868 - Flags: checkin+
Leaving open for the toolbar issue, although I could swear that was already filed...
Whiteboard: [Australis:P3] → [Australis:P3][leave open]
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)
Assignee: gijskruitbosch+bugs → nobody
As far as I can tell, this significantly improves the situation. HOowever, it could probably benefit from someone else looking at the outcome.
Attachment #8369435 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
(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)
This should be better as regards both issues.
Attachment #8369478 - Flags: review?(jaws)
Attachment #8369435 - Attachment is obsolete: true
Attachment #8369435 - Flags: review?(jaws)
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.
Attachment #8369478 - Flags: review?(jaws) → review+
(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
Whiteboard: [Australis:P3][leave open] → [Australis:P3]
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d33aff1eaaed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 29
FYI.  Even with this patch the same thing happens with the search bar on the toolbar.
(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!
Flags: needinfo?(garyshap)
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?
Flags: needinfo?(garyshap)
(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?
Yes it looks like my problem might be 924446 not this one.  Sorry.
QA Contact: cornel.ionce
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.