1401494 - Buttons in Bookmarks Toolbar are height compacted/squeezed up after landing patch from bug #1389966

Status: VERIFIED DUPLICATE of bug 1401523

(Firefox :: Toolbars and Customization, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Enable Bookmarks Toolbar
and see that buttons there are height compacted/squeezed up

Comment 1

[:Gijs (he/him)]

Screenshot? :-)

Comment 2

[:Gijs (he/him)]

FWIW, I expect this is from bug 1389966.

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: good Compact Density.png

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: good Normal Density.png

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bad Compact Density.png

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bad Normal Density.png

Comment 7

[:Gijs (he/him)]

The "good" screenshots look more squashed / having smaller height than the "bad" ones... am I missing something?

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to :Gijs from comment #1)
> Screenshot? :-)

I was working on it, and now it's done, please have a look. :)


(In reply to :Gijs from comment #2)
> FWIW, I expect this is from bug 1389966.

I also suspect that it's regression caused by bug #1389966,
but mozregression GUI fails in this case,
I'm reporting now bug about it.


(In reply to :Gijs from comment #7)
> The "good" screenshots look more squashed / having smaller height than the
> "bad" ones... am I missing something?

Ups, my bad, fixed.

Comment 9

[:Gijs (he/him)]

So, especially in compact density, I think this is the desired outcome. Johann, did you base the implementation on a design somewhere? Who from UI/UX could weigh in on what we 'should' be doing? :-)

Comment 10

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #8)
> (In reply to :Gijs from comment #2)
> > FWIW, I expect this is from bug 1389966.
> 
> I also suspect that it's regression caused by bug #1389966,
> but mozregression GUI fails in this case,
> I'm reporting now bug about it.

FYI - I reported this issue in bug #1401503

Comment 11

[Johann Hofmann [:johannh]]

These buttons are growing with the bookmarks toolbar height, we can't have "don't grow the toolbar when there's a button" and "don't squeeze buttons to the toolbar height" at the same time. Considering that we accepted that bug 1389966 was the wrong situation, I'm going to close this as intended.

Comment 12

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bad Compact Density - button hover.png

(In reply to :Gijs from comment #9)
> So, especially in compact density, I think this is the desired outcome.
> Johann, did you base the implementation on a design somewhere? Who from
> UI/UX could weigh in on what we 'should' be doing? :-)

Hmm, so some tuning to the button width and to the size between buttons will be also needed, as it was left unchanged and looks really bad and out of place now.

Comment 13

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Johann Hofmann [:johannh] from comment #11)
> These buttons are growing with the bookmarks toolbar height, we can't have
> "don't grow the toolbar when there's a button" and "don't squeeze buttons to
> the toolbar height" at the same time. Considering that we accepted that bug
> 1389966 was the wrong situation, I'm going to close this as intended.

It's completely wrong approach to fix that bug,
as bug #1389966 was only about making Bookmark Toolbar height permanent,
so the same size of Bookmark Toolbar with some button and without any button,
so I'm reopening this issue.



@  Eddward - As OP of bug #1389966, did I understand properly it or I'm misunderstanding it?

Comment 14

[Johann Hofmann [:johannh]]

I'm not going to engage in a bug status war here, I'm just going to say that there are specs for the bookmarks toolbar height that we have to adhere to, which makes it irrelevant what you're reading into bug 1389966. The bookmarks toolbar has (approximately) the correct height now, with or without buttons, and that is not up for discussion.

Comment 15

[Edo Karabas]

Well, it was completely right approach to fix that bug. There is no other option. Buttons have to be squezed if there are any, and to be honest Bookmarks toolbar first goal is not to show buttons but bookmarks.
The size of the Bookmarks toolbar has to be fixed no matter if there are some buttons or not. Before Bug 1377184 landed it was the standard behaviour for a long time. Bug 1389966 only fixed that regression.
So this is WONTFIX.
Next step is bug 1388794 and that's all regardig dimensions.

Comment 16

[:Gijs (he/him)]

Then I think this should be duped to bug 1388794. Any changes to the height of whatever is in the toolbar will have to happen there.

Comment 17

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@  Johann Hofmann [:johannh] &  Eddward & :Gijs - OK, thank you very much for replys!
I will wait patiently for fixing bug #1388794.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Johann Hofmann [:johannh] from comment #14)
> The
> bookmarks toolbar has (approximately) the correct height now, with or
> without buttons, and that is not up for discussion.

FWIW, on Win10 it's pretty much unusable now. Bug 1388794, or some part of it, should make the cut to 57.

Comment 19

[Edo Karabas]

I would say it's more wontfix than dupe. Don't think there is much to discuss. The height should be only one for each density option. Whatever is in the toolbar has to adapt to that size otherwise it will be full of inconsistencies. 
If someone feels that the buttons are too small, than there is a density option for that reason. Second option is to put buttons on the Navigation bar.
Mockups are here if someone want to compare: 
http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html (enable Bookmarks bar and select Density)
Bug 1388794 will change dimensions, but that will by only a few pixels up and down, nothing major.

Comment 20

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Marco Bonardo [::mak] from comment #18)
> (In reply to Johann Hofmann [:johannh] from comment #14)
> > The
> > bookmarks toolbar has (approximately) the correct height now, with or
> > without buttons, and that is not up for discussion.
> 
> FWIW, on Win10 it's pretty much unusable now. Bug 1388794, or some part of
> it, should make the cut to 57.

Agreed.



(In reply to Eddward from comment #19)
> I would say it's more wontfix than dupe. Don't think there is much to
> discuss. The height should be only one for each density option. Whatever is
> in the toolbar has to adapt to that size otherwise it will be full of
> inconsistencies. 
> If someone feels that the buttons are too small, than there is a density
> option for that reason. Second option is to put buttons on the Navigation
> bar.
> Mockups are here if someone want to compare: 
> http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html
> (enable Bookmarks bar and select Density)
> Bug 1388794 will change dimensions, but that will by only a few pixels up
> and down, nothing major.

Maybe now it looks like the same like on specs,
but the point is that it looks wrong and inconsistent, like shambles and clutter.
Just compare button size in Address Bar and Bookmark Toolbar.



As bug #1388794 was marked as RESOLVED, I'm reopening this bug again, as issue stands.
What's more, bug #1389966 is breaking #1368580.

Comment 21

[:Gijs (he/him)]

There is already an r+'d patch in bug 1401523.

Comment 22

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

OK. Thank you very much for information.