We should probably even keep the dropdown open in those cases, so that it is easy to perform a search on multiple engines.

This looks like a good first bug for new contributors, would you like to mentor it ?

Is this a javascript bug? I can knock this one out. I already know the prefs that affect it. I wrote something for an addon for the old search bar to respect mid click: https://github.com/Noitidart/Swisearch/blob/master/bootstrap.js#L70

(In reply to Tim Nguyen [:ntim] from comment #2) > This looks like a good first bug for new contributors, would you like to > mentor it ? I can mentor this, yes. Note that we will probably want to get this bug done relatively quickly, and uplift to 35 (so aurora and beta).

(In reply to noitidart from comment #3) > Is this a javascript bug? Yes. The code handling the clicks is at http://hg.mozilla.org/mozilla-central/annotate/08be3008650f/browser/base/content/urlbarBindings.xml#l1185

Is bug 1107135 related?

Since I have a patch up and waiting for review on bug 1106101, I'm going to try tackling this one…

Hey Florian, was something like this what you were thinking of? Thanks, Blake.

Comment on attachment 8536820 [details] [diff] [review] Attempt number 1. Review of attachment 8536820 [details] [diff] [review]: ----------------------------------------------------------------- Why is the check for aEvent.altKey removed? alt+enter should still open the search in a new tab.

I discussed that with Madhava, and it didn't really make sense to either of us, given the new behaviour. (I would happily revert it, at the cost of more code, but I want to hear from Philipp whether that's desirable behaviour or not first. :) (Oh, perhaps alt+enter should open in a new tab, but not alt+click? And cmd/ctrl+click, but not cmd/ctrl+enter?)

(In reply to Blake Winton (:bwinton) from comment #12) > (Oh, perhaps alt+enter should open in a new tab, but not alt+click? And > cmd/ctrl+click, but not cmd/ctrl+enter?) Right, the function is called with both keyboard and mouse events, the modifiers don't have the same effect on both. We should not regress alt+enter, which opens in a new tab in both the urlbar and searchbar.

Okay, that sounds reasonable. I'll post a new patch with that behaviour tomorrow. :)

So, I kind of went down the wrong path, but now I've got cmd/ctrl clicks on autocomplete items also opening up in a new tab. Is that something we also want?

And this is the patch that re-adds alt+enter, and enables ctrl/cmd-click on suggestions.

(In reply to Blake Winton (:bwinton) from comment #15) > now I've got cmd/ctrl clicks on > autocomplete items also opening up in a new tab. Is that something we also > want? Isn't this the existing behavior?

Comment on attachment 8537227 [details] [diff] [review] bug1106101.diff Looks good to me! We might want to look into removing the flicker that happens when cmd-clicking something (the panel disappears and re-appears), but that's fine for now.

(In reply to Florian Quèze [:florian] [:flo] from comment #17) > (In reply to Blake Winton (:bwinton) from comment #15) > > now I've got cmd/ctrl clicks on > > autocomplete items also opening up in a new tab. Is that something we also > > want? > Isn't this the existing behavior? Sorry, opening up in a new *background* tab.

(In reply to Philipp Sackl [:phlsa] on PTO until Jan 7 from comment #18) > We might want to look into removing the flicker that happens when > cmd-clicking something (the panel disappears and re-appears) Is this something we can get by just replacing the .focus() call with aEvent.preventDefault() ?

I don't believe so, because the popup is getting closed by the "w.focus()" at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#308 (And I'm hesitant to add search-related logic to "openLinkIn". ;)

(Drats, I meant the "w.gBrowser.selectedBrowser.focus();" at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#349 .)

(In reply to Blake Winton (:bwinton) from comment #22) > (Drats, I meant the "w.gBrowser.selectedBrowser.focus();" at > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/ > utilityOverlay.js#349 .) I don't really understand why we run that focus call when loadInBackground is true, but I'm not really confident suggesting we change that on beta :-/.

Comment on attachment 8537227 [details] [diff] [review] bug1106101.diff Review of attachment 8537227 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updated patch! :) The one issue I see when applying this patch is that if I Command+click on a search suggestion, the suggestion gets filled in the textfield, and when the panel reopens, the suggestions (now based on the previously command-clicked suggestion) are different; and useless. Maybe a preventDefault() can stop this behavior. If we can't figure out a solution, I think we should take (and uplift) only the search.xml changes. The following comments are minor details. ::: browser/base/content/urlbarBindings.xml @@ +935,5 @@ > // Handle search bar popup clicks > var search = controller.getValueAt(this.selectedIndex); > > + // Open ctrl/cmd clicks on autocomplete items in a new background tab. > + let accel = aEvent && (aEvent.button !== undefined); I dislike the |aEvent.button !== undefined|, I think you meant |aEvent instanceof MouseEvent|. @@ +950,5 @@ > searchBar.value = search; > > // open the search results according to the clicking subtlety > var where = whereToOpenLink(aEvent, false, true); > + if (accel && where == "tab") accel is only used here, so setting it just before this line would be more readable (to me at least). I would possibly also just inline it: if (where == "tab" && aEvent instanceof MouseEvent && (aEvent.button == 1 || #ifdef XP_MACOSX aEvent.metaKey)) #else aEvent.ctrlKey)) #endif ::: browser/components/search/content/search.xml @@ +479,5 @@ > + accel = accel && (aEvent.metaKey || aEvent.button == 1); > +#else > + accel = accel && (aEvent.ctrlKey || aEvent.button == 1); > +#endif > + let alt = aEvent && aEvent.key == "Enter" && aEvent.altKey; I think |aEvent && aEvent.key == "Enter"| can be replaced with |aEvent instanceof KeyboardEvent|. @@ +487,5 @@ > where = whereToOpenLink(aEvent, false, true); > } > else { > var newTabPref = textBox._prefBranch.getBoolPref("browser.search.openintab"); > + if (alt ^ newTabPref) alt is only used here, you can inline it. @@ +492,2 @@ > where = "tab"; > + if (accel) accel is only used here, you can inline it, or move it closer. @@ +524,5 @@ > // null parameter below specifies HTML response for search > + let params = { > + postData: submission.postData, > + inBackground: aWhere == "tab-background" > + } nit: missing ; @@ +526,5 @@ > + postData: submission.postData, > + inBackground: aWhere == "tab-background" > + } > + openUILinkIn(submission.uri.spec, > + aWhere == "tab-background" ? "tab" : aWhere, I think the coding style in the rest of the file is to align the second and following lines of parameters with the first parameter.

(In reply to Florian Quèze [:florian] [:flo] from comment #24) > The one issue I see when applying this patch is that if I Command+click on a > search suggestion, the suggestion gets filled in the textfield, and when the > panel reopens, the suggestions (now based on the previously command-clicked > suggestion) are different; and useless. Maybe a preventDefault() can stop > this behavior. If we can't figure out a solution, I think we should take > (and uplift) only the search.xml changes. Fixed! :) > The following comments are minor details. > ::: browser/base/content/urlbarBindings.xml > @@ +935,5 @@ > > + let accel = aEvent && (aEvent.button !== undefined); > I dislike the |aEvent.button !== undefined|, I think you meant |aEvent > instanceof MouseEvent|. Fixed. And thank you! That's _way_ better! :D > @@ +950,5 @@ > > + if (accel && where == "tab") > accel is only used here, so setting it just before this line would be more > readable (to me at least). I would possibly also just inline it: > if (where == "tab" && aEvent instanceof MouseEvent && > (aEvent.button == 1 || > #ifdef XP_MACOSX > aEvent.metaKey)) > #else > aEvent.ctrlKey)) > #endif I agree. Inlined! > ::: browser/components/search/content/search.xml > @@ +479,5 @@ > > + accel = accel && (aEvent.metaKey || aEvent.button == 1); > > +#else > > + accel = accel && (aEvent.ctrlKey || aEvent.button == 1); > > +#endif > > + let alt = aEvent && aEvent.key == "Enter" && aEvent.altKey; > I think |aEvent && aEvent.key == "Enter"| can be replaced with |aEvent > instanceof KeyboardEvent|. I worry about having that get key events that aren't "Enter", but the code that calls us seems to take care of that, so change made! > @@ +487,5 @@ > > + if (alt ^ newTabPref) > alt is only used here, you can inline it. Inlined! > @@ +492,2 @@ > > + if (accel) > accel is only used here, you can inline it, or move it closer. Inlined, as above. > @@ +524,5 @@ > > + let params = { > > + postData: submission.postData, > > + inBackground: aWhere == "tab-background" > > + } > nit: missing ; Fixed. > @@ +526,5 @@ > > + openUILinkIn(submission.uri.spec, > > + aWhere == "tab-background" ? "tab" : aWhere, > I think the coding style in the rest of the file is to align the second and > following lines of parameters with the first parameter. Changed! And I think that's all!

Comment on attachment 8537883 [details] [diff] [review] The second version of the patch. Review of attachment 8537883 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Could you please file a bug to find a longer term replacement for the .focus calls? ::: browser/base/content/urlbarBindings.xml @@ +952,5 @@ > +#endif > + where = "tab-background"; > + else > + // Fill in the search bar's value if we're not doing a background search. > + searchBar.value = search; Can you please move this else... @@ +958,3 @@ > searchBar.doSearch(search, where); > + if (where == "tab-background") > + searchBar.focus(); ... to here, where you can drop the comment because it will be close enough to the "if (where == "tab-background")" test that it's self explanatory.

And here's the final version. I'm running the tests at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d30eea91220e but it should be good to check in if they pass successfully. :)

And now that the try-server is fairly happy, here's a better version, with a header to make our tree-herder's lives easier! ;) (Carrying forward ui-r=phlsa and r=florian)

Comment on attachment 8538077 [details] [diff] [review] The final version, ready for checkin. Approval Request Comment [Feature/regressing bug #]: 1106101 [User impact if declined]: Worse search UI [Describe test coverage new/current, TBPL]: Manual testing on aurora and beta. [Risks and why]: I think this is fairly low risk, since it's mostly javascript, and adds features which most people won't notice. :) [String/UUID change made/needed]: No.

[Tracking Requested - why for this release]: this is a search fix we want in 35.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b84ed5f4ae0 https://hg.mozilla.org/releases/mozilla-beta/rev/4cde4d970ae6

Verified fixed on Firefox 35.0b6 (20141222200458) using Ubuntu 12.04 LTS 32-bit, Windows 8.1 64-bit and Mac OS X 10.9.5. Blake, one thing I noticed is that the search panel is closed after pressing middle-click and then reopened again. Is this behavior intended for ctrl/cmd+click / middle-click on one-off searches? (i.e. shouldn't it stay opened permanently while performing this action?)

(In reply to Andrei Vaida, QA [:avaida] from comment #34) > Blake, one thing I noticed is that the search panel is closed after pressing > middle-click and then reopened again. Is this behavior intended for > ctrl/cmd+click / middle-click on one-off searches? (i.e. shouldn't it stay > opened permanently while performing this action?) This is a known issue, mentioned at comment 18, then we discussed the technical details of it from comment 20 to comment 23 and decided a fix for that would be too risky for a patch uplifted to beta. I asked Blake to file a bug to improve this in the future in comment 26. You can file the bug for Blake if you want :-).

Filed as bug 1115009. :)

Verified as fixed using Developer Edition 36.0a2 and Nightly 37.0a1 2014-01-05 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

A user reported on SuMo (https://support.mozilla.org/questions/1050466) that in Fx36 browser.tabs.loadInBackground=false is ignored for middle-clicks on search suggestions. They always load in the background. That seems to be true in my testing. Is this a new bug or by design?

I _believe_ this is by design, but if you wanted to file a bug, we could discuss it there…