Closed Bug 515635 Opened 10 years ago Closed 10 years ago

"Search Google for ..." context menu should open tab next to current active tab

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: martijn.martijn, Assigned: jmorkel)

References

Details

(Keywords: verified1.9.2)

Attachments

(2 files, 4 obsolete files)

..just like IE does.

I think this is especially important now that bug 465673 is fixed. Current behavior seems rather inconsistent to me.
Flags: blocking-firefox3.6?
I agree.  Most likely what would be searching for is related to the current content on the page you were on.  An example would be if you are reading a news story with a word you don't understand, select it and search google for it.  The new tab should open next to the current so when you close the tab, you won't be far from the parent tab.

Would be better if tab closing order would be fixed but the point still stands so the user doesn't have to scroll back down the tab strip to continue reading the news story.
Sounds right to me; definitely wanted. John, would you like to continue your great work here?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
I'll be happy to.
Assignee: nobody → jmorkel
Status: NEW → ASSIGNED
Implements the new tab behaviour as per bug 465673 for the context menu search. Depends on the preference browser.tabs.insertRelatedAfterCurrent.
Attachment #399985 - Flags: ui-review?(beltzner)
Attachment #399985 - Flags: review?(dao)
Comment on attachment 399985 [details] [diff] [review]
Insert related "Search Google for x" searches next to current tab

this doesn't need ui review
Attachment #399985 - Flags: ui-review?(beltzner)
With this patch we will also send a referrer now. See also bug 454518 comment 17.
OS: Windows XP → All
Hardware: x86 → All
I've filed bug 515932 to solve this without sending a referrer.
Depends on: 515932
Comment on attachment 399985 [details] [diff] [review]
Insert related "Search Google for x" searches next to current tab

I don't think we want to send a referrer to google in this case.
Attachment #399985 - Flags: review?(dao) → review-
John, are you going to write a new patch?
Component: Tabbed Browser → Search
QA Contact: tabbed.browser → search
This patch makes use of the new relatedToCurrent parameter from bug 515932 to open search tabs opened from the "Search google for x" content menu option.
Attachment #399985 - Attachment is obsolete: true
Attachment #401023 - Flags: review?(dao)
Comment on attachment 401023 [details] [diff] [review]
Tab that uses relatedToCurrent instead of passing referrer

without tabpref, just relatedToCurrent: true.

Thanks!
Attachment #401023 - Flags: review?(dao) → review+
Incorporates review suggestion.
Attachment #401023 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/86a9fa87ea59
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Attachment #401039 - Flags: approval1.9.2?
Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090920 Minefield/3.7a1pre ID:20090920031129

This is a feature a lot of people are using. So it would be nice to see an automated test. I will also flag in-litmus because AFAIR at least one existing test needs an update once it has been landed on 1.9.2.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
Mochitest for checking tab positions of tabs opened by BrowserSearch.loadTab function which is called from the context menu when text is selected.

Is this adequate? The mouse movements don't need to be simulated do they?
Attachment #401849 - Flags: review?(dao)
Comment on attachment 401849 [details] [diff] [review]
Tests for related search tab position

>+  function cleanUp(aTabs) {
>+    aTabs.forEach(gBrowser.removeTab, gBrowser);
>+  }
>+
>+  function tabAdded(event) {
>+    let tab = event.target;
>+    tabs.push(tab);
>+  }
>+
>+  function openTabs() {
>+    gBrowser.addTab("about:blank");
>+    gBrowser.selectedTab = gBrowser.mTabs[0];

this line isn't needed

cleanUp and openTabs don't need to exist as functions, as they are called only once.
- Removed cleanUp and openTabs functions.
- Removed line gBrowser.selectedTab = gBrowser.mTabs[0];
Attachment #401849 - Attachment is obsolete: true
Attachment #402829 - Flags: review?(dao)
Attachment #401849 - Flags: review?(dao)
Attachment #402829 - Flags: review?(dao) → review+
Comment on attachment 402829 [details] [diff] [review]
Tests for related search tab position (updated)

>+                 browser_relatedSearch.js \

relatedSearch doesn't really tell me what that file is about. Maybe contextSearchTabPosition or something like that would be better...

>+  is(tabs[0], gBrowser.mTabs[3], "blank tab opens to the right of first tab");

"blank tab has been pushed to the end"

>+  is(tabs[1], gBrowser.mTabs[1], "search tab opens to the right of first tab");

"first search tab opens next to the current tab"

>+  is(tabs[2], gBrowser.mTabs[2], "search tab opens to the right of first search tab");

"second search tab opens next to the first search tab"
- Renamed test file to browser_contextSearchTabPosition.js
- Changed test strings as per review.
Attachment #402829 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8a9f8280678a
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Attachment #401039 - Flags: approval1.9.2? → approval1.9.2+
(In reply to comment #11)
> (From update of attachment 401023 [details] [diff] [review])
> without tabpref, just relatedToCurrent: true.

Reading that we always wanna open the search page right next to the currently open tab? When I disable the pref the search page gets opened at the end of the tab bar. Is that expected?
That's expected.
Then it is verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b6pre) Gecko/20091222 Namoroka/3.6b6pre ID:20091222033648
Keywords: verified1.9.2
Even it is covered by automation I have added this part as an expected result for the Litmus tests on all branches:

https://litmus.mozilla.org/show_test.cgi?id=10231
https://litmus.mozilla.org/show_test.cgi?id=8243
https://litmus.mozilla.org/show_test.cgi?id=6201
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.