513180 - When browser.search.openintab == true, and you click the search magnifier instead of enter, it does not open in a new tab

Status: VERIFIED FIXED

(Firefox :: Search, defect, P5)

Description

[Daniel Akins]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

When browser.search.openintab == true, and you click the search magnifier instead of enter, it does not open in a new tab. Normally when I use this feature, I just click enter after typing. But today, when I was being lazy and using only my mouse (and copy and paste) I tried to just click the magnifier, and it did not open in a new tab. I user set the boolean to true. 

Reproducible: Always

Steps to Reproduce:
1. Turn on browser.search.openintab to true from about:config
2. Put something in the search bar
3. Click the magnifying glass
Actual Results:  
It opens the result in the tab you are in. 

Expected Results:  
It should open the search in a new tab like hitting enter does. 

Thanks!

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Correct. We will take care of it in bug 476907.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry I meant bug 237027.

Comment 3

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

This isn't related to bug 237027 - that bug is about the Ctrl/Alt difference between the location bar and the search bar.

This bug is about clicking not obeying the preference, as far as I can tell. Could potentially be affected by a fix to that bug, but it's not a prerequisite.

Comment 4

[Thomas D. (:thomas8)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

confirming for windows xp, was confirmed already per comment #1

Comment 5

[Thomas D. (:thomas8)]

can someone confirm platform/os all? (can't test)

Comment 6

[Luke Iliffe (Harlequin99)]

Can confirm on Win 7 RTM 32 and 64 bit

Comment 7

[Luke Iliffe (Harlequin99)]

Doh - sorry missread comment 4, saw bug was inital raised on Win 7

Comment 9

[mmortal03]

Confirmed on Vista 32-bit.

Comment 11

[Tim (fmdeveloper)]

Still an issue on Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110924 Firefox/9.0a1 ID:20110924030858

Think the relevant code is somewhere around http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#470

Comment 12

[cat_in_136]

In my understanding, browser.search.openintab does not affect the search magnifier icon behavior. (i.e. the icon's behavior is same as hyperlink.)

For example,
 - Click: does not open a tab;
 - Ctrl+Click: new tab;
 - Shift+Click: new window.

Is my understanding correct?

Comment 13

[bbird]

Yes.  Your understanding is correct.  Clicking the search magnifier icon should be consistent with pressing enter.  Hence, the bug.

Comment 14

[Olaf Schulz]

confirmed on
USER_AGENT=Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0

Comment 15

[Hlsg]

9 years and still no love for this bug?

Comment 16

[Temisan Iwere]

Hi, I'd like to take on this bug as a first time contributor. Just for clarification, we want to make search results open in new tap when you click on the magnifier?

Comment 17

[Temisan Iwere]

Attachment: Patch file for 513180

This is my attempt to fix this bug, however, I need some extra help, are any mentors available to assist me with this? This is my first time working on bugzilla

Comment 18

[Drew Willcoxon :adw]

Hi Temisan, thanks for the patch. I'm sorry I haven't looked at it yet, but I'll try to get to it on Monday.

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 9048717 [details] [diff] [review]
Patch file for 513180

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

Hi Temisan, it looks like you might be working with an old version of the Firefox code since newer versions don't have the search.xml code referenced in your patch.  Please take a look at these links for introductory info on contributing patches to Firefox.  You'll want to clone the mozilla-central repository, if you haven't already.

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

As for your patch, you're definitely looking in the right place, searchbar.js.  I think we'll want to modify the handleSearchCommand method, here: https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/components/search/content/searchbar.js#225

I think what we should do is move the true-branch of the if-statement -- the part that handles clicks on search-go-button -- into the final else-branch, after the part that checks for the pref.  We have other prefs for handling clicks on things, and that's what whereToOpenLink checks (see https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/base/content/utilityOverlay.js#184 ).  But this search-specific pref should probably override those other more general ones.  So i.e.:

if (aForceNewTab) {
  ...
} else {
  let newTabPref = ...;
  if (((aEvent instanceof KeyboardEvent && aEvent.altKey) ...
  }
  if ((aEvent instanceof MouseEvent) ..
  }
  // move the true-branch code down here:
  else if (aEvent.originalTarget.classList.contains("search-go-button")) {
    if (aEvent.button == 2)
      return;
    where = whereToOpenLink(aEvent, false, true);
  }
}

Please let me know if you have any questions.

Comment 20

[Temisan Iwere]

(In reply to Drew Willcoxon :adw from comment #19)

Comment on attachment 9048717 [details] [diff] [review]
Patch file for 513180

Review of attachment 9048717 [details] [diff] [review]:

Hi Temisan, it looks like you might be working with an old version of the
Firefox code since newer versions don't have the search.xml code referenced
in your patch. Please take a look at these links for introductory info on
contributing patches to Firefox. You'll want to clone the mozilla-central
repository, if you haven't already.

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

As for your patch, you're definitely looking in the right place,
searchbar.js. I think we'll want to modify the handleSearchCommand method,
here:
https://searchfox.org/mozilla-central/rev/
f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/components/search/content/
searchbar.js#225

I think what we should do is move the true-branch of the if-statement -- the
part that handles clicks on search-go-button -- into the final else-branch,
after the part that checks for the pref. We have other prefs for handling
clicks on things, and that's what whereToOpenLink checks (see
https://searchfox.org/mozilla-central/rev/
f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/base/content/utilityOverlay.
js#184 ). But this search-specific pref should probably override those
other more general ones. So i.e.:

if (aForceNewTab) {
...
} else {
let newTabPref = ...;
if (((aEvent instanceof KeyboardEvent && aEvent.altKey) ...
}
if ((aEvent instanceof MouseEvent) ..
}
// move the true-branch code down here:
else if (aEvent.originalTarget.classList.contains("search-go-button")) {
if (aEvent.button == 2)
return;
where = whereToOpenLink(aEvent, false, true);
}
}

Please let me know if you have any questions.

Hi Drew,

Thanks so much for your help! I thought I had old source code too because there were a lot of inconsistencies and I struggled to find the right file. To get the source code, I followed the instructions found here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial

I'm not sure why it's outdated, do you suggest I delete and repeat the process again? I'll work on getting the latest code and making those changes!

Just to clarify I only need to make changes in searchbar.js handleSearchCommand method right? And not the utilityoverlay.js?

Comment 21

[Drew Willcoxon :adw]

I was thinking it may be outdated because you cloned a repo other than mozilla-central. There are multiple Firefox repos available for various versions, so I wanted to make sure you used the one we use for development, mozilla-central. If you followed those directions, then you should have mozilla-central, so maybe search.xml changed between the time you cloned the repo and now, I'm not sure.

Once you have mozilla-central, you can update it to the latest version by unapplying your patch and then hg pull -u in the mozilla-central directory (and then reapplying your patch to start working on it again). There's more on mercurial/hg here:

https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial
https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html

Or if you prefer working with git: https://developer.mozilla.org/en-US/docs/Mozilla/Git

(In reply to Temisan Iwere from comment #20)

Just to clarify I only need to make changes in searchbar.js
handleSearchCommand method right? And not the utilityoverlay.js?

Yes, correct. At least that's what it looks like to me just reading the code. We may end up finding that something else needs to be modified too once you/we test your patch, making the changes I've suggested, if it doesn't work quite right.

Comment 22

[Temisan Iwere]

Attachment: Patch - revision 2

I have made those changes and attached the updated patch file. Please review and let me know if it's fine

Comment 23

[Drew Willcoxon :adw]

Comment on attachment 9052143 [details] [diff] [review]
Patch - revision 2

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

This doesn't quite fix the bug.  Were you able to build your changes and then test them?  Your patch is missing an `else` in the last branch that I suggested, but even when I add it, it doesn't work, and that's my fault.  The first inner `if` is correctly hit, but then the last `else if` is also hit, which clobbers the `where = "tab"` assignment.

I ended up taking a closer look at all the logic here in this method, and it's kind of a mess.  I'll have to get back to you when I get more time to think about it.

Comment 24

[Temisan Iwere]

(In reply to Drew Willcoxon :adw from comment #23)

Comment on attachment 9052143 [details] [diff] [review]
Patch - revision 2

Review of attachment 9052143 [details] [diff] [review]:

This doesn't quite fix the bug. Were you able to build your changes and
then test them? Your patch is missing an else in the last branch that I
suggested, but even when I add it, it doesn't work, and that's my fault.
The first inner if is correctly hit, but then the last else if is also
hit, which clobbers the where = "tab" assignment.

I ended up taking a closer look at all the logic here in this method, and
it's kind of a mess. I'll have to get back to you when I get more time to
think about it.

Hi Drew,

Please let me know if you have any update or advice on this bug

Comment 25

[Drew Willcoxon :adw]

I'm sorry for the big delay. Let's just make this as simple as possible, without breaking tests and possibly unintentionally people's habits, and do this:

if (aEvent && aEvent.originalTarget.classList.contains("search-go-button")) {
  if (aEvent.button == 2)
    return;
  // this `if` branch is copied from below
  let newTabPref = Services.prefs.getBoolPref("browser.search.openintab");
  if (((aEvent instanceof KeyboardEvent && aEvent.altKey) ^ newTabPref) &&
    !gBrowser.selectedTab.isEmpty) {
    where = "tab";
  // this `else` branch is new
  } else {
    // move the `where` assignment down here
    where = whereToOpenLink(aEvent, false, true);
  }
// everything else below is the same as now
} else ...

I've tested this and it works, and I pushed it to tryserver to make sure it doesn't break our automated tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0038bc4c86c25a108f8004d375c6bfc800a669ee

Please manually test your patch to make sure it works before you post it. You can use mach build faster to rebuild Firefox with your changes. The build docs linked above have more info. You should see the expected results in comment 0. Note though that if the current tab is "empty" (meaning, the home tab), it's expected that a new tab won't be opened.

Comment 26

[TheBinocular]

This is still broken by the way.

Comment 27

[Tanner Drake]

It looks like this is still open, I'd like to submit a patch to finally put it to rest if that's ok?

Comment 28

[Drew Willcoxon :adw]

Sure, it looks like this code hasn't changed much if any since the last comments here, so my advice earlier in the bug should still be OK.

Comment 29

[Tanner Drake]

Attachment: Bug 513180 - Add identical if check from key press to search-go-button press for newTabPref r=adw

Comment 30

[Drew Willcoxon :adw]

Attachment: openinnewtab.txt

Thanks, I tried the patch, and now that I see it in action, there's an improvement we can make. I tested all possible combinations of the following:

• browser.search.openintab set to false vs. true
• Using the mouse to click the arrow button vs. hitting the enter key
• All key modifiers: Alt, Shift, Accel, and no modifier
• Starting on an "empty" page (about:newtab) vs. starting on a normal page (google.com)
• Firefox's current behavior vs. with the patch

Everything worked well except for the case where browser.search.openintab = true, starting on a non-empty page and using the mouse. The results with current Firefox vs. the patch were:

            |  Current      |  Patch
------------+---------------+---------
No modifier |  Current tab  |  New tab
Alt         |  Current tab  |  New tab
Shift       |  New window   |  New tab
Accel       |  New tab      |  New tab

Regardless of the key modifier, the patch always opens a new tab. All we want to do is flip the "No modifier" row to "New tab" and keep the other rows the same. i.e., with no modifier, we open open a new tab, and with Alt, we do the opposite and use the current tab. Other than that, we don't want to break people's habits except for the change required to fix this bug.

I tried this, and it works:

      if (
        aEvent &&
        aEvent.originalTarget.classList.contains("search-go-button")
      ) {
        if (aEvent.button == 2) {
          return;
        }
        where = whereToOpenLink(aEvent, false, true);
        if (
          newTabPref &&
          !aEvent.altKey &&
          !aEvent.getModifierState("AltGraph") &&
          where == "current" &&
          !gBrowser.selectedTab.isEmpty
        ) {
          where = "tab";
        }
      } else if (aForceNewTab) {
        // Everything the same below except for moving the `newTabPref` above

I pushed a patch with this change to tryserver to make sure tests don't fail, and so far it looks good: https://phabricator.services.mozilla.com/D90586

For reference, attached are similar tables for all possible combinations of factors. "New" means the the code that I sketched above.

Comment 31

[Tanner Drake]

Attachment: Bug 513180 - Address reviewer comments regarding not breaking current user searching habits r=adw

Depends on D90586

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4c430e03eca
Add identical if check from key press to search-go-button press for newTabPref r=adw

Comment 33

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/a4c430e03eca

Comment 35

[Florencia Di Ciocco, NI to vbalducci]

Hi, I just verified, this has been fixed on FF 85.0a1 (2020-12-01) (64-bit) and on Release 83.0 (64-bit).

Regards, Flor.