:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 3 • 17 years ago

My patch in bug 405235 includes a change similar to this one that would also fix this bug.

	Mike Connor [:mconnor]
	Comment 4 • 17 years ago

is this fixed now?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 5 • 17 years ago

No, needs a separate patch given the solution we chose in bug 398020.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 6 • 17 years ago

This patch fixes urlbar-result-popup's onPopupClick to have it check for the search bar in a different way (using BrowserSearch.searchBar), which fixes this bug. It also changes the urlbar-result-popup binding's name to reflect the fact that it's no longer used for the URL bar, but instead only for search bar and content autocomplete, and simplifies logic in the two onPopupClick methods so it's clearer what's going on.

While writing this patch I noticed that the base binding's onPopupClick method doesn't pay attention to which button is being clicked, and just calls handleEnter unconditionally. This seems wrong, and doesn't appear to match the behavior of IE (or of any of the browser autocomplete users with this patch applied). I'll file a followup on that.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 7 • 17 years ago

(In reply to comment #6)
> This patch fixes urlbar-result-popup's onPopupClick to have it check for the
> search bar in a different way (using BrowserSearch.searchBar)

Which I should point out was stolen shamelessly from Dao's patch :)

	(not reading, please use seth@sspitzer.org instead)
	Comment 8 • 17 years ago

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

r=sspitzer, sorry for the delay gavin.

One question:

> binding's name to reflect the fact that it's no longer used for the URL bar

I like the renaming, but keep in mind that we do use it still for the "history" autocomplete when choosing the home page in the pref ui and for the Open Location dialog when the navigation bar is hidden.

As you mention in comment #6, after you patch, middle click will not do anything, as we no longer call handleEnter().  

It sounds like you intentionally wanted that for content autocomplete, but perhaps you didn't intend to do that for the other two "history" autocomplete instances (mentioned above).  

Without your patch, for those other two instances, middle click acts like a left click (and "selects" the item I've clicked.)

Was that intentional, or is that what you intend to log the follow up on?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 9 • 17 years ago

(In reply to comment #8)
> I like the renaming, but keep in mind that we do use it still for the "history"
> autocomplete when choosing the home page in the pref ui and for the Open
> Location dialog when the navigation bar is hidden.

Ah, indeed... I think my new name is generic enough to still be OK, but perhaps we should make a list of the users in a comment above the binding.

> As you mention in comment #6, after you patch, middle click will not do
> anything, as we no longer call handleEnter().

Actually, for the location bar and search bar case middle click should take the openUILink path that handles middle clicks properly (opens new tab). Seems to work for me with the patch applied... did you see something different?

	(not reading, please use seth@sspitzer.org instead)
	Comment 10 • 17 years ago

> we should make a list of the users in a comment above the binding.

Yes, I think that would be a good idea.

> did you see something different?

I haven't tested, but eyeballing your patch, with your fix, middle click will:

1) search:   open new tab
2) url bar:  open new tab
3) content:  nothing, but that's intentional, right?
4) history autocomplete in pref UI:  nothing, as we don't call handleEnter(), was this unintentional?
5) open location:  nothing, as we don't call handleEnter(), was this unintentional?

For #4 and #5, without your patch, middle click acts like left click and will select the clicked item (and fill in the input.)  

If we aren't calling handleEnter() anymore, it wouldn't do that, right?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 11 • 17 years ago

(In reply to comment #10)
> 1) search:   open new tab
> 2) url bar:  open new tab
> 3) content:  nothing, but that's intentional, right?
> 4) history autocomplete in pref UI:  nothing, as we don't call handleEnter(),
> was this unintentional?
> 5) open location:  nothing, as we don't call handleEnter(), was this
> unintentional?

Oh, I see! Thanks for clarifying. That was indeed an intentional change - I think that by default only left click should select entries in autocomplete popups, except for in the location bar and search bar where middleclick is a known way to open things in new tabs. This matches the behavior I get with IE's autocomplete (middle and right click on the popup are ignored in content and in the location bar) and seems reasonable to me. Since my patch only affects the few popups where these bindings are used, my followup bug was going to be about extending this behavior change to all autocomplete users (by making a similar change in the base autocomplete.xml binding).

	(not reading, please use seth@sspitzer.org instead)
	Comment 12 • 17 years ago

gavin:  thanks for elaborating.

can you add a comment to the binding listing all the known users before checking in?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 13 • 17 years ago

(In reply to comment #8)
> I like the renaming, but keep in mind that we do use it still for the "history"
> autocomplete when choosing the home page in the pref ui and for the Open
> Location dialog when the navigation bar is hidden.

Hmm, now that I look into this a bit more, is this really true? The binding is applied using an ID selector matching "PopupAutoComplete", and those two users:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/preferences/main.xul&rev=1.16&mark=118-120#118
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/openLocation.xul&rev=1.9&mark=70-74#70

don't seem to specify an autocomplete popup (meaning the get their own popup by default, per http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.98&mark=109,113-118#107 .

Am I missing something?

	(not reading, please use seth@sspitzer.org instead)
	Comment 14 • 17 years ago

> Am I missing something?

No, you are right and I was mistaken.  I forgot that that search, browser content autocomplete (and before the change for rich url bar autocomplete), url bar autocomplete share the PopupAutoComplete popup.  (Now, rich url bar uses it's own, PopupAutoCompleteRichResult)

I tested your patch, and confirmed that your change doesn't impact those other instances (location dialog and pref UI).

but now I have a concern about your patch.  Does it really open the proper thing in a new tab when I middle click?  In your search box, type in "tom slick"

The old code did:

-              var search = controller.getValueAt(this.selectedIndex);
-              var textbox = this.mInput;
-
-              // close the autocomplete popup and copy the selected value to the search box
-              this.closePopup();
-              textbox.value = search;
-              // open the search results according to the clicking subtlety
-              var where = whereToOpenLink(aEvent, false, true);
-              textbox._getParentSearchbar().doSearch(search, where);

in this case, search would be the value of what we middle clicked (like "tom slick cartoon")

with the old code, I see how we would have done the search.

With the new code, I don't see how this will work:

+            // Handle search bar popup clicks
+            var url = controller.getValueAt(this.selectedIndex);
+  
+            // close the autocomplete popup and revert the entered address
+            this.closePopup();
+            controller.handleEscape();
+
+            // respect the usual clicking subtleties
+            openUILink(url, aEvent);

url would be "tom slick cartoon", which if you pass to openUILink() would give you and alert "The URL is not valid and cannot be loaded."

Do you see that too?

	(not reading, please use seth@sspitzer.org instead)
	Comment 15 • 17 years ago

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

clearing review request, based on my last comment

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 16 • 17 years ago

Heh, oops. That is indeed a copy-paste error on my part. Good catch!

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 17 • 17 years ago

Fixes the rather embarrassing copy/pasto from the previous patch, and adds a comment about where the binding is used.

	(not reading, please use seth@sspitzer.org instead)
	Comment 18 • 17 years ago

Comment on attachment 292773 [details] [diff] [review]
updated patch

r=sspitzer

I've also tested and the the url bar and search bar handle middle click as open in new tab (as do the open location and home page pref ui, will you be logging a spin off bug to change the default behaviour?), and autocomplete in web content ignores middle click

note, in the search bar block of code, you have:

"// close the autocomplete popup and revert the entered address"

You probably want something like:

"// close the autocomplete popup and revert the entered search value"

or something, right?

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 19 • 17 years ago

(In reply to comment #18)
> I've also tested and the the url bar and search bar handle middle click as open
> in new tab (as do the open location and home page pref ui, will you be logging
> a spin off bug to change the default behaviour?)

You mean the open location and home page pref ui treat middleclick the same as left click, right? That's the current expected behavior, and that's what my followup will be about, yes (it will also be about fixing other autocomplete users to ignore middle clicks by default).

> note, in the search bar block of code, you have:
> 
> "// close the autocomplete popup and revert the entered address"
> 
> You probably want something like:
> 
> "// close the autocomplete popup and revert the entered search value"
> 
> or something, right?

Yeah, I'll make that change. This also brings up something else: now that we use openUILink Shift+Click is also supported, and opens the results in a new window. Not sure whether we should be entering the search text in the originating window's search bar in that case. Maybe we should just not bother entering the into the search bar at all. I also noticed that we don't go through handleSearchCommand in search.xml for these searches, so we don't save the search terms in the searchbar form history. I'll file followups on both of those.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 20 • 17 years ago

I've filed bug 408246 and bug 408251 about the two search bar issues I mentioned, and bug 408253 on the general autocomplete issue.

	:Gavin Sharp [email: gavin@gavinsharp.com] Assignee
	Comment 21 • 17 years ago

mozilla/browser/base/content/browser.css 	1.44 	mozilla/browser/base/content/urlbarBindings.xml 	1.49 	mozilla/browser/components/search/content/search.xml 	1.114 

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 22 • 17 years ago

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

	(not reading, please use seth@sspitzer.org instead)
	Comment 23 • 17 years ago

> You mean the open location and home page pref ui treat middleclick the same as
> left click, right?

yes.

And, thanks for logging those spin off bugs.

	Mike Lierman
	Comment 24 • 17 years ago

*** VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3