Closed Bug 1126816 Opened 5 years ago Closed 5 years ago

a search started before the search panel opens goes to the previously selected one-off engine

Categories

(Firefox :: Search, defect)

x86
All
defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox35 --- wontfix
firefox37 --- verified
firefox38 --- verified

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
I had initially noticed this when I was at a place with a slow Internet connection and pressed Command+k,Command+v,Enter quickly to search something I had in the clipboard without letting time for the suggestions to appear.

I've now found steps to reproduce reliably even with a fast connection:
- focus the searchbox, type something in it
- press tab once to select the first one off button
- press escape to close the panel but keep the searchbox focused
- press enter

Expected result: search performed with the default engine

Actual result: search performed with the first one-off engine.

The cause of the bug is that we clear the selected one-off when the panel is redisplayed rather than when it's hidden.

I believe this bug is the cause of the Linux failure of the test I'm adding in bug 1124900. The attached patch applies after the patches from bug 1124904.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eef9a2188be
Attachment #8555884 - Flags: review?(dtownsend)
Flags: qe-verify+
Flags: firefox-backlog+
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Comment on attachment 8555884 [details] [diff] [review]
Patch

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

Test?
Attachment #8555884 - Flags: review?(dtownsend) → review+
(In reply to Dave Townsend [:mossop] from comment #1)

> Test?

I pondered the same question and more or less intentionally decided to not write a test here because:
1. This already makes an existing test fail.
2. I intend to write much more thorough tests for the one-off button behaviors in bug 1107178 soon, so I don't think testing this specific case now is worth the time.
Attached patch Patch v2Splinter Review
Turns out when pressing enter the command event fires after the popuphidden event (I expected it to fire between popuphiding and popuphidden), so the previous patch was breaking using the one-off buttons with the keyboard.

I really want bug 1107178 fixed!
Attachment #8555884 - Attachment is obsolete: true
Attachment #8558019 - Flags: review?(dtownsend)
Comment on attachment 8558019 [details] [diff] [review]
Patch v2

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

Icky but I guess if it is the only solution. I don't suppose the blur event on the search bar happens later does it?
Attachment #8558019 - Flags: review?(dtownsend) → review-
I don't think the blur event would be fired at all in some of the cases, eg. type something in the textbox, press <tab> to select the first one-off, then press <escape> to close the panel, and press <enter> to search with the default engine. Am I missing something?
Flags: needinfo?(dtownsend)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> I don't think the blur event would be fired at all in some of the cases, eg.
> type something in the textbox, press <tab> to select the first one-off, then
> press <escape> to close the panel, and press <enter> to search with the
> default engine. Am I missing something?

Probably not, let's just go with what you have.
Flags: needinfo?(dtownsend)
Attachment #8558019 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/9b09899049a6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Comment on attachment 8558019 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1088660
[User impact if declined]: the engine used when searching quickly using only the keyboard may not be what the user expects.
[Describe test coverage new/current, TreeHerder]: We somehow have some code coverage in that landing bug 1124900 without this turned bc1 orange, but we are hoping to add more tests in the future in bug 1107178. QA will verify.
[Risks and why]: the patch is quite simple.
[String/UUID change made/needed]: none.

Note: this patch applies after the patches from bug 1124904 which I don't intend to uplift to beta/36. This bug is serious enough that I think we could have attempted to fix it for 36 (with a different patch targetting 36). However, given that it seems I'm the only one who noticed this bug, and we already shipped it in 34 and 35, I would suggest that we just wontfix for 36.
Attachment #8558019 - Flags: approval-mozilla-aurora?
Attachment #8558019 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: petruta.rasa
Verified that the searches are performed with the default search engine using Nightly 38.0a1 2015-02-08 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

The issue is still reproducible with Developer Edition 37.0a2 2015-02-08 - all platforms.
Status: RESOLVED → VERIFIED
(In reply to Petruta Rasa [QA] [:petruta] from comment #11)

> The issue is still reproducible with Developer Edition 37.0a2 2015-02-08 -
> all platforms.

The patch that landed here does nothing if the patches from bug 1124904 don't land first, and these haven't landed on aurora yet.
Verified as fixed using Firefox 37 beta 1 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.
You need to log in before you can comment on or make changes to this bug.