1173715 - double borders when clicking on the magnifying glass of an empty searchbar, with all one-off engines hidden

Status: VERIFIED FIXED

(Firefox :: Search, defect, P5)

Description

[Florian Quèze [:florian]]

Attachment: Screenshot

Edge case where we show 2 borders and should be showing only one. See attached screenshot.

Steps to reproduce:
- in the search settings, uncheck all the one-click search engines
- click on the glass icon (will only show the bug when the textfield is empty)

Comment 1

[Atique Ahmed Ziad [:atiqueahmedziad]]

can you please a more info. of fixing it ?

Comment 2

[Atique Ahmed Ziad [:atiqueahmedziad]]

**can you please add a more info. of fixing it ?

Comment 3

[Nadif Ali Fahim]

Well I'm new. And I wanna try to fix this bug.

Comment 4

[Atique Ahmed Ziad [:atiqueahmedziad]]

Great :) Good luck. Florian will help you with fixing the bug.

Comment 5

[Nadif Ali Fahim]

Florian can you please help me out. I don't know where to get started.

Comment 6

[Florian Quèze [:florian]]

(In reply to Nadif Ali Fahim from comment #5)
> Florian can you please help me out. I don't know where to get started.

Hi Nadif :-).

Do you already have a local build of Firefox that you can modify, or do you need help for this?

The CSS files for this part of the UI are browser/themes/linux/searchbar.css browser/themes/osx/searchbar.css and browser/themes/windows/searchbar.css

https://hg.mozilla.org/mozilla-central/rev/e73e95394f91#l4.25 is an example of what was done in bug 1136139 to prevent showing a double border when there's no search suggestion.

When there's no one-off engine button to show, we hide the header and the list of one-off engines at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml?rev=17f159b29970#1276

Feel free to ask questions if you need more help to get started.

Comment 7

[Nadif Ali Fahim]

Thanks alot Florian. But I need a little more help in getting the local build of Firefox. How am I supposed to get it? Or where can I find it?

Comment 8

[Florian Quèze [:florian]]

(In reply to Nadif Ali Fahim from comment #7)
> Thanks alot Florian. But I need a little more help in getting the local
> build of Firefox. How am I supposed to get it? Or where can I find it?

There are explanations on https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Comment 9

[Nadif Ali Fahim]

Thanks for the help Florian. I'll try to fix the bug as fast as I can.

Comment 10

[Jeremy Francispillai]

Hi, could I please try to fix this bug? I already have Firefox built and would like to try to fix this as my first bug.

Comment 11

[Atique Ahmed Ziad [:atiqueahmedziad]]

Great Jeremy :) 
Please work with it. I am assigning you.  

Thanks for contributing!

Comment 12

[Jeremy Francispillai]

Hi Atique, I believe that I have a fix for the bug on my working copy. What are the next steps to get my changes reviewed?
Thanks!

Comment 13

[Florian Quèze [:florian]]

(In reply to Jeremy from comment #12)
> Hi Atique, I believe that I have a fix for the bug on my working copy. What
> are the next steps to get my changes reviewed?

Hi Jeremy, thanks for working on this bug! :)

If you have a fix locally, the next step is to attach a patch and request review. You can generate a patch using 'hg diff', and then attach it to the bug using the "Add an attachment" link.
On the attachment form, check the 'patch' checkbox; set the 'review' flag to '?' and put my email address in the requestee field that will appear.

Comment 14

[Jeremy Francispillai]

Attachment: bug1173715.txt

I made the change on the three different css files for osx, windows, and linux. To fix the problem, I removed the bottom border on the search bar at the top and added a top border to the list of suggested searches. This way, you would no longer need to check which element is below the search bar if there are no suggestions and constantly remove the top borders off of the elements that are directly below. Since all of the below elements have top borders, this solves the bug and also let me remove the check on the search-panel-one-offs-header element to see if the suggestions element is collapsed or not. This seemed like the best solution because checking if all of the above elements are hidden/collapsed would be more complex. This should also ensure that you will not run into this bug again if any additional elements are added in the future.

Comment 15

[Jeremy Francispillai]

Regarding my above comment, I actually didn't remove the bottom border off of the search bar, but rather the search-panel-current-engine element which contains the text "Google Search" under which the bug was occuring

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8676034 [details] [diff] [review]
bug1173715.txt

Looks good to me, thanks!

The next step is to get the patch checked in. Would it be possible for you to attach the patch with a commit message and author, as described at https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ?

Comment 17

[Jeremy Francispillai]

Hi Florian, I don't know if this is a dumb question, but I created the patch and it is located in the hidden .hg folder, so I cannot attach it here because I can't access the folder from the webbrowser

Comment 18

[Jeremy Francispillai]

Attachment: bug1173715.patch

Here is the patch. I just made a copy of the patch onto my desktop to solve my above problem.

Comment 19

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/ef2b5e2a78672c228e42529ca181fdfae714fb1b
Bug 1173715 - Removed the bottom border off of the search-panel-current-engine element and removed the check to see if the search suggestions is collapsed, r=florian.

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8678541 [details] [diff] [review]
bug1173715.patch

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

(In reply to Jeremy from comment #17)
> Hi Florian, I don't know if this is a dumb question, but I created the patch
> and it is located in the hidden .hg folder, so I cannot attach it here
> because I can't access the folder from the webbrowser

You can use "hg export" and redirect the output to a file to generate a patch file where you want.

If you are going to export patches to bugzilla often, you may want to install the bzexport mercurial extension (easily done by running mach mercurial-setup), see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch for details.

::: browser/themes/linux/searchbar.css
@@ +88,5 @@
> +  border-bottom: none;
> +}
> +
> +.search-panel-tree {
> +    border-top: 1px solid #ccc !important;

I missed this during the previous review: the indent here should be only 2 spaces, not 4.

I fixed this before pushing the patch.

Comment 21

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

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

Comment 22

[Ashickur Rahman]

Successfully reproduce the bug on Firefox 42.0;Build ID:20151029151421;User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0

The fix works for me on Firefox 44.0a2;Build ID:20151103004217;User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:44.0) Gecko/20100101 Firefox/44.0

Comment 23

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced this bug on Nightly 41.0a1 (2015-06-11); (Build ID: 20150611030208) with the instruction from comment 0 on Linux, 64 bit

This Bug is now verified as fixed on Latest Firefox Developer Edition 44.0a2 (2015-11-03)

Build ID: 20151103004217
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0

As it is also verified on Mac OS X (Comment 22), Marking it as verified!

Comment 24

[Florian Quèze [:florian]]

Jeremy, congratulations for your first patch that has landed and been verified :-). Would you like help to find another bug to work on next? (Bug 1119872 may be a good one)

Comment 25

[Jeremy Francispillai]

Hi, sorry for the late reply. Thanks a lot for all of the help with this bug. I would love the take a look at Bug 111982. I will look over it in the next few days.

Comment 26

[Jeremy Francispillai]

I meant Bug 1119872.