1071558 - Middle and right click on search suggestions perform the search in the same tab on about:home/newtab

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Petruta Horea [:phorea], Desktop QA]

Reproducible on:
Firefox 33 beta 6 - 20140922173023
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
latest Aurora 34.0a2 - 20140922004004
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
latest Nightly 35.0a2 - 20140922030204
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Steps to reproduce:
1. Enter a string on about:home or about:newtab page 
2. Move the mouse on suggestions drop-down list
3. Click on them using the mouse middle and right click buttons

Actual results:
Middle and right click perform the search on the same tab. The contextual menu is opened and remains on the screen when right click is used.

Expected results:
On search toolbox from navigation toolbar, middle click on search suggestions opens a new tab and right click has no action.

Notes: It also occurs under Mac OSX 10.9.5 and Ubuntu 12.10 32-bit.

Comment 1

[Tracy Walker [:tracy]]

Please find the regression range.

Comment 2

[Drew Willcoxon :adw]

It's not really a regression because that's how search suggestions on about:home/newtab have always worked -- it's just a bug.  Should be very easy to fix.  _onMousedown should make sure event.button == 0: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/searchSuggestionUI.js?rev=37f902fba2de#230

This test should also be updated: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_searchSuggestionUI.js?rev=37f902fba2de#99

I'm reversing the dependency relationships to reflect the fact that this was caused by bug 612453, which implemented these suggestions.

Comment 3

[Drew Willcoxon :adw]

Although hopefully bug 1066787 will be fixed soon, making this one unnecessary.

Comment 4

[shreyas]

Hi, I would like to work in this bug? How should I go about it?

Thanks,
shreyas

Comment 5

[Drew Willcoxon :adw]

Hi shreyas, thanks for volunteering!

First you'll need to set up your computer to build Firefox.  Please see:

https://developer.mozilla.org/en-US/docs/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Once you've built Firefox, you can start working on the bug.

I think all we have to do here is ignore right-clicks.  I tested just now with middle-clicks, and they open the search results for the clicked suggestion in a new tab, like Petruta says, which I guess is fine behavior.  (And on second thought, I don't think the test needs to check this.)

So you'll need to modify the _onMousedown method that I linked to in comment 2 to return early when the event button is a right-click.  You can see which button value indicates a right-click here: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.button

Good luck, and let me know here in the bug when you have questions.  You can also join #fx-team on IRC.  I'm adw there.  https://wiki.mozilla.org/IRC

Comment 6

[Drew Willcoxon :adw]

Er, I'm wrong, middle-click opens search results in the same tab -- which is actually what Petruta said.

If we want to open them in a new tab, which we should probably, this is more work.  Currently the search suggestion controller doesn't forward any click info to clients, and neither of the two clients -- about:home and about:newtab -- are set up to open search results in a new tab.

So there are two parts to this: (1) Update _onMousedown to pass the click event to onClick, and (2) update about:home and about:newtab to open search results in a new tab when the click event is the middle button.

shreyas, about:home passes in its onClick to SearchSuggestionUIController here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js?rev=c8019c6eda14#368

onSearchSubmit already takes an event argument and calls preventDefault on it, which we should not do in this case.  Therefore onSearchSubmit will need to take a second clickEvent argument and add clickEvent.button to the eventData object.  The eventData object is eventually passed up from content to chrome, and ends up here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js?rev=29b05d283b00#446  And then here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/AboutHome.jsm?rev=5161e9b47a3c#181

So at that last link, you'll need to load the search URI in a new tab instead of in the current window.

Next, about:newtab passes its onClick to to SearchSuggestionUIController here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js?rev=f1798d79a96f#246

You can see that its search method also takes an event and calls preventDefault, so you'll have to add a second parameter again: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js?rev=f1798d79a96f#45

search.js then forwards the search event to chrome, where it ends up here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm

You'll need to modify _onMessageSearch to examine `data` to see which button, if any, was clicked, and then open a new tab in the middle-click case.

Comment 7

[shreyas]

I tried to reproduce the bug I am not able to reproduce it.
Apparently the middle click opens in a new tab and the right click does nothing.

Comment 8

[Drew Willcoxon :adw]

Really?  For Petruta and me, middle- and right-click both open the search results page of the clicked suggestion in the current tab.  I don't think there's a reason for that to be different for you.  Are you sure you're doing it right?  You go to about:home or about:newtab, type something in the search field in the page, and click one of the suggestions that pops up.

Comment 9

[Peter Myers]

My experience is the same as Shreya's (see 2015-02-11 07:01:21 PST above). I am not a technical person just a heavy user. Perhaps those who are looking into this problem can fix a similar bug introduced in very recent Firefox versions. If profile option "When I open a link in a new tab, switch to it immediately" is selected and user middle clicks one of the search suggestions in the search bar, a new tab is opened but only in the background. The new tab should be opened (switched to) in the foreground. Thank you.

Comment 10

[shreyas]

Attachment: bug1071558.diff

Reproduced the bug. 
Getting a "clickEvent is undefined on this patch"

Comment 11

[Peter Myers]

(In reply to shreyas from comment #10)
> Created attachment 8564721 [details] [diff] [review]
> bug1071558.diff
> 
> Reproduced the bug. 
> Getting a "clickEvent is undefined on this patch"

Shreyas: Will this fix the bug I documented above?

Comment 12

[shreyas]

No it does not fix the bug. I just submitted it for feedback on the progress I made and the errors I was getting.

Comment 13

[Peter Myers]

(In reply to shreyas from comment #12)
> No it does not fix the bug. I just submitted it for feedback on the progress
> I made and the errors I was getting.

Thanks, Shreyas. Hopefully this will be addressed by someone.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8564721 [details] [diff] [review]
bug1071558.diff

Review of attachment 8564721 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks shreyas, this is a good start.  In addition to my comments below, to fix the problem where right-clicking triggers the suggestion to be loaded, you need to modify searchSuggestionUI.js like I said in comment 5:

> So you'll need to modify the _onMousedown method that I linked to in comment
> 2 to return early when the event button is a right-click.  You can see which
> button value indicates a right-click here:
> https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.button

::: browser/base/content/abouthome/aboutHome.js
@@ +298,4 @@
>    }
>  }
>  
> +function onSearchSubmit(aEvent, clickEvent)

Sorry, I don't know what I was thinking when I said we need a second event argument.  That's not true.  We should call event.preventDefault() in the click case just like we do in other cases.  So in the click case, `aEvent` should be the click event.

@@ +310,5 @@
>  
>      let eventData = {
>        engineName: engineName,
> +      searchTerms: searchTerms,
> +      clickEventButton: clickEventButton,

Instead of passing the raw button like I suggested, let's pass a useNewTab bool.  That's what the chrome code really needs to know.  useNewTab should be true for the middle button and false for others.

::: browser/base/content/newtab/search.js
@@ +42,4 @@
>      });
>    },
>  
> +  search: function (event, clickEvent) {

Same here, no second click argument needed, sorry.

@@ +54,4 @@
>          engineName: this.currentEngineName,
>          searchString: searchStr,
>          whence: "newtab",
> +        clickEvent: clickEvent.button

Same here, pass a useNewTab bool instead.

::: browser/base/content/searchSuggestionUI.js
@@ +251,4 @@
>      this.input.setAttribute("selection-kind", "mouse");
>      this._hideSuggestions();
>      if (this.onClick) {
> +      this.onClick.call(event);

This is right, thanks.  Please update the comment near the top of the file, here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/searchSuggestionUI.js#32

The description of onClick should say that the function is passed one argument, the click event.

::: browser/modules/AboutHome.jsm
@@ +198,4 @@
>  #endif
>            // Trigger a search through nsISearchEngine.getSubmission()
>            let submission = engine.getSubmission(data.searchTerms, null, "homepage");
> +          window.addTab.loadURI(submission.uri.spec, null, submission.postData);

You don't want to unconditionally open a new tab (or unconditionally use the same tab like the current code does).  You added the clickEventButton property to the data (but please use useNewTab like I said), so you need to examine that property here to determine whether to use a new tab or the current tab.

You can see that there's a `data` object being used in the code in this function.  That's where your clickEventButton/useNewTab property is.

To open a new tab, you can use this openUILinkIn method defined on `window`.  The definition of openUILinkIn is here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js?rev=f6d678e3120b#191

Pass "tab" for `where` and false for aAllowThirdPartyFixup.  And of course the submission URL and the post data.

::: browser/modules/ContentSearch.jsm
@@ +224,5 @@
>      let win = browser.ownerDocument.defaultView;
>      win.BrowserSearch.recordSearchInHealthReport(engine, data.whence,
>                                                   data.selection || null);
> +    if(data == 0) {
> +      window.addTabloadURI(submission.uri.spec, null, submission.postData);

The line where ContentSearch loads the URI in the current tab is here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=d9adce044082#214  There's a try-catch around the line.  You want to keep that.

So around there is where you need to examine your useNewTab bool.  Again, you can see that there's a `data` object.  That's where useNewTab will be defined.

Opening a new tab in this case is different from the about:home case, but it's still simple.  See this line here that gets the current browser: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=d9adce044082#212

If useNewTab is true, what we should do is redefine that `browser` variable to be a new browser, and then after the try-catch, we should make `browser` the currently selected tab.

To get a new browser, do:

  browser.getTabBrowser().addTab().linkedBrowser

Set the return value to `browser`.  Then the current code that opens the URL in `browser` will just work.

To make the new browser the currently selected tab, do:

  browser.getTabBrowser().selectedTab = browser.linkedTab;

Comment 15

[shreyas]

Attachment: bug1071558.diff

Updated the patch, but there still seems to be some error.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8566196 [details] [diff] [review]
bug1071558.diff

Review of attachment 8566196 [details] [diff] [review]:
-----------------------------------------------------------------

What's the error?  I think this should work except for the stray onClick.

::: browser/base/content/abouthome/aboutHome.js
@@ +303,4 @@
>    let searchText = document.getElementById("searchText");
>    let searchTerms = searchText.value;
>    let engineName = document.documentElement.getAttribute("searchEngineName");
> +  let useNewTab = false;

Please move this definition inside the `if`.

@@ +307,5 @@
>    if (engineName && searchTerms.length > 0) {
>      // Send an event that will perform a search and Firefox Health Report will
>      // record that a search from about:home has occurred.
> +    if (aEvent) {
> +      this.onClick.call(aEvent);

There's no onClick defined here.  You don't need to call any click handler.  Just stick useNewTab in eventData.

::: browser/base/content/searchSuggestionUI.js
@@ +32,5 @@
>   * @param onClick
>   *        A function that's called when a search suggestion is clicked.  Ideally
>   *        we could call submit() on inputElement's ancestor form, but that
> + *        doesn't trigger submit listeners. The function is passed one argument, 
> +          the click event

Please remove the trailing space on the first line, add an asterisk to the front of the second line like all the other lines in this comment, and end the sentence with a period.

::: browser/modules/AboutHome.jsm
@@ +201,5 @@
> +          if (data.useNewTab == true) {
> +            window.openUILinkIn(submission.uri.spec, "tab", false, submission.postData);
> +          }
> +          else {
> +            window.loadURI(submission.uri.spec, null, submission.postData);

Actually now that I see this, we can use openUILinkIn in both cases.  In the same-tab case, pass "current" as the `where` argument.  Then you only need a conditional, or better yet a ternary, to define `where`.

::: browser/modules/ContentSearch.jsm
@@ +206,4 @@
>        "engineName",
>        "searchString",
>        "whence",
> +      "useNewTab",

This shouldn't be a required property.  If it's absent, then treat it as false.

@@ +210,5 @@
>      ]);
>      let engine = Services.search.getEngineByName(data.engineName);
>      let submission = engine.getSubmission(data.searchString, "", data.whence);
>      let browser = msg.target;
> +    if (data.useNewTab == true) {

if (data.useNewTab) {

@@ +225,4 @@
>        // method on it will throw.
>        return Promise.resolve();
>      }
> +    if (data.useNewTab == true) {

if (data.useNewTab) {

Comment 17

[shreyas]

Attachment: bug1071558.diff

Updated the patch , but it doesn't produce the expected results. I guess there is something wrong with the assignment of useNewTab.

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8566498 [details] [diff] [review]
bug1071558.diff

Review of attachment 8566498 [details] [diff] [review]:
-----------------------------------------------------------------

I noticed a couple of problems just now that I didn't before.

One is how you're calling onClick as described below.  The other is that we need to update the onClick function that about:newtab's passes to the SearchSuggestionUIController constructor.  Right now it's passing a function that doesn't expect any arguments.

You need to change this line: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js#247

To this:

event => this.search(event));

::: browser/base/content/abouthome/aboutHome.js
@@ +307,5 @@
>      // Send an event that will perform a search and Firefox Health Report will
>      // record that a search from about:home has occurred.
> +    let useNewTab = false;
> +    if (aEvent) {
> +      if (aEvent.button == 1) {

Please write this like:

let useNewTab = aEvent && aEvent.button == 1;

::: browser/base/content/newtab/search.js
@@ +52,5 @@
>      if (this.currentEngineName && searchStr.length) {
>  
> +    if (event) {
> +      if (event.button == 1) {
> +        useNewTab = true;

Same here, and please remember to indent properly since you're inside the conditional here.

::: browser/base/content/searchSuggestionUI.js
@@ +255,4 @@
>      this.input.setAttribute("selection-kind", "mouse");
>      this._hideSuggestions();
>      if (this.onClick) {
> +      this.onClick.call(event);

The first argument to `call` is the `this` argument that the method is bound to.  So what this does is event.onClick().  You need to keep the null and pass `event` as the second argument.  See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call

::: browser/modules/ContentSearch.jsm
@@ +224,5 @@
>        // method on it will throw.
>        return Promise.resolve();
>      }
> +    if (data.useNewTab) {
> +      browser.getTabBrowser().selectedTab = browser.linkedTab;

I told you the wrong thing here, sorry.  What you should do instead is save the return value of browser.getTabBrowser().addTab() above in a `newTab` variable.  And then here on this line, do:

browser.getTabBrowser().selectedTab = newTab;

Comment 19

[shreyas]

I made the changes but the patch doesn't seem to be working for about:newtab. For about:newtab it opens a newtab but loads the url in the existing tab.

Comment 20

[Drew Willcoxon :adw]

Could you post your new patch please?

Comment 21

[shreyas]

Attachment: bug1071558.diff

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 8566701 [details] [diff] [review]
bug1071558.diff

Review of attachment 8566701 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/abouthome/aboutHome.js
@@ +303,4 @@
>    let searchText = document.getElementById("searchText");
>    let searchTerms = searchText.value;
>    let engineName = document.documentElement.getAttribute("searchEngineName");
> +  

Please remove the trailing space here.

::: browser/base/content/newtab/search.js
@@ +48,4 @@
>      }
>      let searchText = this._nodes.text;
>      let searchStr = searchText.value;
> +    let useNewTab = event && event.button == 1;

Please move this definition inside the conditional.

::: browser/modules/ContentSearch.jsm
@@ +211,5 @@
>      let submission = engine.getSubmission(data.searchString, "", data.whence);
>      let browser = msg.target;
> +    let newTab;
> +    if (data.useNewTab) {
> +      newTab = browser.getTabBrowser().addTab().linkedBrowser;

You need to set newTab to the return value of addTab().  addTab returns a tab; linkedBrowser is a browser.

Comment 23

[shreyas]

Attachment: bug1071558.diff

Still doesn't work for about:newtab. The same problem persists.

Comment 24

[Drew Willcoxon :adw]

Comment on attachment 8566728 [details] [diff] [review]
bug1071558.diff

Review of attachment 8566728 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/abouthome/aboutHome.js
@@ -303,4 @@
>    let searchText = document.getElementById("searchText");
>    let searchTerms = searchText.value;
>    let engineName = document.documentElement.getAttribute("searchEngineName");
> -

Please revert this change.

::: browser/modules/ContentSearch.jsm
@@ +211,5 @@
>      let submission = engine.getSubmission(data.searchString, "", data.whence);
>      let browser = msg.target;
> +    let newTab;
> +    if (data.useNewTab) {
> +      newTab = browser.getTabBrowser().addTab();

It doesn't work because now you're not setting browser to newTab.linkedBrowser anymore.  You still need to do that.

Comment 25

[shreyas]

The patch works but I not able to revert the first change. Adding a newline and then doing a "hg qrefresh" doesn't seem to work.

Comment 26

[Drew Willcoxon :adw]

I don't know what to tell you.  Could you post the new patch?  I'll take care of it.

Comment 27

[shreyas]

Attachment: bug1071558.diff

Comment 28

[Drew Willcoxon :adw]

Attachment: landed patch with nits fixed

Here's the patch I landed with my nits fixed.

Thanks shreyas!  Congratulations on fixing two bugs. :-)

https://hg.mozilla.org/integration/fx-team/rev/2e9ff15fa9c7

Comment 29

[Peter Myers]

Shreyas and Drew: It looks like you both worked very diligently on these issues. I have no knowledge of anything you did, only to say how much I admire you abilities, not only technical, but also your patience and persistence and your ability to communicate well with each other. I was wondering if you could tell me if the bug I reported above is one of the two fixed. If so, could you estimate when it will be released in the product. And if you didn't get a chance to fix it, would you recommend the best way of reporting it. I did report it through Bugzilla but it was declared a duplicate of 1123442, but nobody seems to be working on that. So, is there a better way to submit bugs or should I submit it again. Anyway, thanks for all your work. I have used FF for many years and appreciate its many benefits.

Comment 30

[Drew Willcoxon :adw]

Hi Peter, no, the patch in this bug did not fix bug 1123442.  If you have already reported your bug and it was marked a duplicate, it would not be helpful to submit it again.  Unfortunately there are way many more bugs than there are people to work on them, and it looks like bug 1123442 is not a priority for paid Mozilla staff right now, although it's still very new.  Volunteers are always welcome to work on bugs though.

Comment 31

[Peter Myers]

Thanks, Drew for the speedy response. I hear what you say. I guess I'm a bit frustrated because one of the reasons I like FF is the the duel awesome bar/search bar concept and something recently broke in the search bar that prevents middle clicking a search suggestion, opening a new tab, and switching to it immediately. This is a feature that worked well for years. I did find an add-on that helps called Omnibar. This replaces the awesome bar and supports search suggestions. So, my life goes on, but long term I'd rather not have add-ons if I don't need them and particularly when I know FF should and always use to support a feature that is now broken. By the way I don't really believe bug 1123443 is a duplicate of the one I reported. Similar, but different. Perhaps fixing 1123442 will fix my issue, I have no way of knowing. Meanwhile I'll remain patient, try not to annoy anyone, and hope eventually the issue will be resolved. Kindest regards.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e9ff15fa9c7

Comment 33

[Petruta Horea [:phorea], Desktop QA]

Verified Nightly 38.0a1 build under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5 and noticed:
- Middle click opens the search page in a new tab in foreground - this is different from the search toolbar where the searches performed with middle click are opened in background tabs
- Right click on search suggestions now only opens the contextual menu of the page and dismisses the search suggestion list, while on older versions the searches were performed too

Comment 34

[Petruta Horea [:phorea], Desktop QA]

Drew, can you please check if the above issues worth filling? Thank you!

Comment 35

[Drew Willcoxon :adw]

(In reply to Petruta Rasa [QA] [:petruta] from comment #33)
> - Middle click opens the search page in a new tab in foreground

Oh... we should have made it open in the background... I didn't realize that.  Yes, could you file a new bug and mark it blocking this one, please?

> - Right click on search suggestions now only opens the contextual menu of
> the page and dismisses the search suggestion list, while on older versions
> the searches were performed too

That's the right behavior.

Comment 36

[Petruta Horea [:phorea], Desktop QA]

Thanks a lot for responses, Drew!

I logged bug 1137234 and marking this one as verified.