405235 - search bar autocomplete popup (history, suggestions) doesn't work

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Richard Marti (:Paenglab)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre

Since the landing of Bug 398020 the search suggestions don't appears

Reproducible: Always

Steps to Reproduce:
1. Choose Google as search engine
2. Search for example the word mozilla
Actual Results:  
No list with search examples appears

Expected Results:  
List with search examples appears

Comment 1

[Jo Hermans]

Works for me, using Google, Yahoo and Wikipedia
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre

Comment 2

[Richard Marti (:Paenglab)]

Strange, now it works for me again.
I tested it with a new profile and the hourlys. It never worked with the hourlys after the landing of Bug 398020.

I close this bug

Comment 3

[Ria Klaassen (not reading all bugmail)]

I can reproduce this on Windows XP. Curious to know what made it work for the reporter; maybe only in shiny new profiles? :).
Regression range is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1195882020&maxdate=1195884479

Comment 4

[Richard Marti (:Paenglab)]

Ria try to change the search engine and then back again. I am not shure, but I think, I've done this before the check after the post from Jo.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Search suggestions work for me using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre .

Extension-related, perhaps? Any errors in Error Console (with javascript.options.showInConsole set)?.

Comment 6

[Reed Loden [:reed]]

WFM on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112404 Minefield/3.0b2pre ID:2007112404

Comment 7

[Ria Klaassen (not reading all bugmail)]

No, new profile, first run. I tried changing search engines, but no effect. The previous build worked.
What could be going on.

Comment 8

[Richard Marti (:Paenglab)]

I think I found it: open FF, then try to search something -> no suggestions. Now type a letter into the url bar (no enter needed), go to the search bar and search again -> voila the suggestions appears.

Tryed with a new profile and my old one. It's the same.

Comment 9

[Ria Klaassen (not reading all bugmail)]

Yes exactly.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> I think I found it: open FF, then try to search something -> no suggestions.

Does this require your search history to be empty? If it isn't empty, do the history entries show up, but not suggestions?

Comment 11

[aja+bugzilla]

Once the workaround in comment #8 is performed, I get both search history and suggestions.  Prior to workaround, neither.

Comment 12

[Richard Marti (:Paenglab)]

(In reply to comment #10)
> (In reply to comment #8)
> > I think I found it: open FF, then try to search something -> no suggestions.
> 
> Does this require your search history to be empty? If it isn't empty, do the
> history entries show up, but not suggestions?

It doesn't matter. As I wrote, no difference if a new or a old profile with search history. It appears no list (no suggestions and no history).

IMHO it has something to do with the new mechanism of the go-button. Only after the button appears one time all works as exepted.

Comment 13

[Dão Gottwald [:dao]]

The same is happening for the content area due to bug 402668.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Why did the landing of bug 398020 trigger this? Or is that just wrong?

Comment 15

[Dão Gottwald [:dao]]

Because the search bar isn't using the openAutocompletePopup method from urlbar-result-popup.

But I'm not sure anymore that this caused a problem for the content area -- I can't reproduce this right now.

Comment 16

[(not reading, please use [e-mail] instead)]

marking as all, as I see this on the mac as well.

note, as dao points out, in search.xml we have an openPopup method now:

http://lxr.mozilla.org/seamonkey/source/browser/components/search/content/search.xml#650

should the fix be to add something this this to openPopup() in search.xml?

+          // initially the panel is hidden
+          // to avoid impacting startup / new window performance
+          popup.hidden = false;

Comment 17

[(not reading, please use [e-mail] instead)]

that seems to fix it for me, I'll attach a patch and seek review from dao / gavin.

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 290062 [details] [diff] [review]
patch

This works, but I'd only do it if the popup isn't open.

Comment 20

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, per dao

note to dao, I have not been able to reproduce this problem with the content area.

Comment 21

[Dão Gottwald [:dao]]

When I wrote comment 13, autocomplete in the content area started working once I had typed in the Location bar. But it seems that this was random. I know I've seen that problem several times before, but it comes and goes unpredictably. Anyway, it's most likely not a regression from bug 402668.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 290063 [details] [diff] [review]
updated patch, per dao

If the search bar isn't using the urlbar-result-poppup anymore, that's the bug we should fix.

The check-in for bug 398020 does appear to regress bug 295498 for the search bar.

Comment 23

[(not reading, please use [e-mail] instead)]

Comment on attachment 290063 [details] [diff] [review]
updated patch, per dao

obsoleting, based on gavin's comment.

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: remove the openPopup in search.xml

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 290068 [details] [diff] [review]
remove the openPopup in search.xml

This regresses the alignment of the popup (its left edge doesn't match up with the left edge of the typable area). Per discussion with dao on IRC I think we want to factor out the parts of _openAutocompletePopup in autocomplete.xml that get the information used to calculate width and the context element to pass to openPopup and have the search bar binding override that explcitly.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate approach

This also fixes bug 405269.

<gavin> dao: http://pastebin.mozilla.org/248842
<dao> gavin, this.mInput.className == "searchbar-textbox" looks fragile, too
<gavin> well, I guess so
<gavin> could use the code from your patch
<gavin> that uses browsersearch
<gavin> the other thing is that urlbar-result-popup now overrides both _setWidthAndOpenPopup and openAutocompletePopup
<gavin> which is kind of messy
<dao> I kinda like it

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #26)
> <gavin> could use the code from your patch
> <gavin> that uses browsersearch

(attachment 290071 [details] [diff] [review])

Comment 28

[:Gavin Sharp [email: [e-mail]]]

I also want to improve the comments before landing this, because I did this quick. I just wanted to get it up here and see what you guys think of the approach in general.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #26)
> <gavin> the other thing is that urlbar-result-popup now overrides both
> _setWidthAndOpenPopup and openAutocompletePopup
> <gavin> which is kind of messy
> <dao> I kinda like it

what I like is the _setWidthAndOpenPopup idea, not that urlbar-result-popup is becoming messy :)

Comment 30

[(not reading, please use [e-mail] instead)]

1)

I see what dao was pointing out (about having search.xml depend on
urlbarBindings.xml)

but I guess we already have code that does that in urlbarBindings.xml (in the
onPopupClick method)

is this necessary because the searchbar binding, unlike the urlbar binding,
doesn't extend the autocomplete binding?

2)

Good point / comment about:  

// Fall through for now, bug 397594 might change this

if this lands, we should go put a comment in that bug referring back to this
code.

Over to gavin.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 290077 [details] [diff] [review]
alternate approach

Using BrowserSearch.searchBar for the comparison and doSearch still seems better to me. What you have now would support multiple search bars, but I don't think we need that (as with the urlbar).

The final patch should probably remove openPopup from search.xml.

And yes, it's confusing that this binding is called "urlbar-result-popup" and lives is in urlbarBindings.xml.

Comment 32

[Dão Gottwald [:dao]]

btw, I think you don't need to override openAutocompletePopup anymore ... |this.hidden = false| in _setWidthAndOpenPopup should do the trick.

Comment 33

[(not reading, please use [e-mail] instead)]

Comment on attachment 290077 [details] [diff] [review]
alternate approach

clearing review request, sounds like gavin / dao have a plan of attack.

Comment 35

[RenegadeX]

I filed Bug 406303, having never experienced this issue before the 20071130 build on WinXP. Before filing it, I went back and installed 20071129, no problem whatsoever, no need to do any of the steps listed in this(405235) Bug to reproduce the condition. Then just now saw my Bug had been marked as a Duplicate of this, so tried all of the suggested Steps To Reproduce, and nothing works. Therefore I believe 406303 to be a separate issue, and valid.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #35)
> Therefore I believe 406303 to be a separate issue, and valid.

If you make the location bar dropdown appear (by typing something into the location bar, or clicking the dropdown button), can you reproduce bug 406303?

Comment 37

[RenegadeX]

(In reply to comment #36)
> If you make the location bar dropdown appear (by typing something into the
> location bar, or clicking the dropdown button), can you reproduce bug 406303?

Yes. Sorry if I was unclear. To restate what I observe: starting with 20071130, no matter what I do, which includes typing in the Location Bar (observing the Star change to the 'Go' arrow), submitting what I've typed in the LB either by pressing Enter or by clicking the 'Go' button, clicking the LB dropdown, clicking the LB dropdown and selecting and visiting a displayed item, etc -- Google Suggest does not work, and no errors are present in the Error Console. Hence, why I filed Bug 406303 as a separate Bug.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Ah, what about content autocomplete? If you get the dropdown to show up for a text input or password field, I bet it will work then. The patch for bug 399664 made us use a different popup for the location bar, so that hackaround doesn't work anymore.

Comment 39

[RenegadeX]

Ah very good.(In reply to comment #38)
> Ah, what about content autocomplete?

Ah, super good call! I think you just cracked this one + as a bonus, made a interesting discovery concerning Bug 404135 ("Restore Default Set.. breaks the location bar and the search bar"). The reason I'd come across both is because I was using new profiles to test and therefore had no search/content-autocomplete history; when there is an autocomplete history available, the Bug is not present!

Steps to Reproduce what I was seeing in Bug 406303:
1. Create a brand new profile. Start Minefield.
2. In the *toolbar* Search Box with the default "Google" engine already selected, start typing a word, ex: "firefox" but do *not* hit Enter or click the Search Submit button; expected behaviour: as you type the Google Suggest pop-up should appear with list of suggestions; actual behaviour: nothing appears.
4. backspace/delete all of the typed characters
5. The 'firstrun' page that loads by default - http://www.mozilla.org/projects/firefox/3.0b2pre/firstrun/ - will do for this next step - we need a page with a content-area text box, such as a searchbox.
- Enter a word, such as "firefox" into the *in-page* text-box - and press Enter/Submit button.
6. Just to test things, click back in the toolbar Search Bar and start typing any word - eg: "firefox", but do not hit Enter. Still no Google Suggest appears. Delete all of the characters again.
7. Click the 'Back' button to return to the original page. The original word "firefox" will be present in the in-page text box.
8. Double-click the text box(/"firefox" text). The autocomplete dropdown appears below the textbox with the word "firefox" in it. We have now activated autocomplete!
9. Now focus the toolbar Search Bar, and start typing "firefox" again. BINGO! After a character or 2, Google Suggest kicks in!!


Now the really neat part (that ties in with Bug 404135) is that if you then:
10. Right click on a toolbar and choose 'Customize' to open the Customize Toolbar palette.
11. Click 'Restore Default Set', then 'Done' to exit the pane
12. Repeat Step#2 from above.... you'll notice Google Suggest is now broken
13. Then click in the Location Bar.... you'll notice it's now acting weird. Even the dropdown arrow no longer works. Highlight all the text in the LB and delete it.
14. Then type "firefox.com" and press Enter. Nothing happens!
15. Repeat steps #7 and #8; we have now re-activated autocomplete!
15. Repeat Step #14. It works! As does the Search Bar!

I'll post a note in 404135 pointing to this Bug. If I'm up the (wrong) garden path, let me know.. ;0)

Comment 40

[Richard Marti (:Paenglab)]

I can confirm this. With browser.urlbar.richResults=true, I have to use content autocomplete for working search bar autocomplete.
With browser.urlbar.richResults=false, using the url bar autocomplete is enough.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Attachment: yet another approach (diff -w)

Boris pointed out on IRC that bindings are attached whenever the node is accessed, to permit calling methods on the element even when it is display:none. This means we can use the binding constructor to unhide the panel and avoid having to do it from the openAutocompletePopup method, which in turn means that other users that override that method (or methods that call it), like the search bar, don't need to explicitly unhide the popup themselves.

The only question is whether this means we'll unhide the panel in the startup path, and thus lose the startup win from bug 402668. This could happen if the binding for the panel is applied on startup. That would only happen if someone tries to access the panel during startup, though, and I don't think that's the case. I'll have to watch closely at the perf numbers if I land this. If it does cause a perf regression we can go with one of Seth's earlier two patches for B2 while we figure out something better.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

I ran this latest patch through Txul locally, about 5 times with and 5 times without the patch, and was not able to identify a perf regression caused by the patch.

Comment 43

[(not reading, please use [e-mail] instead)]

Comment on attachment 291250 [details] [diff] [review]
yet another approach (diff -w)

r=sspitzer, thanks for double checking Txul locally.

Comment 44

[(not reading, please use [e-mail] instead)]

in bug #402668, I had the following note:

> 3)  extend this fix for the other popups / panels

following up:

1)  I think we have five panels, and you've fixed 3.

could we get some more txul / ts wins by doing this to:

<panel id="customizeToolbarSheetPopup" noautohide="true">

we also have, but I don't know if we are doing my hack or your better way, in
the ctor.

<panel id="identity-popup" position="after_start" hidden="true"
noautofocus="true">

2) what about for the other popups / tooltips in browser, in the mainPopupSet?

Thoughts?  I can make these spin off bugs.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Yeah, I think we should file some spinoff bugs on those... and probably one on making the hack unnecessary by improving panel initialization performance, if it isn't already filed.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

I tried landing this earlier today:
mozilla/toolkit/content/widgets/autocomplete.xml 	1.95
mozilla/browser/base/content/urlbarBindings.xml 	1.46

but had to back it out because it regressed perf. The binding is probably attached early because we need it to call attachToBrowser, which happens during startup. I suppose we should just land attachment 290063 [details] [diff] [review] for b2.

Comment 47

[(not reading, please use [e-mail] instead)]

gavin, fwiw, I agree that we should land https://bugzilla.mozilla.org/attachment.cgi?id=290063 (a less-than-perfect-fix) for this bug.

we could log a spin off bug about the other way (which is cleaner, but regresses Ts / Txul.)

as for comment #44, I think this makes my spin off bugs moot, except we can still reduce Ts / Txul by applying the "hidden trick" to <panel id="customizeToolbarSheetPopup" noautohide="true">.

I've logged bug #406860 on this issue.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Attachment: what I'll land

This is attachment 290063 [details] [diff] [review] with an adjusted comment.

Comment 49

[:Gavin Sharp [email: [e-mail]]]

Landed that patch for b2:
mozilla/browser/components/search/content/search.xml 	1.113

I'm kind of undecided about whether I really want to pursue landing the previous patch (attachment 291250 [details] [diff] [review]). This patch that did land (attachment 290063 [details] [diff] [review]) isn't really as messy as some of the alternatives, so I'm less concerned about cleaning this up than I was before, after having thought about this a bit. I think ideally we'd probably want to take something more like attachment 290077 [details] [diff] [review], but that's something for a followup.

Comment 50

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5017

in-litmus+

Comment 51

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre. I verified using stephen's test case in Comment 50 as well as running the sequence of steps in Comment 39.