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

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Sam Hall]

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

Comment 1

[Sam Hall]

I previously put the wrong bug number in the Blocks field

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Did this get dropped?

Comment 4

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #3)
> Did this get dropped?

Just delayed, I was sick part of last week...

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

Comment 6

[Florian Quèze [: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.

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

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).

Comment 9

[Florian Quèze [: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 10

[Florian Quèze [:florian]]

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.

Comment 11

[Dão Gottwald [:dao]]

landed as is:

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36d60c36b236

Comment 13

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed on Nightly 38.0a1 2015-02-01, Ubuntu 14.04 LTS 32-bit.

Comment 14

[Florian Quèze [:florian]]

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.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/019f2fb5db29

Comment 17

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Firefox 37 beta 1 under Ubuntu 12.04 LTS 32-bit.