Closed Bug 1111868 Opened 5 years ago Closed 5 years ago

open search items and settings button missing borders at the bottom of the search suggestions panel on Linux

Categories

(Firefox :: Theme, defect)

34 Branch
x86_64
Linux
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- verified
firefox38 --- verified

People

(Reporter: sam.hall, Assigned: dao)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached image search.png
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141127111021

Steps to reproduce:

I've only just realised that the Quick Search isn't intended to look this way after looking at the screenshots affixed to bug 1088660 (particularly attachment 8512288 [details]).

Install Xubuntu 14.04, Firefox 34.0 and use the Quick Search to see what I mean.


Actual results:

There are white background that look out of place and missing border lines. It looks disturbing and out of place.


Expected results:

Something that blends in more with the rest of the UI. Grey backgrounds and borders so it just looks like a simple drop down menu.
Blocks: 1107292
Blocks: fx34-searchui
No longer blocks: 1107292
I previously put the wrong bug number in the Blocks field
Component: Untriaged → Theme
Summary: Mismatching look and feel of the Quick Search dropdown in Xubuntu 14.04 → open search items and settings button missing borders at the bottom of the search suggestions panel on Linux
Duplicate of this bug: 1116021
Flags: firefox-backlog+
Status: UNCONFIRMED → NEW
Points: --- → 3
Ever confirmed: true
Flags: qe-verify+
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Did this get dropped?
Flags: needinfo?(dao)
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #3)
> Did this get dropped?

Just delayed, I was sick part of last week...
Flags: needinfo?(dao)
Attached patch patch (obsolete) — Splinter Review
Attachment #8555854 - Flags: review?(florian)
Comment on attachment 8555854 [details] [diff] [review]
patch

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

::: browser/themes/linux/searchbar.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #PopupSearchAutoComplete {
>    -moz-margin-start: -24px;
> +  padding: 1px;

This change is causing the layout of the one-off buttons to be messed up for some widths of the searchbox. See http://i.imgur.com/gJeD37a.png

@@ -121,5 @@
>  
>  .search-panel-header {
>    font-weight: normal;
>    background-color: rgb(245, 245, 245);
> -  border-top: 1px solid #ccc;

Removing this removes the border at the top of the "Search for <search terms> with:" line.
Attachment #8555854 - Flags: review?(florian) → review-
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> Comment on attachment 8555854 [details] [diff] [review]
> patch
> 
> Review of attachment 8555854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/searchbar.css
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #PopupSearchAutoComplete {
> >    -moz-margin-start: -24px;
> > +  padding: 1px;
> 
> This change is causing the layout of the one-off buttons to be messed up for
> some widths of the searchbox. See http://i.imgur.com/gJeD37a.png

This padding is supposed to make room for the native border such that not every individual popup child needs to do that (which is basically an invitation for this very bug to happen). Can you suggest a better way to achieve that? Or would you prefer that we keep hacking around this?
Flags: needinfo?(florian)
Attached patch patch v2Splinter Review
Now using a negative margin on .search-panel-one-offs to make up for the popup padding. This is pretty nasty and means that hovering on a .searchbar-engine-one-off-item keeps overlapping the popup border (like it does without this patch).
Attachment #8555854 - Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8556381 - Flags: review?(florian)
(In reply to Dão Gottwald [:dao] from comment #7)

> Can you suggest a better way to
> achieve that? Or would you prefer that we keep hacking around this?

The 2 ideas I had of possible solutions to explore were:
- negative margin/padding on the one-off button area (what you implemented)
- see if we can tweak the code near https://hg.mozilla.org/releases/mozilla-beta/annotate/fac8edb83df7/browser/base/content/urlbarBindings.xml#l1157 to remove the padding from the width of the panel before computing the sizes of the one-off buttons. This may help with bug 1104325.
Comment on attachment 8556381 [details] [diff] [review]
patch v2

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

> This is pretty nasty and means that hovering on a
> .searchbar-engine-one-off-item keeps overlapping the popup border (like it
> does without this patch).

I probably wouldn't have noticed if you hadn't mentioned it, but it may be much more visible with other GTK themes. Up to you whether it's acceptable or not, but I'm OK with r+'ing as-is, as the patch is already a very significant improvement over the current state.
Attachment #8556381 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/36d60c36b236
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Verified as fixed on Nightly 38.0a1 2015-02-01, Ubuntu 14.04 LTS 32-bit.
Status: RESOLVED → VERIFIED
Comment on attachment 8556381 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1088660
[User impact if declined]: some borders missing on the search panel on linux. Hardly visible with some window managers, very visible with others.
[Describe test coverage new/current, TreeHerder]: verified by QA on central.
[Risks and why]: moderate, an error in this CSS could cause layout issues like http://i.imgur.com/gJeD37a.png (which we have fixed). This is the reason why I'm not requesting beta approval.
[String/UUID change made/needed]: none.
Attachment #8556381 - Flags: approval-mozilla-aurora?
Comment on attachment 8556381 [details] [diff] [review]
patch v2

I agree with not wanting to take the risk late in Beta. The style is not right at all bug the UI does look to still be usable with this bug. Let's get this fix into 37 Aurora.
Attachment #8556381 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using Firefox 37 beta 1 under Ubuntu 12.04 LTS 32-bit.
You need to log in before you can comment on or make changes to this bug.