First AutoCompleteSearch that returns RESULT_NOMATCH cancels all other searches when popup is open

RESOLVED FIXED in mozilla11

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Mardak, Assigned: felix)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
If you specify multiple searches for an autocomplete, if there are any that return RESULT_NOMATCH before the other searches finish, all the other ones will get canceled if the popup is already open.

This is because the first one that gives RESULT_NOMATCH makes the autocomplete controller close the popup, which causes the other searches to stop for some reason.

For a workaround, return RESULT_NOMATCH_ONGOING but you'll need to make sure to eventually give a RESULT_NOMATCH.. otherwise it'll think it's still searching.
(Reporter)

Comment 1

8 years ago
(In reply to comment #0)
> controller close the popup, which causes the other searches to stop for some
> reason.
This is probably...

nsAutoCompleteController::ProcessResult
  if mRowCount == 0 and result != ONGOING -> call ClosePopup()

eventually this triggers...

autocomplete.xml#autocomplete-base-popup popuphiding handler
  unconditionally calls controller.stopSearch()
(Assignee)

Updated

7 years ago
Assignee: nobody → ffung
(Assignee)

Comment 2

7 years ago
Created attachment 572614 [details] [diff] [review]
NO_MATCH in Multiple AutoComplete Searches Cancels Others

I don't think individual searches should return NO_MATCH_ONGOING because that would require an individual search to know of the status of other searches in some way or another.
Attachment #572614 - Flags: review?(edilee)
(Reporter)

Comment 3

7 years ago
Comment on attachment 572614 [details] [diff] [review]
NO_MATCH in Multiple AutoComplete Searches Cancels Others

>+++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp
>@@ -1301,20 +1301,22 @@ nsAutoCompleteController::ProcessResult(
>-  else if (result != nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING)
>+  } else if (mSearchesOngoing == 0 &&
>+             result != nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING) {
Why not just check mSearchesOngoing == 0 without the other ONGOING check? If result /was/ ONGOING, mSearchesOngoing would not be 0.
(Assignee)

Comment 4

7 years ago
Created attachment 573603 [details] [diff] [review]
NO_MATCH in Multiple AutoComplete Searches Cancels Others

- Removed redundant ONGOING condition
Attachment #572614 - Attachment is obsolete: true
Attachment #573603 - Flags: review?(edilee)
Attachment #572614 - Flags: review?(edilee)
(Reporter)

Comment 5

7 years ago
Comment on attachment 573603 [details] [diff] [review]
NO_MATCH in Multiple AutoComplete Searches Cancels Others

I'm not sure what's the styling rules are anymore about bracing single line ifs.
Attachment #573603 - Flags: review?(gavin.sharp)
Attachment #573603 - Flags: review?(edilee)
Attachment #573603 - Flags: feedback+
(In reply to Felix Fung (:felix) from comment #2)
> I don't think individual searches should return NO_MATCH_ONGOING because
> that would require an individual search to know of the status of other
> searches in some way or another.

RESULT_NOMATCH_ONGOING means that the individual search hasn't yet produced results, but is still attempting to. I'm not sure why this is useful to indicate, though. The controller can't really do anything in this case, and there's no reason to notify if there are no new results, AFAICT...
Comment on attachment 573603 [details] [diff] [review]
NO_MATCH in Multiple AutoComplete Searches Cancels Others

Seems like it should be pretty easy to write a test for this (similar to test_378079.js). r=me with that!
Attachment #573603 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/913ee424d1d2
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

7 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.