1335992 - Search with default search engine stops working

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot

This is a regression since Firefox51.

Reproducible: always

Steps to reproduce:
1. Type something in Search Bar
2. Click [Google Search       ]

Actual results:
Nothing happens

Expected results:
Search with default search engine(Google) should be performed

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Search function is broken


Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=4e8b0579b51108c1d7c60bd1d70053136ec4521d&tochange=474f38fc48be9f97b49b084ebf0b59293b81cf16

Regressed by: 474f38fc48be	Drew Willcoxon — Bug 1180944 - Implement one-off searches from Awesomebar. r=mak,florian

Comment 2

[Florian Quèze [:florian]]

Now that we are handling click events inside the search-one-offs binding, our click handler is not receiving clicks from the header anymore. I wish the patch from bug 1110235 included a test.

Drew, do you see an easy way to fix this?

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1335992 - Search with default search engine stops working.

Review commit: https://reviewboard.mozilla.org/r/109798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109798/

Comment 4

[Drew Willcoxon :adw]

Pretty easy I think, just add a click handler that calls this.oneOffButtons.handleSearchCommand().  I'll write a test and then request review.

For easy reference, here's the click handler that was on the popup before the refactoring: https://hg.mozilla.org/integration/fx-team/file/82517993e1cc/browser/components/search/content/search.xml#l1408

Comment 5

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We should fix this for 52. Once you land a patch on m-c, can you request uplift to aurora and beta? Thanks!

Comment 7

[Julien Cristau [:jcristau]]

Drew, did you get a chance to write that test?  We're running out of time to fix this in 52.

Comment 8

[Florian Quèze [:florian]]

If we are running out of time, we could land a fix without test (but with more manual QA testing) in 52.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8833571 [details]
Bug 1335992 - Search with default search engine stops working.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109798/diff/1-2/

Comment 10

[Drew Willcoxon :adw]

Florian, the diff-1 to diff-2 interdiff isn't right because I rebased the patch, sorry about that.  But it's a small patch so comparing to the original shouldn't be a problem.

There's already a test for the header, so this updates that test.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8833571 [details]
Bug 1335992 - Search with default search engine stops working.

https://reviewboard.mozilla.org/r/109798/#review117882

::: browser/components/search/content/search.xml:1124
(Diff revision 2)
> +        if (event.button == 2) {
> +          return; // ignore right clicks.
> +        }
> +
> +        let button = event.originalTarget;
> +        let engine = button.engine || button.parentNode.engine;

the 'button.parentNode.engine' case was added in bug 1110235 to support clicking the header, so I suspect that now that this code path is (unfortunately) no longer handled in the same place as the normal one-off buttons, we can likely simplify both click handlers to have avoid this ||.

::: browser/components/search/content/search.xml:1133
(Diff revision 2)
> +        }
> +
> +        // For some reason, if the context menu had been opened prior to the
> +        // click, the suggestions popup won't be closed after loading the search
> +        // in the current tab - so we hide it manually. Some focusing magic
> +        // that happens when a search is loaded ensures that the popup is opened

We were hoping to remove that focus magic in bug 1115009. :-(

::: browser/components/search/content/search.xml:1135
(Diff revision 2)
> +        // For some reason, if the context menu had been opened prior to the
> +        // click, the suggestions popup won't be closed after loading the search
> +        // in the current tab - so we hide it manually. Some focusing magic
> +        // that happens when a search is loaded ensures that the popup is opened
> +        // again if it needs to be, so we don't need to worry about which cases
> +        // require manual hiding.

How difficult would it be to figure out the specific cases that require manual hiding?

Also, should we do the hidePopup call after the handleSearchCommand call? I'm afraid popuphiding/popuphidden handlers may (either now, or in the future) cleanup things that handleSearchCommand depends on.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8833571 [details]
Bug 1335992 - Search with default search engine stops working.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109798/diff/2-3/

Comment 13

[Drew Willcoxon :adw]

I copied and pasted the click handler right from the revision before the one-off refactoring for the urlbar landed (https://hg.mozilla.org/mozilla-central/annotate/82517993e1cc/browser/components/search/content/search.xml) -- not sure if that was clear.  So I went back and looked at the blame for that comment, and it's bug 1110771.  Bug 1110771 comment 3:

(In reply to Nihanth Subramanya [:nhnt11] from bug 1110771 comment #3)
> - There's a weird issue where if you open a context menu, and then click a
> one off button to search (even if you didn't click any of the menu items),
> the suggestions popup doesn't get closed. I worked around this by adding a
> manual hidePopup() which doesn't seem to change any existing behavior - but
> if there's a better fix that'd be great. Please see the comment in the code.

I removed the hidePopup() and I couldn't reproduce this problem on Windows (7) or Mac.  So... whatever was the problem there, it's not happening anymore...?

Bug 1110771 comment 21 describes similar problems with the context menu by the way, but they aren't this problem.

Comment 14

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #13)

> I removed the hidePopup() and I couldn't reproduce this problem on Windows
> (7) or Mac.  So... whatever was the problem there, it's not happening
> anymore...?

We removed that hidePopup() and that comment in bug 1316863.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8833571 [details]
Bug 1335992 - Search with default search engine stops working.

https://reviewboard.mozilla.org/r/109798/#review118308

Looks good, thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11eb095a3715
Search with default search engine stops working. r=florian

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/11eb095a3715

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Welp, this landed too late for Fx52. But we should probably get this uplifted to Aurora for 53 at least. And maybe we can consider it for ESR52 approval during the upcoming cycle too.

Comment 19

[Drew Willcoxon :adw]

Attachment: Aurora and ESR 52 patch

Approval Request Comment
[Feature/Bug causing the regression]: One-off searches in the urlbar, bug 1180944
[User impact if declined]: This bug
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Probably not
[Why is the change risky/not risky?]: Small patch, small change, comes with test
[String changes made/needed]: None

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's a regression in a long-time interaction in the search bar popup
User impact if declined: This bug
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

Comment 20

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8843149 [details] [diff] [review]
Aurora and ESR 52 patch

Fix for search bar, let's uplift to 53 aurora.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06f51944867c

Comment 22

[Simona Badau, Desktop QA]

Reproduced using the Firefox 51.0.1 (Build ID: 20170125094131) release on Windows 10 x64.

Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170306080637) and latest Firefox Developer Edition 53.0a2 (20170306004003) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8843149 [details] [diff] [review]
Aurora and ESR 52 patch

fix for search popup clicks, verified in 53 and 54, let's get this in esr52

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/fe579c64db03