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

NEW
Unassigned

Status

()

defect
P5
normal
10 years ago
23 days ago

People

(Reporter: danman71188, Unassigned, Mentored)

Tracking

Trunk
Desktop
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

10 years ago
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
Last Resolved: 10 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 → ---

Comment 4

10 years ago
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

Comment 5

10 years ago
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

Comment 9

8 years ago
Confirmed on Vista 32-bit.

Updated

8 years ago
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

Comment 12

7 years ago
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

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

Comment 14

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

Updated

2 years ago
See Also: → 344381

Updated

2 years ago
Blocks: 1383669
Blocks: 1420969

Comment 15

10 months ago
9 years and still no love for this bug?

Comment 16

3 months ago

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

3 months ago
Posted 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.

Updated

2 months ago
Flags: needinfo?(adw)

Updated

2 months ago
OS: Windows 7 → All
Priority: -- → P5
Hardware: x86 → Desktop

Updated

2 months ago
Mentor: adw

Comment 20

2 months ago

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

Comment 22

2 months ago

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

Updated

2 months ago
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-

Updated

2 months ago
Flags: needinfo?(adw)

Comment 24

a month ago

(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)
You need to log in before you can comment on or make changes to this bug.