1400165 - Margin around the search field in the bookmark sidebar should be bigger

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Valentina Claudia Ona]

Attachment: bookmark.bmp

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID 20170914100122

[Affected version]: Nightly 57.0a1

[Affected platforms]: Windows 10 x64, Windows 10 x64 Surface Pro4, Ubuntu 16.04 x64, Mac OS x 10.12

[Steps to reproduce]:

1. Launch Nightly 57.0a1 
2. Enable the Bookmark sidebar from the menu bar.

[Expected result]: The distance between search box and bookmark box should be match the specifications.

[Actual result]: The distance next to search box from bookmark side is smaller than spec. For more information please check the attachment.

Comment 1

[Dão Gottwald [:dao]]

Nihanth, could you please evaluate this bug?

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

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

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

To be honest, I don't remember why I didn't match the spec on this, oh well.

Anyway, for the default width of the sidebar, increasing the padding results in the history search bar being too small for the placeholder text on macOS, and overall feels very cramped, so I want to increase the default sidebar width if we're going to add this padding.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

https://reviewboard.mozilla.org/r/181992/#review187488

::: browser/base/content/browser.xul:1189
(Diff revision 1)
>  # spacer should significantly out-flex the button.
>            <spacer flex="1000"/>
>            <toolbarbutton id="sidebar-close" class="close-icon tabbable" tooltiptext="&sidebarCloseButton.tooltip;" oncommand="SidebarUI.hide();"/>
>          </sidebarheader>
>          <browser id="sidebar" flex="1" autoscroll="false" disablehistory="true" disablefullscreen="true"
> -                  style="min-width: 14em; width: 18em; max-width: 36em;" tooltip="aHTMLTooltip"/>
> +                  style="min-width: 14em; width: 22em; max-width: 36em;" tooltip="aHTMLTooltip"/>

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)
> Anyway, for the default width of the sidebar, increasing the padding results
> in the history search bar being too small for the placeholder text on macOS,
> and overall feels very cramped, so I want to increase the default sidebar
> width if we're going to add this padding.

Ugh. I think this is because we increase the font size in the side bar on Mac, but "18em" on #sidebar still refers to the default font size. Making the side bar wider across platforms doesn't seem like the right solution to that. Can we instead consistently increase the font size for #sidebar on Mac so that 18em refers to the right thing?

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

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

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

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

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

https://reviewboard.mozilla.org/r/181992/#review189706

::: browser/themes/osx/places/places.css:60
(Diff revision 2)
>  #sidebar-search-label {
>    display: none;
>  }
>  
> +#sidebar-search-container {
> +  padding: 4px;

Why is this different on Mac vs. Windows/Linux?

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Dão Gottwald [::dao] from comment #7)
> Comment on attachment 8910546 [details]
> Bug 1400165 - Increase padding of sidebar search container, adjust sidebar
> font-size on MacOS.
> 
> https://reviewboard.mozilla.org/r/181992/#review189706
> 
> ::: browser/themes/osx/places/places.css:60
> (Diff revision 2)
> >  #sidebar-search-label {
> >    display: none;
> >  }
> >  
> > +#sidebar-search-container {
> > +  padding: 4px;
> 
> Why is this different on Mac vs. Windows/Linux?

MacOS native searchbar styling already adds a 4px margin, also see bug 1382033 comment 12.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

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

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

https://reviewboard.mozilla.org/r/181992/#review189734

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f63df3b6606a
Increase padding of sidebar search container, adjust sidebar font-size on MacOS. r=dao

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

Approval Request Comment
[Feature/Bug causing the regression]: photon-visual polish
[User impact if declined]: Search bar is a little cramped in the sidebar
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: minor CSS changes, avoiding saying "no risk" because there is a font-size adjustment, but any regression can easily be fixed and uplifted quickly
[String changes made/needed]: none

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/f63df3b6606a

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910546 [details]
Bug 1400165 - Increase padding of sidebar search container, adjust sidebar font-size on MacOS.

Polish photon, should be in 57b5

Comment 15

[Valentina Claudia Ona]

Attachment: aa.bmp

I tested this problem using Nightly 58.0a1 with the build ID 20171002100134 on Windows 10 x64 and Windows 7 x64 and I see no difference, but on Mac OS 10.12 and Ubuntu 16.04, the problem is resolved.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c7b47e1282c2

Comment 17

[Valentina Claudia Ona]

Can you look on this issue?

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Valentina Claudia Ona from comment #17)
> Can you look on this issue?

Filed bug 1411236. Thanks for spotting this.

Comment 19

[Valentina Claudia Ona]

Based on the fact that has been filled a new bug 1411236, I will mark this as verified fixed.