Closed
Bug 1391593
Opened 8 years ago
Closed 7 years ago
Use same amount of padding vertically as horizontally for bookmark items
Categories
(Firefox :: Theme, defect, P1)
Tracking
()
People
(Reporter: afnankhan, Assigned: johannh)
References
Details
(Whiteboard: [reserve-photon-visual])
Attachments
(5 files)
Currently, bookmark items have 4px padding horizontally. Use the same amount of padding vertically.
This is for normal density. Changing density should also increase or decrease padding for bookmark items.
Updated•8 years ago
|
Whiteboard: [photon-visual][triage]
Comment 2•7 years ago
|
||
shorlander, please confirm or close as needed.
Blocks: 1388676
Flags: needinfo?(shorlander)
Priority: -- → P5
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Comment 3•7 years ago
|
||
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.
Flags: needinfo?(shorlander)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jhofmann
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Updated•7 years ago
|
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: P5 → P1
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8909296 [details]
Bug 1391593 - Increase the bottom padding of the bookmarks toolbar by 1px.
https://reviewboard.mozilla.org/r/180876/#review186448
Attachment #8909296 -
Flags: review?(nhnt11) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2dc160bb6a34
Increase the bottom padding of the bookmarks toolbar by 1px. r=nhnt11
Comment 9•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 10•7 years ago
|
||
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•7 years ago
|
||
+ screenshot compact mode
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Comment 12•7 years ago
|
||
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?
Flags: needinfo?(nhnt11)
Comment 13•7 years ago
|
||
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. :)
Flags: needinfo?(nhnt11) → needinfo?(jhofmann)
Assignee | ||
Comment 14•7 years ago
|
||
(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.
Flags: needinfo?(jhofmann)
Comment 15•7 years ago
|
||
(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.
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 16•7 years ago
|
||
(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.
Flags: needinfo?(jhofmann)
Comment 17•7 years ago
|
||
(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•7 years ago
|
||
Based on previous comments, mark this bug verified on Firefox 58.
status-firefox58:
--- → verified
Comment 19•7 years ago
|
||
Based on previous comments, marking this bug verified on Firefox 57.
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•