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

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?

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

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

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.

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

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!

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/11eb095a3715 Search with default search engine stops working. r=florian

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.

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 on attachment 8843149 [details] [diff] [review] Aurora and ESR 52 patch Fix for search bar, let's uplift to 53 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

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