Search with default search engine stops working

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: alice0775, Assigned: adw)

Tracking

({regression, ux-consistency})

51 Branch
Firefox 54
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 wontfix, firefox52+ wontfix, firefox-esr52 fixed, firefox53+ verified, firefox54+ verified)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments)

Posted image 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
[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
Blocks: 1180944
Flags: needinfo?(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?
Flags: needinfo?(florian) → needinfo?(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
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
We should fix this for 52. Once you land a patch on m-c, can you request uplift to aurora and beta? Thanks!
Flags: needinfo?(adw)
Duplicate of this bug: 1339457
Blocks: 1337003
Drew, did you get a chance to write that test?  We're running out of time to fix this in 52.
If we are running out of time, we could land a fix without test (but with more manual QA testing) in 52.
Priority: -- → P1
Whiteboard: [fxsearch]
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.
Flags: needinfo?(adw)
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.
Attachment #8833571 - Flags: review?(florian) → review-
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.
(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 on attachment 8833571 [details]
Bug 1335992 - Search with default search engine stops working.

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

Looks good, thanks!
Attachment #8833571 - Flags: review?(florian) → review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11eb095a3715
Search with default search engine stops working. r=florian
https://hg.mozilla.org/mozilla-central/rev/11eb095a3715
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Flags: needinfo?(adw)
Flags: in-testsuite+
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
Flags: needinfo?(adw)
Attachment #8843149 - Flags: approval-mozilla-esr52?
Attachment #8843149 - Flags: approval-mozilla-aurora?
Comment on attachment 8843149 [details] [diff] [review]
Aurora and ESR 52 patch

Fix for search bar, let's uplift to 53 aurora.
Attachment #8843149 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
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
Attachment #8843149 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.