515106 - dragging site icon to bookmark toolbar resizes bookmark toolbar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Asa Dotzler [:asa]]

Attachment: ogg video showing resizing bookmark toolbar height

If you drag the site icon from the addressbar to the bookmarks toolbar, the toolbar height grows 1 pixel.  This seems to be the result of the drop marker height. (Dragging to a folder, where you get no drop marker) on the bookmarks toolbar does not increase the height)

This has been around for a while. I've tested with old and new profiles and with small and large toolbar icons. 

The attached video demonstrates the problem.

Comment 1

[Markus Stange [:mstange]]

I think this has been caused by bug 382466.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the toolbar just needs a min-height: 24px;

btw i have a patch for this in bug 512353

Comment 3

[Markus Stange [:mstange]]

I don't think we want to make the toolbar higher.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

why higher? the drop indicator is 17px, the toolbar has 5px of padding and 2px of border.

Comment 5

[Markus Stange [:mstange]]

I don't understand. The problem in this bug is that the toolbar becomes higher if the drop indicator is visible. And you want to fix it by making the with-dropindicator height (which is higher than normal) the default height?

I think we should rather stop the drop indicator from influencing the height at all.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i could be wrong, but i'd bet the toolbar is now smaller than before bug 382466 due to the scrollbox instead. the drop indicator before was already part of the toolbar and i suspect changing the hbox to a scrollbox has caused us to lose in height.
Plus the fact this change just unifies the threee platforms, os x is the only one that does not have a min-height but is instead using a bunch of paddings to set the toolbar height. I've also seen some situation (prior to to bug 382466) that were causing the toolbars on os x to flicker in height in some situations.
So yes, i think setting a min-height is the simplest, platforms consistent and better option here.
Btw, i agree that comparing the toolbar heights could be a good way to understand if i'mk correct, so i'll do.

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #6)
> i could be wrong, but i'd bet the toolbar is now smaller than before bug 382466

Doesn't look like that to me. The toolbar height in the current nightly is the same as in Firefox 3.5. Maybe it's a rounding issue?

I agree that adding a min-height is the right way to go, as long as it doesn't change the default toolbar height. :-)

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the correct min-height is 23px, not 24px.
the problem is the padding, indeed doing calculation i was getting 24px.
in previous implementation the drop indicator was in a vbox with the toolbar hbox, but its margin was adjusted to a negative value with hacky-magik. the new implementations puts the indicator in the same hbox as the scrollbox, so the padding gets applied to the drop indicator, that is 1px taller.
with a 23px of min-height there are no resizes, and i just compared patched version with a trunk nightly before bug 382466, they have same height.

Comment 9

[Markus Stange [:mstange]]

That explains it. Thanks for looking at this.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

So, dupe of that other bug, or are you taking this, Marco?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

sorry. taking. It's not a dupe, just that i'm working on the same code and is a regression of the same bug, so i'm just adding the min-height on the other patch.
Adding dependancy.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

fixed with bug 512353.