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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: valentina.ona, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual] )

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8908509 [details]
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.
(Reporter)

Updated

a year ago
Blocks: 1325171
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
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
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)

Updated

a year ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Whiteboard: [photon-visual][triage] → [reserve-photon-visual]
(Assignee)

Updated

a year ago
Priority: P4 → P1
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?

Comment 4

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
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?
(Assignee)

Comment 8

a year ago
(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 hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
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+

Comment 11

a year ago
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
(Assignee)

Comment 12

a year ago
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?

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f63df3b6606a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
status-firefox56: --- → unaffected
status-firefox57: --- → affected
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+
(Reporter)

Comment 15

a year ago
Created attachment 8914349 [details]
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)

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c7b47e1282c2
status-firefox57: affected → fixed
Flags: needinfo?(kwierso)
(Reporter)

Comment 17

a year ago
Can you look on this issue?
Flags: needinfo?(nhnt11)
(Assignee)

Updated

a year ago
Depends on: 1411236
(Assignee)

Comment 18

a year ago
(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)
(Reporter)

Comment 19

a year ago
Based on the fact that has been filled a new bug 1411236, I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.