Closed Bug 1179234 Opened 4 years ago Closed 4 years ago

[OSX retina] Horizontal grey bars in the new search panel should have the same height

Categories

(Firefox :: Search, defect, P5)

41 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox41 --- verified
firefox42 --- verified
firefox43 --- verified

People

(Reporter: petruta.rasa, Assigned: florian)

References

Details

(Keywords: polish, Whiteboard: [ui][fxsearch])

Attachments

(2 files)

Attached image retina screenshot
+++ 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.
Rank: 55
Flags: firefox-backlog+
Rank: 55 → 58
Attached patch PatchSplinter Review
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)
(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 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+
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
(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.
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 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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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?
Flags: qe-verify+
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+
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.
You need to log in before you can comment on or make changes to this bug.