Closed Bug 1106101 Opened 9 years ago Closed 9 years ago

Cmd/Ctrl-Click and middle-click on a one-off button should open the search in a new tab

Categories

(Firefox :: Search, defect)

36 Branch
defect
Not set
normal
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox35 + verified
firefox36 + verified
firefox37 --- verified

People

(Reporter: phlsa, Assigned: bwinton, Mentored)

References

Details

Attachments

(1 file, 4 obsolete files)

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 ?
Flags: needinfo?(florian)
Hardware: x86 → All
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).
Mentor: florian
Flags: needinfo?(florian)
(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
Flags: firefox-backlog+
Points: --- → 1
Since I have a patch up and waiting for review on bug 1106101, I'm going to try tackling this one…
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Attached patch Attempt number 1. (obsolete) — Splinter Review
Hey Florian, was something like this what you were thinking of?

Thanks,
Blake.
Attachment #8536820 - Flags: review?(florian)
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?)
Flags: needinfo?(philipp)
(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.  :)
Flags: needinfo?(philipp)
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?
Attached patch bug1106101.diff (obsolete) — Splinter Review
And this is the patch that re-adds alt+enter, and enables ctrl/cmd-click on suggestions.
Attachment #8536820 - Attachment is obsolete: true
Attachment #8536820 - Flags: review?(florian)
Attachment #8537227 - Flags: review?(florian)
(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.
Attachment #8537227 - Flags: ui-review+
(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.
Attachment #8537227 - Flags: review?(florian) → review-
Attached patch The second version of the patch. (obsolete) — Splinter Review
(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!
Attachment #8537227 - Attachment is obsolete: true
Attachment #8537883 - Flags: review?(florian)
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.
Attachment #8537883 - Flags: review?(florian) → review+
Attached patch The final version of the patch. (obsolete) — Splinter Review
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.  :)
Attachment #8537883 - Attachment is obsolete: true
Attachment #8538000 - Flags: review+
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)
Attachment #8538000 - Attachment is obsolete: true
Attachment #8538077 - Flags: ui-review+
Attachment #8538077 - Flags: review+
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.
Attachment #8538077 - Flags: approval-mozilla-beta?
Attachment #8538077 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/22701d94b200
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
QA Contact: petruta.rasa
[Tracking Requested - why for this release]: this is a search fix we want in 35.
Attachment #8538077 - Flags: approval-mozilla-beta?
Attachment #8538077 - Flags: approval-mozilla-beta+
Attachment #8538077 - Flags: approval-mozilla-aurora?
Attachment #8538077 - Flags: approval-mozilla-aurora+
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?)
Status: RESOLVED → VERIFIED
Flags: needinfo?(bwinton)
(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.  :)
Flags: needinfo?(bwinton)
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…
Depends on: 1138176
Depends on: 1236436
Depends on: 1115009
See Also: → 1316863
You need to log in before you can comment on or make changes to this bug.