Closed Bug 1108648 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Search, defect, P5)

36 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- verified

People

(Reporter: aleth, Assigned: nhnt11)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Stephen, do you have an opinion about this?
Flags: needinfo?(shorlander)
(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.
Flags: needinfo?(shorlander)
Flags: qe-verify+
Flags: firefox-backlog+
OS: Mac OS X → All
Hardware: x86 → All
Priority: -- → P5
Whiteboard: [fxsearch][searchui]
Rank: 55
Whiteboard: [fxsearch][searchui] → [ui][fxsearch]
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8609073 - Flags: review?(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;
}
Attachment #8609073 - Flags: review?(florian) → feedback+
OK, your CSS works well on OS X (retina and non-retina) and Linux. I'll test on Windows once I get a VM.
Attached patch Patch v2Splinter Review
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!
Attachment #8609073 - Attachment is obsolete: true
Attachment #8610940 - Flags: review?(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.
Attachment #8610940 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/0bb094a070d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
QA Contact: petruta.rasa
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.
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.
Status: RESOLVED → VERIFIED
Depends on: 1181868
Depends on: 1212337
You need to log in before you can comment on or make changes to this bug.