Closed
Bug 1335992
Opened 8 years ago
Closed 8 years ago
Search with default search engine stops working
Categories
(Firefox :: Search, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 54
People
(Reporter: alice0775, Assigned: adw)
References
Details
(Keywords: regression, ux-consistency, Whiteboard: [fxsearch])
Attachments
(3 files)
34.15 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
3.62 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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
![]() |
Reporter | |
Comment 1•8 years ago
|
||
[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•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
We should fix this for 52. Once you land a patch on m-c, can you request uplift to aurora and beta? Thanks!
tracking-firefox54:
--- → +
Flags: needinfo?(adw)
Comment 7•8 years ago
|
||
Drew, did you get a chance to write that test? We're running out of time to fix this in 52.
Comment 8•8 years ago
|
||
If we are running out of time, we could land a fix without test (but with more manual QA testing) in 52.
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
mozreview-review |
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-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
mozreview-review |
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+
Comment 16•8 years ago
|
||
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11eb095a3715
Search with default search engine stops working. r=florian
Comment 17•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
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?
Updated•8 years ago
|
Comment 20•8 years ago
|
||
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+
Comment 21•8 years ago
|
||
bugherder uplift |
Comment 22•8 years ago
|
||
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•8 years ago
|
||
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+
Comment 24•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•