Closed Bug 823990 Opened 11 years ago Closed 11 years ago

Work - Search options disappear when typing in the Firefox app bar

Categories

(Toolkit :: Autocomplete, defect)

All
Windows 8.1
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla21

People

(Reporter: benjamin, Assigned: bbondy)

References

Details

(Whiteboard: feature=work status=verified)

Attachments

(1 file, 3 obsolete files)

I just got Windows 8 on a new family computer and installed an Elm build. My daughter couldn't figure out how to search: when she first opened the location bar, there was a "google search" option in the suggestions. But when she started typing "cool math games" the option to search google for "cool math games" didn't come up as we expected it to.

I see bug 782801 for search *suggestions*, but she didn't really need search suggestions, just the ability to search.
It looks like we switch from the "suggestion" view back to the "new tab" view if there are no bookmark/history matches for what you've typed.  Instead we should continue to show the "suggestion" view so the search engine UI remains visible.
Assignee: nobody → mbrubeck
OS: Linux → Windows 8 Metro
Hardware: x86_64 → All
Whiteboard: [metro-mvp][LOE:1][metro-it2]
We are using a <textbox autocompletesearch="history">, so nsAutoCompleteController searches for history results only.  Our own code separately updates the search engine list.  If nsAutoCompleteController does not see any history results, it calls closePopup.

To do this properly, we should probably implement another nsIAutoCompleteSearch component to provide the search suggestion results, so the controllor is aware of those results too.
The fix for this is slightly more involved than I hoped.  Pushing to it3.
Status: NEW → ASSIGNED
Whiteboard: [metro-mvp][LOE:1][metro-it2] → [metro-mvp][LOE:2][metro-it2][metro-it3]
Assignee: mbrubeck → netzen
nsAutoCompleteController.cpp auto closes the autocomplete popup when there are no results left.
The URL autocomplete bar is hooked up to nsPlacesAutoComplete.js which finds there are no results invoking the nsAutoCompleteController code to close the popup.
The fix is either to ignore calls to closePopup inside the nsIAutoCompleteSearch handler inside metro code under certain conditions, or else to
pass some flag to nsAutoCompleteController to handle this.  I think I'll do the former.
Attached patch Patch v1. (obsolete) — Splinter Review
With the Metro autocomplete results we also display search engine buttons beside the results, so we don't want it to automatically close the popup when no results are found.  This adds an option which can be set to stop closing the popup.
Attachment #703572 - Flags: review?(mak77)
Depends on: 831910
Attached patch Patch v2. (obsolete) — Splinter Review
Added some extra functionality only for that new option to show the popup if not already shown.  Combining it with the existing patch instead of posting a new bug.
Attachment #703572 - Attachment is obsolete: true
Attachment #703572 - Flags: review?(mak77)
Attachment #703689 - Flags: review?(mak77)
Blocks: 831903
No longer depends on: 831910
Whiteboard: [metro-mvp][LOE:2][metro-it2][metro-it3] → [metro-mvp][LOE:2][metro-it2][metro-it3] feature=work
Comment on attachment 703689 [details] [diff] [review]
Patch v2.

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

at first glance looks like it may do what you are asking for, but this code is really fragile and has so many edge cases I will surely require a second check from soneone else, either Enn or Gavin (since you need a SR for this I suppose Gavin would be best choice).

It's a bit hard to figure out if this is the right approach, since I don't know what's the code using this, looks like you are injecting something into the popup, how does that injecting work, could it be implemented as an additional search instead so that the popup always contains results from it? If you could point me to the autocomplete using this, that'd be appreciated.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +568,5 @@
>    if (index >= (int32_t)mRowCount)
>      index = mRowCount - 1;
>  
> +  bool persistOnNoResults = false;
> +  mInput->GetPersistOnNoResults(&persistOnNoResults);

this should use input, not mInput.

@@ +590,5 @@
>      // Nothing left in the popup, clear any pending search timers and
>      // close the popup.
>      ClearSearchTimer();
>      ClosePopup();
>    }

I think you should still invoke ClearSearchTimer even if persistOnNoResults is true.

@@ +1353,5 @@
>      if (mRowCount) {
>        OpenPopup();
>      } else if (result != nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING) {
> +      bool persistOnNoResults = false;
> +      mInput->GetPersistOnNoResults(&persistOnNoResults);

should use input, not mInput

@@ +1361,5 @@
> +        bool isOpen = false;
> +        input->GetPopupOpen(&isOpen);
> +        if (!isOpen) {
> +          OpenPopup();
> +        }

wouldn't be clearer to read persist earlier and do

if (mRowCount || persistOnNoResults) {
  open
} else {
  close
}

why do we have to check if it's already open, afaik all the input implementations before doing anything check if the popup is already open and just no-op in such a case.

@@ +1407,5 @@
> +      input->GetPopupOpen(&isOpen);
> +      if (!isOpen) {
> +        OpenPopup();
> +      }
> +    }

ditto for isOpen.
and if you invert the above if as I suggested, I'd invert this as well so both check persistOnNoResults or !persistOnNoResults in a coherent way.

::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl
@@ +49,5 @@
>     */
>    attribute boolean forceComplete;
> +
> +  /*
> +   * Option to keep the autocomplete window open when there are no results

what does "window" mean here, maybe you meant popup?
Attachment #703689 - Flags: review?(mak77)
Attached patch Patch v3 (obsolete) — Splinter Review
Implemented review comments.
Thanks for the review, ya I'll get Gavin to do a super review after this reaches r+.

The code that's using this simply sets the value for persistOnNoResult to true.
You can see the changes which uses this code here: bug 831532. 
And you can find the related code in browser/metro on elm: https://hg.mozilla.org/projects/elm

In all non-metro cases, as far as I can see, sets it to false.
Having persistOnNoResult false should leave the logic unchanged from before to minimize the chance of regressions.
Attachment #703689 - Attachment is obsolete: true
Attachment #704880 - Flags: review?(mak77)
(In reply to Brian R. Bondy [:bbondy] from comment #8)
> In all non-metro cases, as far as I can see, sets it to false.
> Having persistOnNoResult false should leave the logic unchanged from before
> to minimize the chance of regressions.

Yeah I'm not really worried of breaking existing consumers (considered the patch), more whether this is the right approach to take.
For example if minResultsForPopup is 0, that is your case, it could already make sense to keep the popup open, even if there are no results, since the popup is expected even for zero results.

I actually think closing the popup on no results is a bug, id minResultsForPopup == 0.

Hijacking that existing property would allow to avoid persistOnNoResult.
Btw, in both cases the hooking points are the same, so I'm going to review the patch as-is and let Gavin evaluate if it's worth the added attribute or we should rather just fix minResultsForPopup (I'm honestly more prone to this).
Comment on attachment 704880 [details] [diff] [review]
Patch v3

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

The hook points look correct, a test in toolkit/components/autocomplete/tests/unit/, similar to the existing ones, would be appreciated.

When asking for SR, please point Gavin to comment 9.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +568,5 @@
>    if (index >= (int32_t)mRowCount)
>      index = mRowCount - 1;
>  
> +  bool persistOnNoResults = false;
> +  input->GetPersistOnNoResults(&persistOnNoResults);

why do we have to do this here instead of just before its use, inside the else?

@@ +1393,5 @@
>        mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_NO_MATCH;
>    } else {
>      mSearchStatus = nsIAutoCompleteController::STATUS_COMPLETE_NO_MATCH;
> +    bool persistOnNoResults = false;
> +    mInput->GetPersistOnNoResults(&persistOnNoResults);

should use input, not mInput
Attachment #704880 - Flags: review?(mak77) → review+
Ya I thought of using minResultsForPopup == 0 as well but wasn't sure if anyone else did this as well and wanted different behavior.
I don't see why someone who wants to always show the popup would want it to be closed, indeed I think they currently workaround that by reopening the panel when the search finishes.
Comment on attachment 704880 [details] [diff] [review]
Patch v3

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

Hey Gavin, could you see Comment 9 and let me know if you'd prefer me to change how minResultsForPopup == 0 works instead of the approach I took of adding a new option? Thanks!
Attachment #704880 - Flags: feedback?(gavin.sharp)
Attached patch Patch v4Splinter Review
I think it's better to do as you suggested and fix the bug with minresults == 0 instead of adding a new option.  It simplifies the patch a lot.  That way I don't think we need a super review as well.
Attachment #704880 - Attachment is obsolete: true
Attachment #704880 - Flags: feedback?(gavin.sharp)
Attachment #706890 - Flags: review?(mak77)
https://hg.mozilla.org/projects/elm/rev/76c57e5b8a96
Will sync review comments to elm
Whiteboard: [metro-mvp][LOE:2][metro-it2][metro-it3] feature=work → [metro-mvp][LOE:2][metro-it2][metro-it3][completed-elm] feature=work
Summary: Search options disappear when typing in the location bar → Work - Search options disappear when typing in the location bar
Whiteboard: [metro-mvp][LOE:2][metro-it2][metro-it3][completed-elm] feature=work → [completed-elm] feature=work
Closing the autocomplete popup (and dismissing the location bar) by pressing Escape no longer works correctly since this patch.
(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> Closing the autocomplete popup (and dismissing the location bar) by pressing
> Escape no longer works correctly since this patch.

It looks like it was actually bug 831532 that caused this regression.
Whiteboard: [completed-elm] feature=work → feature=work [completed-elm]
Comment on attachment 706890 [details] [diff] [review]
Patch v4

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

It looks pretty good, sorry for late, I was traveling.
Attachment #706890 - Flags: review?(mak77) → review+
Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Component: Browser → Autocomplete
Product: Firefox for Metro → Toolkit
Resolution: FIXED → ---
Summary: Work - Search options disappear when typing in the location bar → Work - Search options disappear when typing in the Metro location bar
Version: unspecified → Trunk
Depends on: 836129
Matt, didn't you say that bug 836129 had been caused by bug 831532 and not this one?
Actually I'm not really sure, since I realized after that comment that the code in this bug isn't even used unless the patch from bug 831532 is applied.  I think both bugs' patches are required to produce the regression.
I'll wait to land this until I investigate and fix bug 836129 just in case this code has to change.
Backed out changes from elm for now:
https://hg.mozilla.org/projects/elm/rev/27e22a0d37b9

I'll either repost patch for review if needed, or re-land after bug 836129 is done.
Whiteboard: feature=work [completed-elm] → feature=work
Re-landed, will land on m-c shortly so keeping this open:
https://hg.mozilla.org/projects/elm/rev/2563bca995db
(In reply to Brian R. Bondy from comment #11)
> Ya I thought of using minResultsForPopup == 0 as well but wasn't sure if
> anyone else did this as well and wanted different behavior.

FYI XPFE autocomplete as used by SeaMonkey opens the popup if minResultsForPopup == 0 (although Thunderbird currently uses XPFE autocomplete it never sets minResultsForPopup to zero).
Summary: Work - Search options disappear when typing in the Metro location bar → Work - Search options disappear when typing in the Firefox app bar
https://hg.mozilla.org/mozilla-central/rev/ed412c2a599d
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Tested on 2013-02-13 Nightly built from http://hg.mozilla.org/project/elm/rev/b3e65fe37681
- Typed the term "mozilla" in the Firefox app bar and the search options remain.
Status: RESOLVED → VERIFIED
Whiteboard: feature=work → feature=work status=verified
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.