1391593 - Use same amount of padding vertically as horizontally for bookmark items

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[afnankhan]

Attachment: bookmark bar.PNG

Currently, bookmark items have 4px padding horizontally. Use the same amount of padding vertically.

Comment 1

[afnankhan]

This is for normal density. Changing density should also increase or decrease padding for bookmark items.

Comment 2

[Dão Gottwald [:dao]]

shorlander, please confirm or close as needed.

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Bookmarks Bar Spacing Comparison

The visual spacing is keyed off of the tallest elements (Back button) and should be 4px on top and bottom. So something does look off there.

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1391593 - Increase the bottom padding of the bookmarks toolbar by 1px.

Review commit: https://reviewboard.mozilla.org/r/180876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180876/

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8909296 [details]
Bug 1391593 - Increase the bottom padding of the bookmarks toolbar by 1px.

https://reviewboard.mozilla.org/r/180876/#review186448

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dc160bb6a34
Increase the bottom padding of the bookmarks toolbar by 1px. r=nhnt11

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2dc160bb6a34

Comment 10

[Mehmet]

Attachment: Normal Mode.png

Hello. I took a look at the latest Trunk 57.0a1 (2017-09-21) today and I noticed that the vertically padding still has not the same height.

Please find attached two screenshots showing it in Normal and Compact Mode.

May be adding 1px more padding at the bottom could be good compromise between Normal and Compact Mode. What do you think?

Can you please check this again? Thank you very much in advance :-)

Comment 11

[Mehmet]

Attachment: Compact Mode.png

+ screenshot compact mode

Comment 12

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170926220106

This issue has been verified on latest Firefox Nightly Build ID: 20170926220106 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and it is still reproducible. The padding from the bottom of the bookmarks toolbar is not the same with the padding from the top of it. Confirming the picture from comment 11 .

@Nihanth, can you please take a look at this? Is there anything that can be done here?

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Uh, Johann, could you comment on this? I'm not sure I know what the exact goal was here, I just r+'d the code based on the commit message. :)

Comment 14

[Johann Hofmann [:johannh]]

(In reply to Vlad Bacia-Mociran [:VladB] from comment #12)
> User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101
> Firefox/58.0
> Build ID: 20170926220106
> 
> This issue has been verified on latest Firefox Nightly Build ID:
> 20170926220106 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and it is
> still reproducible. The padding from the bottom of the bookmarks toolbar is
> not the same with the padding from the top of it. Confirming the picture
> from comment 11 .
> 
> @Nihanth, can you please take a look at this? Is there anything that can be
> done here?

It's not supposed to be the same, what are these claims based on? The navbar has some vertical padding at its bottom that adds up to it. Measuring the bookmarks toolbar height relative to elements in the navbar is just not correct.

Comment 15

[Vlad Bacia-Mociran [:VladB]]

(In reply to Johann Hofmann [:johannh] from comment #14)
> It's not supposed to be the same, what are these claims based on? The navbar
> has some vertical padding at its bottom that adds up to it. Measuring the
> bookmarks toolbar height relative to elements in the navbar is just not
> correct.

I was referring to the screenshots from comment 10 and comment 11. Until now nobody said that this is not correct. 

Although, after looking at the mockups, the padding seems to be implemented as requested.

Comment 16

[Johann Hofmann [:johannh]]

(In reply to Vlad Bacia-Mociran [:VladB] from comment #15)
> (In reply to Johann Hofmann [:johannh] from comment #14)
> > It's not supposed to be the same, what are these claims based on? The navbar
> > has some vertical padding at its bottom that adds up to it. Measuring the
> > bookmarks toolbar height relative to elements in the navbar is just not
> > correct.
> 
> I was referring to the screenshots from comment 10 and comment 11. Until now
> nobody said that this is not correct. 

Alright, just to clarify: The assumption that the bookmark toolbar items should have the same distance to the urlbar as to the lower border is not correct.

Comment 17

[Vlad Bacia-Mociran [:VladB]]

(In reply to Johann Hofmann [:johannh] from comment #16)
> (In reply to Vlad Bacia-Mociran [:VladB] from comment #15)
> > (In reply to Johann Hofmann [:johannh] from comment #14)
> > > It's not supposed to be the same, what are these claims based on? The navbar
> > > has some vertical padding at its bottom that adds up to it. Measuring the
> > > bookmarks toolbar height relative to elements in the navbar is just not
> > > correct.
> > 
> > I was referring to the screenshots from comment 10 and comment 11. Until now
> > nobody said that this is not correct. 
> 
> Alright, just to clarify: The assumption that the bookmark toolbar items
> should have the same distance to the urlbar as to the lower border is not
> correct.

Thank you. Then, from my point of view the issue is fixed and I cannot reproduce it (comment 0 and comment 3).

Comment 18

[Brindusa Tot, Desktop QA]

Based on previous comments, mark this bug verified on Firefox 58.

Comment 19

[Alexandru Simonca [:asimonca], Release Desktop QA]

Based on previous comments, marking this bug verified on Firefox 57.