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

Status: VERIFIED FIXED

(Firefox :: Search, defect, P5)

Description

[Petruta Horea [:phorea]]

Attachment: 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.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

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?

Comment 2

[Florian Quèze [:florian]]

(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

[(inactive) Nihanth Subramanya [:nhnt11]]

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?

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 5

[Florian Quèze [:florian]]

(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 6

[Florian Quèze [:florian]]

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.

Comment 7

[:Gijs (he/him)]

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;

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d8462f36c341

Comment 9

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

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

Comment 10

[Florian Quèze [:florian]]

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.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8645005 [details] [diff] [review]
Patch

Let's uplift to Beta and Aurora. The patch is simple and straightforward.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c691d5e66a3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84939af40230

Comment 14

[Petruta Horea [:phorea]]

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.