Closed
Bug 1179234
Opened 9 years ago
Closed 9 years ago
[OSX retina] Horizontal grey bars in the new search panel should have the same height
Categories
(Firefox :: Search, defect, P5)
Tracking
()
VERIFIED
FIXED
Firefox 43
People
(Reporter: phorea, Assigned: florian)
References
Details
(Keywords: polish, Whiteboard: [ui][fxsearch])
Attachments
(2 files)
3.02 MB,
image/png
|
Details | |
959 bytes,
patch
|
Gijs
:
review+
nhnt11
:
feedback+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1108648 +++ This is a minor detail, but I suspect the panel would look better if the default search engine grey bar did not have a different height to the "Search for x" bar. The current difference is probably due to the icon of the default search engine enforcing a minimum height. AR: On a Macbook pro retina 10.9.5 the height on "<search engine> search" field is 44px while there are 43px on "Search with" field. ER: The height should be the same, as for the other platforms.
Updated•9 years ago
|
Rank: 55
Flags: firefox-backlog+
Updated•9 years ago
|
Rank: 55 → 58
Assignee | ||
Comment 1•9 years ago
|
||
Setting margin-bottom to a value >= 1.75px (I assume it's rounded to 2px) on ".search-panel-header > label" fixes bug 1181868 for me, and the second 1px -> 2px change is so that both bars have the same height again; which fixes this bug. nhnt11, requesting feedback from you so that you can try the patch and check that I haven't forgotten something that we had discussed before and caused us to pick 1px rather than 2px for that value. I wonder if that change should be made for all 3 platforms for consistency. Do you have any thoughts on this? Anything I should be careful about before making this change on Linux/Windows?
Attachment #8645005 -
Flags: feedback?(nhnt11)
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #1) > I wonder if that change should be made for all 3 platforms for consistency. > Do you have any thoughts on this? Anything I should be careful about before > making this change on Linux/Windows? With that change ported to the linux/ folder, I get http://i.imgur.com/SHtNk2N.png That seems a regression compared to http://i.imgur.com/WnBn9MW.png (same screenshot without the patch).
Comment 3•9 years ago
|
||
Comment on attachment 8645005 [details] [diff] [review] Patch Review of attachment 8645005 [details] [diff] [review]: ----------------------------------------------------------------- Finally got to try the patch, it works fine. I don't remember any particular reason for choosing 1px over 2px. > I wonder if that change should be made for all 3 platforms for consistency. I'd imagine we want the margins to be the same on all platforms. Other than that, I don't really have any comments. > With that change ported to the linux/ folder, [...] Interesting, I'd have thought that consistent CSS means consistent UI. Since this doesn't appear to be the case, will the incosistent CSS between platforms be painful for bug 1106569?
Attachment #8645005 -
Flags: feedback?(nhnt11) → feedback+
Comment 4•9 years ago
|
||
Hmm, after using the search box a bit with the patch applied, I'm wondering if we should increase the font size of the header text to fill up some of the whitespace. The text seems a bit small. I wonder if font size is related to the inconsistency on linux. Here's a screenshot with the font size set to 11px: http://puu.sh/jyacD/54be591b34.png versus the current 10px: http://puu.sh/jyaed/bf5323e578.png
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Nihanth Subramanya [:nhnt11] from comment #3) > will the incosistent CSS between > platforms be painful for bug 1106569? Somehow yes, but not more painful than the other existing differences between platforms.
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8645005 [details] [diff] [review] Patch Thanks for the feedback! Given that the problem isn't visible on Windows or Linux, and that the patch here applied to linux causes a regression, I'm going to keep it Mac-only and request review. Increasing the font size on Mac/Windows may make sense, but I would rather not do that in a regression fix that we have to uplift to 41.
Attachment #8645005 -
Flags: review?(gijskruitbosch+bugs)
Comment 7•9 years ago
|
||
Comment on attachment 8645005 [details] [diff] [review] Patch Review of attachment 8645005 [details] [diff] [review]: ----------------------------------------------------------------- I trust your testing here, though I have no idea why this works the way it does... ::: browser/themes/osx/searchbar.css @@ +166,4 @@ > } > > .search-panel-current-input > label { > + margin: 2px 0 2px !important; Nit: margin: 2px 0 !important;
Attachment #8645005 -
Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/d8462f36c341
Assignee: nobody → florian
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8645005 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: regression from bug 1108648. [User impact if declined]: broken suggestions on retina macs. See bug 1181868. [Describe test coverage new/current, TreeHerder]: tested locally. QA will verify. [Risks and why]: low risk, only adding 1px of margin; only on Mac. [String/UUID change made/needed]: none.
Attachment #8645005 -
Flags: approval-mozilla-beta?
Attachment #8645005 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Flags: qe-verify+
status-firefox41:
--- → affected
Comment on attachment 8645005 [details] [diff] [review] Patch Let's uplift to Beta and Aurora. The patch is simple and straightforward.
Attachment #8645005 -
Flags: approval-mozilla-beta?
Attachment #8645005 -
Flags: approval-mozilla-beta+
Attachment #8645005 -
Flags: approval-mozilla-aurora?
Attachment #8645005 -
Flags: approval-mozilla-aurora+
status-firefox42:
--- → affected
Reporter | ||
Comment 14•9 years ago
|
||
Verified that the height is now the same using Firefox 41 beta 2, latest Dev Edition 42.0a2 and latest Nightly 43.0a1 2015-08-17 under MacBook Pro retina 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•