Closed Bug 1400165 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: valentina.ona, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual] )

Attachments

(3 files)

Attached image 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.
Whiteboard: [photon-visual][triage]
Nihanth, could you please evaluate this bug?
Flags: needinfo?(nhnt11)
Summary: the distance next to search box from bookmark sidebar is smaller than spec. → Margin around the search field in the bookmark sidebar should be bigger
Priority: -- → P3
Priority: P3 → P4
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.
Flags: needinfo?(nhnt11)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
Priority: P4 → P1
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
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?
Attachment #8910546 - Flags: review?(dao+bmo) → review-
Flags: qe-verify? → qe-verify+
QA Contact: valentina.ona
Iteration: 57.3 - Sep 19 → ---
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?
(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 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
Attachment #8910546 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f63df3b6606a
Increase padding of sidebar search container, adjust sidebar font-size on MacOS. r=dao
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
Attachment #8910546 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/f63df3b6606a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
Attachment #8910546 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image 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.
Flags: needinfo?(wkocher)
Can you look on this issue?
Flags: needinfo?(nhnt11)
Depends on: 1411236
(In reply to Valentina Claudia Ona from comment #17)
> Can you look on this issue?

Filed bug 1411236. Thanks for spotting this.
Flags: needinfo?(nhnt11)
Based on the fact that has been filled a new bug 1411236, I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.