Closed Bug 513180 Opened 12 years ago Closed 8 months ago

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

Categories

(Firefox :: Search, defect, P5)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
83 Branch
Tracking Status
firefox83 --- verified
firefox84 --- unaffected
firefox85 --- unaffected

People

(Reporter: danman71188, Assigned: tanner.drake, Mentored)

References

Details

Attachments

(3 files, 2 obsolete files)

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!
Correct. We will take care of it in bug 476907.
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 476907
Sorry I meant bug 237027.
Duplicate of bug: 237027
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.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
can someone confirm platform/os all? (can't test)
Severity: major → normal
Can confirm on Win 7 RTM 32 and 64 bit
Doh - sorry missread comment 4, saw bug was inital raised on Win 7
Duplicate of this bug: 599187
Confirmed on Vista 32-bit.
Duplicate of this bug: 688983
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
Version: unspecified → Trunk
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?
Yes.  Your understanding is correct.  Clicking the search magnifier icon should be consistent with pressing enter.  Hence, the bug.
confirmed on
USER_AGENT=Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
See Also: → 344381
Blocks: 1383669
Blocks: 1420969
9 years and still no love for this bug?

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?

Attached patch Patch file for 513180 (obsolete) — Splinter Review

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

Flags: needinfo?(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 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.
Flags: needinfo?(adw)
OS: Windows 7 → All
Priority: -- → P5
Hardware: x86 → Desktop
Mentor: adw

(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?

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.

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

Attachment #9048717 - Attachment is obsolete: true
Attachment #9052143 - Flags: feedback?(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.
Attachment #9052143 - Flags: feedback?(adw) → feedback-
Flags: needinfo?(adw)

(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

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.

Flags: needinfo?(adw)

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

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.

Assignee: nobody → tanner.drake
Status: NEW → ASSIGNED
Attached file 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.

Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4c430e03eca
Add identical if check from key press to search-go-button press for newTabPref r=adw
Status: ASSIGNED → RESOLVED
Closed: 12 years ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Attachment #9176771 - Attachment is obsolete: true
Duplicate of this bug: 1427913

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.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.