1369415 - Add 1px margin below bookmark item

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[afnankhan]

Attachment: bookmark.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170601030206

Comment 1

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

Attachment: Fx.png

There is 1px margin below bookmark item, or I'm missing something?

Comment 2

[afnankhan]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #1)
> Created attachment 8873817 [details]
> Fx.png
> 
> There is 1px margin below bookmark item, or I'm missing something?

That's border.
http://imgur.com/a/TXW8q
In the image I have set padding-bottom: 1px to #PersonalToolbar

Also in photon mockup if you enable bookmark bar and hover bookmark item you can see there is 1px margin below it
https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html

Comment 3

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

Oh, yes, you're right. I can confirm this.

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

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

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review166754

::: browser/themes/windows/browser.css:163
(Diff revision 1)
>  
>  #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
>    overflow: -moz-hidden-unscrollable;
>    max-height: 4em;
>    transition: min-height 170ms ease-out, max-height 170ms ease-out;
> -  padding: 0 5px;
> +  padding: 0 5px 1px;

Can we make this consistent across platforms?

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161456/diff/1-2/

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review166754

> Can we make this consistent across platforms?

I updated the patch to make all bookmark toolbars look like shorlanders spec.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review167302

::: browser/themes/shared/browser.inc.css:43
(Diff revision 2)
>  %endif
>  
> +/* Bookmark toolbar */
> +
> +#personal-bookmarks {
> +  min-height: 19px;

Is this still needed at all?

::: browser/themes/shared/browser.inc.css:50
(Diff revision 2)
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
> +  overflow: -moz-hidden-unscrollable;
> +  max-height: 4em;
> +  transition: min-height 170ms ease-out, max-height 170ms ease-out;
> +  padding: 0 5px 3px;

Unless there's a specific reason to have more horizontal padding, let's keep it at 4px like it was on mac and linux.

::: browser/themes/shared/browser.inc.css:50
(Diff revision 2)
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
> +  overflow: -moz-hidden-unscrollable;
> +  max-height: 4em;
> +  transition: min-height 170ms ease-out, max-height 170ms ease-out;
> +  padding: 0 5px 3px;

1px at the bottom is sufficient I think.

::: browser/themes/shared/toolbarbuttons.inc.css:466
(Diff revision 2)
>  
>  /* ::::: bookmark buttons ::::: */
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton) {
>    margin: 0;
> -  padding: 2px 3px;
> +  padding: 0 4px;

The extra horizontal padding means that fewer items might fit on the screen. Users are sensitive to that, so let's not do this.

I also think we want to keep some vertical padding. 2px looks fine over here with the min-height removed.

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review167302

> Is this still needed at all?

Hm, I thought it was needed for the animation, but it still works without, so let's drop it.

> Unless there's a specific reason to have more horizontal padding, let's keep it at 4px like it was on mac and linux.

Right, the spec says the same, I missed that.

> 1px at the bottom is sufficient I think.

The spec says 2px, I'm doing 3px because it includes the bottom border of the toolbar for some reason.

> The extra horizontal padding means that fewer items might fit on the screen. Users are sensitive to that, so let's not do this.
> 
> I also think we want to keep some vertical padding. 2px looks fine over here with the min-height removed.

Both of these are according to spec AFAICS (https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/237332831). Feel free to take that to shorlander :)

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161456/diff/2-3/

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review167362

::: browser/themes/shared/browser.inc.css:46
(Diff revision 3)
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
> +  overflow: -moz-hidden-unscrollable;
> +  max-height: 4em;
> +  transition: min-height 170ms ease-out, max-height 170ms ease-out;
> +  padding: 0 4px 3px;

I can't tell from the invision mockup what the bottom padding is supposed to be, but in the interactive mockup it's 1px and I think that's preferable. The bottom border does not take away from the padding over here on Linux.

::: browser/themes/shared/toolbarbuttons.inc.css:466
(Diff revision 3)
>  
>  /* ::::: bookmark buttons ::::: */
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton) {
>    margin: 0;
> -  padding: 2px 3px;
> +  padding: 0 4px;

That horizontal padding is actually not part of the spec. The spec has a 4px _margin_ between the buttons, but let's make that part of this bug either.

::: browser/themes/shared/toolbarbuttons.inc.css:468
(Diff revision 3)
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton) {
>    margin: 0;
> -  padding: 2px 3px;
> +  padding: 0 4px;
>    -moz-appearance: none;
> +  height: 18px;

Please use vertical padding instead of setting an explicit height so that this works with different font sizes.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161456/diff/3-4/

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review167362

> I can't tell from the invision mockup what the bottom padding is supposed to be, but in the interactive mockup it's 1px and I think that's preferable. The bottom border does not take away from the padding over here on Linux.

It says 2px in invision. It's also 2px in the interactive mockup, I sent you a screenshot on IRC. The bottom border thing is indeed only happening on OSX, so I'll get this down to 2px. It still looks fine on Mac.

> That horizontal padding is actually not part of the spec. The spec has a 4px _margin_ between the buttons, but let's make that part of this bug either.

The horizontal padding is both part of the spec and defined in the interactive mockup. Please check again. The spec indeed also says 4px margin, I got confused by that. I updated the patch and will needinfo shorlander about your concern.

> Please use vertical padding instead of setting an explicit height so that this works with different font sizes.

I set min-height, which hopefully works as well. I think that's more correct since this is about making sure the height doesn't shrink on OSX (which it does for some reason).

Comment 14

[Johann Hofmann [:johannh]]

Stephen, we're a bit concerned that the new bookmark bar design adds a lot of horizontal empty space, moving more bookmark items to the overflow. I'd suggest getting rid of the margin between the bookmark items (we've not had any before). What do you think?

Comment 15

[Dão Gottwald [:dao]]

Attachment: bmtoolbar.png

(In reply to Johann Hofmann [:johannh] from comment #13)
> > I can't tell from the invision mockup what the bottom padding is supposed to be, but in the interactive mockup it's 1px and I think that's preferable. The bottom border does not take away from the padding over here on Linux.
> 
> It says 2px in invision. It's also 2px in the interactive mockup, I sent you
> a screenshot on IRC. The bottom border thing is indeed only happening on
> OSX, so I'll get this down to 2px. It still looks fine on Mac.

I don't know where that screenshot is from, here's what I see.

> > That horizontal padding is actually not part of the spec. The spec has a 4px _margin_ between the buttons, but let's make that part of this bug either.
> 
> The horizontal padding is both part of the spec and defined in the
> interactive mockup. Please check again. The spec indeed also says 4px
> margin, I got confused by that. I updated the patch and will needinfo
> shorlander about your concern.

Just drop the controversial parts and file another bug? The scope creep here doesn't seem useful.

> > Please use vertical padding instead of setting an explicit height so that this works with different font sizes.
> 
> I set min-height, which hopefully works as well. I think that's more correct
> since this is about making sure the height doesn't shrink on OSX (which it
> does for some reason).

I don't understand what exactly that means, but the vertical padding ensures that what's inside a bookmark item isn't too close to the button's edges, i.e. that the button doesn't look crammed. Neither height nor min-height do that.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review167804

::: browser/themes/shared/browser.inc.css:46
(Diff revision 4)
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
> +  overflow: -moz-hidden-unscrollable;
> +  max-height: 4em;
> +  transition: min-height 170ms ease-out, max-height 170ms ease-out;
> +  padding: 0 6px 2px;

Please revert the horizontal padding to 4px and file a new bug on extending it. For the vertical padding, see my screenshot.

::: browser/themes/shared/toolbarbuttons.inc.css:465
(Diff revision 4)
>  }
>  
>  /* ::::: bookmark buttons ::::: */
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton) {
> -  margin: 0;
> +  margin: 0 2px;

Please revert this.

::: browser/themes/shared/toolbarbuttons.inc.css:466
(Diff revision 4)
>  
>  /* ::::: bookmark buttons ::::: */
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton) {
> -  margin: 0;
> -  padding: 2px 3px;
> +  margin: 0 2px;
> +  padding: 0 4px;

ditto

Comment 17

[Stephen Horlander [:shorlander]]

(In reply to Johann Hofmann [:johannh] from comment #14)
> Stephen, we're a bit concerned that the new bookmark bar design adds a lot
> of horizontal empty space, moving more bookmark items to the overflow. I'd
> suggest getting rid of the margin between the bookmark items (we've not had
> any before). What do you think?

The goal is to make sure every item group (Icon + Label) has visually a distinct grouping. Right now they are all running together. That's going to require a trade-off for some horizontal space.

I played around with smaller numbers but it wasn't enough when dealing with a lot of elements that are occupying an already dense space.

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161456/diff/4-5/

Comment 19

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #15)
> > > Please use vertical padding instead of setting an explicit height so that this works with different font sizes.
> > 
> > I set min-height, which hopefully works as well. I think that's more correct
> > since this is about making sure the height doesn't shrink on OSX (which it
> > does for some reason).
> 
> I don't understand what exactly that means, but the vertical padding ensures
> that what's inside a bookmark item isn't too close to the button's edges,
> i.e. that the button doesn't look crammed. Neither height nor min-height do
> that.

I don't think I actually need the min-height at all now. Also, the bookmark buttons center and pad their content with these CSS rules, but I'm not 100% sure what makes them do it.

> Please revert the horizontal padding to 4px and file a new bug on extending it. For the vertical padding, see my screenshot.

Since Stephen is okay with it I'd like to go ahead and add the horizontal space in this patch. I should note that the compact mode features a smaller bookmarks toolbar, I'll create a bug for that as follow-up.

Again, the 2px vertical bottom padding are in the spec. Since our web mockups differ mysteriously I don't think they should be used as reference. If you'd like me to deviate from the spec please take it to shorlander.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review171604

::: browser/themes/shared/browser.inc.css:46
(Diff revision 5)
> +
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
> +  overflow: -moz-hidden-unscrollable;
> +  max-height: 4em;
> +  transition: min-height 170ms ease-out, max-height 170ms ease-out;
> +  padding: 0 6px 2px;

The 2px bottom padding is still too much on Linux and Windows.

At the risk of sounding like a broken record: The increased horizontal padding should not be part of this bug. It will make bug 1352042 worse, so I'd like to land this change separately if at all.

::: browser/themes/shared/toolbarbuttons.inc.css:466
(Diff revision 5)
>  
>  /* ::::: bookmark buttons ::::: */
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton) {
> -  margin: 0;
> -  padding: 2px 3px;
> +  margin: 0 2px;
> +  padding: 0 4px;

Ditto, separate bug. We will get complaints about this, as we have before, so I don't want to sneak this in with other changes.

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8890329 [details]
Bug 1369415 - Consolidate bookmark toolbar styling across platforms to the new Photon design.

https://reviewboard.mozilla.org/r/161456/#review171604

> The 2px bottom padding is still too much on Linux and Windows.
> 
> At the risk of sounding like a broken record: The increased horizontal padding should not be part of this bug. It will make bug 1352042 worse, so I'd like to land this change separately if at all.

Again, I'm implementing the spec. Please ask shorlander. I'm happy to change it when you get sign-off from UX but since you're advocating for changing the spec, I feel like you should approach them about it (or r+ my patch).

> Ditto, separate bug. We will get complaints about this, as we have before, so I don't want to sneak this in with other changes.

I'm really confused by your hesitation to "sneak in" changes when we've been doing this all the time, making broader fixes that are out of scope of the specific bugs they were submitted in.

This bug is about bookmark toolbar spacing and the patch is titled "Consolidate bookmark toolbar styling across platforms to the new Photon design". I don't consider the changes I'm making completely out of scope, especially considering that they're conforming to the spec we were given.

Landing a 1px fix and then changing this number to match the spec right afterwards seems like a waste of time. This would only make sense if the patch had a high risk of back-out, which is unlikely for a style change.

Since you're determined to review this change outside of this bug, I'll just submit the same patch in a new bug and let it block and resolve this bug. (Or WONTFIX this bug since the title says 1px).

Comment 22

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #21)
> Again, I'm implementing the spec. Please ask shorlander. I'm happy to change
> it when you get sign-off from UX but since you're advocating for changing
> the spec, I feel like you should approach them about it (or r+ my patch).

I don't think shorlander cares how this lands and I'm not going to discuss this with him...

> > Ditto, separate bug. We will get complaints about this, as we have before, so I don't want to sneak this in with other changes.
> 
> I'm really confused by your hesitation to "sneak in" changes when we've been
> doing this all the time, making broader fixes that are out of scope of the
> specific bugs they were submitted in.

It's always a judgement call. For obviously controversial changes, I prefer to have bugs with clear summaries and justifications for the specific change.

> Landing a 1px fix and then changing this number to match the spec right
> afterwards seems like a waste of time. This would only make sense if the
> patch had a high risk of back-out, which is unlikely for a style change.

Backing out in response to user complaints is certainly an options.

Comment 23

[Dão Gottwald [:dao]]

Attachment: Bug 1369415 - Share bookmarks toolbar styling to remove cross-platform inconsistencies, adjust bottom padding on Mac to compensate border.

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

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8895305 [details]
Bug 1369415 - Share bookmarks toolbar styling to remove cross-platform inconsistencies, adjust bottom padding on Mac to compensate border.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166502/diff/1-2/

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8895305 [details]
Bug 1369415 - Share bookmarks toolbar styling to remove cross-platform inconsistencies, adjust bottom padding on Mac to compensate border.

https://reviewboard.mozilla.org/r/166502/#review171658

This patch doesn't add bottom padding on Mac, it removes 2px of bottom padding. Please adjust either the commit message or the CSS value.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #25)
> Comment on attachment 8895305 [details]
> Bug 1369415 - Share bookmarks toolbar styling to remove cross-platform
> inconsistencies, add extra bottom padding on Mac.
> 
> https://reviewboard.mozilla.org/r/166502/#review171658
> 
> This patch doesn't add bottom padding on Mac, it removes 2px of bottom
> padding. Please adjust either the commit message or the CSS value.

"Add" was meant relative to the shared CSS...

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8895305 [details]
Bug 1369415 - Share bookmarks toolbar styling to remove cross-platform inconsistencies, adjust bottom padding on Mac to compensate border.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166502/diff/2-3/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01ec784cbfd7
Share bookmarks toolbar styling to remove cross-platform inconsistencies, adjust bottom padding on Mac to compensate border. r=johannh

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01ec784cbfd7

Comment 30

[ovidiu boca[:Ovidiu]]

I verified this issue on Mac and Windows 10 and from what I see there is 3 px below bookmarks item. Please tell me if that is correct? I also tried to reproduce the bug 1289162 and I can't.

Comment 31

[Dão Gottwald [:dao]]

(In reply to ovidiu boca[:Ovidiu] from comment #30)
> I verified this issue on Mac and Windows 10 and from what I see there is 3
> px below bookmarks item. Please tell me if that is correct?

No, it's not. There should be a 1px thin line between a hovered bookmark item and the toolbar's bottom border.

Comment 32

[ovidiu boca[:Ovidiu]]

Attachment: pixels.png

Please see the attached file, it's a print screen on Mac, and I used xScope2 to measure the pixels.

Comment 33

[Dão Gottwald [:dao]]

(In reply to ovidiu boca[:Ovidiu] from comment #32)
> Created attachment 8897790 [details]
> pixels.png
> 
> Please see the attached file, it's a print screen on Mac, and I used xScope2
> to measure the pixels.

Looks like you're including the button's bottom edge and the toolbar border in your measurement? The line between the button and the toolbar border is 1px thin in that screenshot as far as I can tell.

Comment 34

[ovidiu boca[:Ovidiu]]

Thanks, I wasn't sure if that measurement is the correct one, that's why I attached the print screen.