1108648 - Horizontal grey bars in the new search panel should have the same height

Status: VERIFIED FIXED

(Firefox :: Search, defect, P5)

Description

[aleth [:aleth]]

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.

Comment 1

[Florian Quèze [:florian]]

Stephen, do you have an opinion about this?

Comment 2

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> Stephen, do you have an opinion about this?

Yes, they should be the same height.

Comment 3

[Nihanth Subramanya [:nhnt11]]

Attachment: Patch

I'm not sure whether the margin on the label was forced to 0 on all sides for convenience or because the top and bottom margins actually needed to be 0 in some case(s). I couldn't find such a case though, so I changed it to only forcing margin-right and -left to 0.

Comment 4

[Florian Quèze [:florian]]

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

Review of attachment 8609073 [details] [diff] [review]:
-----------------------------------------------------------------

This patch only touches the osx file, I assume Windows and Linux have the same issue too.

::: browser/themes/osx/searchbar.css
@@ +153,5 @@
>    border-top: 1px solid #ccc;
>    margin: 0;
>    padding: 3px 6px;
>    color: #666;
> +  min-height: 24px;

This line doesn't seem to have an effect for me, did it have one for you?

@@ +162,5 @@
>  }
>  
>  .search-panel-current-input > label {
> +  margin-right: 0 !important;
> +  margin-left: 0 !important;

While reviewing this, I've noticed that in the header at the top, the engine icon is stretched to a 17px height instead of 16. This seems to be caused by the 13px line height + 2px of margin top + 2px of margin bottom. If we remove a px of margin at the bottom, the icon is no longer stretched, but then we need to also remove a pixel on the other gray bar, so I ended up with something like this:

.search-panel-header > label {
  margin-top: 2px !important;
  margin-bottom: 1px !important;
}

.search-panel-current-input > label {
  margin: 2px 0 1px !important;
}

Comment 5

[Nihanth Subramanya [:nhnt11]]

OK, your CSS works well on OS X (retina and non-retina) and Linux. I'll test on Windows once I get a VM.

Comment 6

[Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

I tested the CSS you ended up with on Windows and Ubuntu VMs, works fine on both. Note that while the height of the header ends up at 22px + border on Windows and OS X, it ends up at 26px + border on Ubuntu. The main header was already 26px + border though, so it's fine I think.
Thanks!

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8610940 [details] [diff] [review]
Patch v2

Review of attachment 8610940 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

(In reply to Nihanth Subramanya [:nhnt11] from comment #6)
> Note that while the height of the header ends up at 22px + border on
> Windows and OS X, it ends up at 26px + border on Ubuntu. The main header was
> already 26px + border though, so it's fine I think.

This is OK, the default font size is larger on Linux.

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bb094a070d4

Comment 10

[Petruta Horea [:phorea]]

Verified as fixed on Nightly 41.0a1 2015-06-03 under Win 7 64-bit and Ubuntu 14.04 32-bit. I couldn't verify on Mac OS X due to bug 1171406. Will mark accordingly after it will be fixed.

Comment 11

[Petruta Horea [:phorea]]

With 41.0a1 2015-06-29 Nightly, I have 22px (without border) on Mac OS X 10.9.5 without retina.
Using a retina Macbook Pro 10.9.5 I can see 44px on "<search engine> search" field and 43px on "Search with" field. I will file a follow up bug for this and mark this one as verified.