Closed Bug 992373 Opened 6 years ago Closed 6 years ago

Items in panel jump up slightly when the customization transition finishes

Categories

(Firefox :: Toolbars and Customization, defect)

31 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- verified

People

(Reporter: phlsa, Assigned: mconley)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 4 obsolete files)

Reproducible on current Nightlies as well as on Aurora and Beta with a clean profile. It affects Windows and OS X (possibly also Linux).

We once had a similar bug that was caused by multiline labels in the panel. This one seems to affect all items.
I'll take a look at this tomorrow.
Assignee: nobody → mconley
This is occurring during the wrapping phase - the toolbarpalleteitem wrappers are causing the buttons in the menu panel to expand.
So it looks like there's a 6px margin-top being applied to .toolbarbutton-1's under panelviews.

I believe the problem here is that both the toolbarpaletteitems and the panelview .toolbarbutton-1's both have the same height setting:

height: calc(51px + 2.2em);

So that's all well and good when toolbarbutton-1's are outside of the toolbarpaletteitems - they don't have their immediate parents constraining their margin-tops.

But when they're the children of the toolbarpaletteitem's, they're inside a thing that's supposed to have the same height as them, and so the margin-top gets squished out.
Comment on attachment 8402798 [details] [diff] [review]
Patch v1

This doesn't remove all of the motion in the panel (I still see some text shifting a little bit...), but it removes the vast majority of it. The shifting is now much more difficult to see with this patch.

Hey mikedeboer - do you see any possible concerns with this patch?
Attachment #8402798 - Flags: review?(mdeboer)
Comment on attachment 8402798 [details] [diff] [review]
Patch v1

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

Hey Mike, I do see that this approach will lead to the solution, but in its current state it has one problem:

I can't drag a wide widget to the last row - which has 3 items in a row - anymore, whereas without this patch applied I can.
IOW, this seems to have a negative impact on the DnD code...

I'll n-i Gijs; he might be able to shed some light on this part.
Attachment #8402798 - Flags: review?(mdeboer) → feedback+
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> Comment on attachment 8402798 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8402798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Mike, I do see that this approach will lead to the solution, but in its
> current state it has one problem:
> 
> I can't drag a wide widget to the last row - which has 3 items in a row -
> anymore, whereas without this patch applied I can.
> IOW, this seems to have a negative impact on the DnD code...
> 
> I'll n-i Gijs; he might be able to shed some light on this part.

Hm - good eye. Let me poke at it.
See also: bug 963773 comment 9, bug 963773 comment 10. If Mike doesn't figure it out here, I'll try to look into it tomorrow.
Flags: needinfo?(gijskruitbosch+bugs)
Uh, this patch of mine also somehow regresses the cursor on the toolbar items. Great.
Attached patch WIP Patch v1.1 (obsolete) — Splinter Review
Ok, I think I've figured this one out. Finally.
Attachment #8402798 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
Ok, this seems to address mikedeboer's concerns.

Gijs - would you mind making sure I didn't screw things up in DragPositionManager.jsm? I had no idea that thing even existed. It was a fun surprise. :)
Attachment #8403641 - Attachment is obsolete: true
Attachment #8403645 - Flags: review?(mdeboer)
Attachment #8403645 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8403645 [details] [diff] [review]
Patch v1.1

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

::: browser/components/customizableui/src/DragPositionManager.jsm
@@ +340,5 @@
>      return rect;
>    },
>  
>    _firstInRow: function(aNode) {
> +    if (this.isWide(aNode)) {

Is this just a fast-path? Like, is this really necessary? We can keep it, but I'd like to understand if it's necessary or just faster. If it's necessary, I don't immediately understand why and would like help with that. :-)

Comment about the above wouldn't hurt (I've tried, at the request of the /other/ Mike to comment this module rather heavily... you be the judge of whether that helped in understanding your surprise! ;-) )

@@ +344,5 @@
> +    if (this.isWide(aNode)) {
> +      return aNode;
> +    }
> +
> +    let bound = Math.floor(this._lazyStoreGet(aNode).top);

Likewise... why is this necessary? The code here is essentially already claiming "you are the first in a row if your previous visible sibling's bottom bound is lower than your top bound" which shouldn't really be problematic. Is this just because if I have two subsequent overlapping (because top margin -1) wide nodes, that confuses this code, and the floor'ing gets around that? (not really sure because this wasn't the case that the other Mike asked about)

I might almost want to come up with a better definition of "in the same row" in that case, instead of this, which seems like it'll break if we start using e.g. 2px negative margin bottom instead of 1. :-)

Maybe I just don't understand! It seems I forgot to comment these methods, and it is past midnight here, so... consider these questions with some salt. :-)
Comment on attachment 8403645 [details] [diff] [review]
Patch v1.1

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

r=me once you were able to convince Gijs about the DragnDrop code changes :)

Tested it and looks mighty-fine! Thanks!
Attachment #8403645 - Flags: review?(mdeboer) → review+
Attached patch Patch v1.2 (r+'d by mikedeboer) (obsolete) — Splinter Review
(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8403645 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8403645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/DragPositionManager.jsm
> @@ +340,5 @@
> >      return rect;
> >    },
> >  
> >    _firstInRow: function(aNode) {
> > +    if (this.isWide(aNode)) {
> 
> Is this just a fast-path? Like, is this really necessary? We can keep it,
> but I'd like to understand if it's necessary or just faster. If it's
> necessary, I don't immediately understand why and would like help with that.
> :-)
> 
> Comment about the above wouldn't hurt (I've tried, at the request of the
> /other/ Mike to comment this module rather heavily... you be the judge of
> whether that helped in understanding your surprise! ;-) )

It turns out this is working around an actual bug where the live DOMRects were being updated in the cache, instead of being totally frozen as expected. I've fixed that and added a comment.

> 
> @@ +344,5 @@
> > +    if (this.isWide(aNode)) {
> > +      return aNode;
> > +    }
> > +
> > +    let bound = Math.floor(this._lazyStoreGet(aNode).top);
> 
> Likewise... why is this necessary? The code here is essentially already
> claiming "you are the first in a row if your previous visible sibling's
> bottom bound is lower than your top bound" which shouldn't really be
> problematic. Is this just because if I have two subsequent overlapping
> (because top margin -1) wide nodes, that confuses this code, and the
> floor'ing gets around that? (not really sure because this wasn't the case
> that the other Mike asked about)
> 
> I might almost want to come up with a better definition of "in the same row"
> in that case, instead of this, which seems like it'll break if we start
> using e.g. 2px negative margin bottom instead of 1. :-)
> 

I also don't really understand, I'm afraid - when doing the comparisons, I periodically saw fractional differences between the tops and the bottoms, and this caused us to return an incorrect node as the first element in the row.

I could dig into it deeper, but I'd rather spend more time fixing bugs. Lemme know if you want me to deep dive.
Attachment #8403645 - Attachment is obsolete: true
Attachment #8403645 - Flags: review?(gijskruitbosch+bugs)
Attachment #8404109 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8404109 [details] [diff] [review]
Patch v1.2 (r+'d by mikedeboer)

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

Sweet, thanks!

::: browser/components/customizableui/src/DragPositionManager.jsm
@@ +336,5 @@
> +      // getBoundingClientRect() returns a DOMRect that is live, meaning that
> +      // as the element moves around, the rects values change. We don't want
> +      // that - we want a snapshot of what the rect values are right at this
> +      // moment, and nothing else. So we have to clone the values.
> +      let clientRect = aNode.getBoundingClientRect();

Nit: I would prefer:

rect = {
  left: clientRect.left, right: clientRect.right,
  ...
}

@@ +352,5 @@
>      return rect;
>    },
>  
>    _firstInRow: function(aNode) {
> +    let bound = Math.floor(this._lazyStoreGet(aNode).top);

Hrmpf. Add a comment above and file a followup bug, I guess? I agree that it's not the most important right now, I guess.
Attachment #8404109 - Flags: review?(gijskruitbosch+bugs) → review+
Filed bug 994247.

Also noticed that attempting to move a dangling item at the bottom acts funny when the panel is scrollable. *sigh* - looking at that next.
Attached patch Patch v1.3Splinter Review
Last change I swear - this just clears the DragPositionManager's on drop.

So there's still a bug where attempting to drag items from the last row into the previous row can be tough if the panel is scrollable, but that appears to be the case from before this patch anyways. I'd rather not try to fix the world here. :)
Attachment #8404109 - Attachment is obsolete: true
Attachment #8404158 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8404158 [details] [diff] [review]
Patch v1.3

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

Nice!

::: browser/components/customizableui/src/DragPositionManager.jsm
@@ +353,5 @@
>      return rect;
>    },
>  
>    _firstInRow: function(aNode) {
> +    // I'm not entirely sure why we need to take the floor of these values -

Nit: if you're gonna go all personal, state your name and business, sir!

(//xxxmconley or whatever)
Attachment #8404158 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks! Added my name to the comment. :)

remote:   https://hg.mozilla.org/integration/fx-team/rev/81ff415ace94
Status: NEW → ASSIGNED
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/81ff415ace94
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Comment on attachment 8404158 [details] [diff] [review]
Patch v1.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

The menu panel items will move a bit once the transition completes, which makes the transition less smooth. Also, this patch makes it easier to drag wide-items between other items.


Testing completed (on m-c, etc.): 

Lots of local testing, and a few nights on m-c.


Risk to taking this patch (and alternatives if risky): 

I'd say pretty low.


String or IDL/UUID changes made by this patch:

None.
Attachment #8404158 - Flags: approval-mozilla-beta?
Attachment #8404158 - Flags: approval-mozilla-aurora?
Attachment #8404158 - Flags: approval-mozilla-beta?
Attachment #8404158 - Flags: approval-mozilla-beta+
Attachment #8404158 - Flags: approval-mozilla-aurora?
Attachment #8404158 - Flags: approval-mozilla-aurora+
Flags: in-testsuite?
Keywords: verifyme
Flags: in-testsuite? → in-testsuite-
Flags: in-qa-testsuite?
Verified fixed on Windows 7, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using Firefox 31.0b4 (buildID: 20140623175014)
Status: RESOLVED → VERIFIED
Depends on: 1092147
You need to log in before you can comment on or make changes to this bug.