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)
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: benjamin, Assigned: bbondy)
References
Details
(Whiteboard: feature=work status=verified)
Attachments
(1 file, 3 obsolete files)
1.87 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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]
Comment 2•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: mbrubeck → netzen
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
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)
Updated•11 years ago
|
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Comment 9•11 years ago
|
||
(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 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Ya I thought of using minResultsForPopup == 0 as well but wasn't sure if anyone else did this as well and wanted different behavior.
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
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
Updated•11 years ago
|
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
Comment 16•11 years ago
|
||
Closing the autocomplete popup (and dismissing the location bar) by pressing Escape no longer works correctly since this patch.
Comment 17•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [completed-elm] feature=work → feature=work [completed-elm]
Comment 18•11 years ago
|
||
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+
Comment 19•11 years ago
|
||
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
Updated•11 years ago
|
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
Comment 20•11 years ago
|
||
Matt, didn't you say that bug 836129 had been caused by bug 831532 and not this one?
Comment 21•11 years ago
|
||
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.
Assignee | ||
Comment 22•11 years ago
|
||
I'll wait to land this until I investigate and fix bug 836129 just in case this code has to change.
Assignee | ||
Comment 23•11 years ago
|
||
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
Assignee | ||
Comment 24•11 years ago
|
||
Re-landed, will land on m-c shortly so keeping this open: https://hg.mozilla.org/projects/elm/rev/2563bca995db
Assignee | ||
Comment 25•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed412c2a599d
Target Milestone: --- → mozilla21
Comment 26•11 years ago
|
||
(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).
Updated•11 years ago
|
Summary: Work - Search options disappear when typing in the Metro location bar → Work - Search options disappear when typing in the Firefox app bar
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed412c2a599d
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 28•11 years ago
|
||
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
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•