385791 - Context-menu (Google) search should open in new tab/window

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Brandon Crawford]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.4) Gecko/20070509 Camino/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.4) Gecko/20070509 Camino/1.5

This behavior is already a feature of Firefox. When right-clicking on any word or phrase and selecting Search, the default action in Camino is to google for that specific word in the active tab. Firefox's default action is to google for that specific word in a new tab in the background. This is much more desirable when doing research when a page has multiple words or phrases which need further investigation without leaving the source page.

Reproducible: Always

Steps to Reproduce:
1. Visit page full of words you don't know
2. Highlight and right click on said word
3. Click 'Search'
Actual Results:  
Active page is replaced w/ Google search forcing a Back-button-press to return to the source page.

Expected Results:  
New tab should be created w/ the appropriate Google search leaving the active page in tact

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We already support using the Cmd-key modifier to open the search in a new tab, and none of the other context menu items default to opening in new foo without explicitly telling you so; doing so here would be inconsistent.

Comment 2

[Brandon Crawford]

Ok. Please open Camino and Firefox side by side. Visit http://www.google.com on both. Highlight the same word on both browers. Access the contextual menu via right-click. In Camino, click (with no modifiers) 'Search'. Now in Firefox, click (again, no modifiers) "Search Google for 'highlighted word'". I am currently using Camino 1.5 and Firefox 2.0.0.4. 

Comment 3

[Stuart Morgan]

Consistency with Firefox is not a UI goal for us; we aren't Firefox.

View source does open a new window/tab, so it's not totally unprecedented in our UI. I could go either way on this; we should discuss it at the next meeting.

Comment 4

[Chris Lawson (gone)]

Personally, I would find it very annoying if this opened a new tab every time (especially if there wasn't a way to prevent it from doing so with a modifier key, and any modifier key I can think of for accomplishing this would result in basically the opposite action that said modifier has everywhere else in Camino). I think we're doing well to stick with the "Command opens this in new foo" paradigm.

View Source seems like sort of a historical exception to me, although ISTR that it didn't always open a new tab in Camino (back when opening it in a tab was the only way to view source).

cl

Comment 5

[Brandon Crawford]

AFAIK (having just done it) the command modifier does not currently open it in a new tab. I'm not asking for a permanent change... just that it be considered as an option. 

Comment 6

[Stuart Morgan]

It does, but only if you press it before opening the menu; that's a known bug.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Confirming per today's meeting.

[12:54pm] smorgan: So shall we just say new tab/window, honor backrgound pref, and see what people think?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: fix per comment 7

Someone who browsers without a keyboard has been complaining about this in the forum, and while looking to see if middle-click would work, I found this bug.

This is just code removal (3 lines) and reindentation and a new comment.

(Shift still needs bug 359694 or bug 363895 comment 8 to work as sane people expect it, but, AFAICT, shouldLoadInBackgroundForDestination will handle checking for shift for us then as now.)

Comment 9

[Stuart Morgan]

Comment on attachment 432044 [details] [diff] [review]
fix per comment 7

Now that we don't have the if block, we can simplify a bit more. Remove these two lines:
>   EOpenDestination destination = eDestinationCurrentView;
>   BOOL loadInBackground = NO;

And change these:
>+  destination = loadInTab ? eDestinationNewTab : eDestinationNewWindow;
>+  loadInBackground = [BrowserWindowController shouldLoadInBackgroundForDestination:destination
>+                                                                            sender:nil];
to:
  EOpenDestination destination = ...
  BOOL loadInBackground = ...

sr=smorgan with those changes.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ahem.  Eventually I'll have done this "write Obj-C code" thing enough times that I'll figure out why keeping those lines bothered me, even though deleting them entirely broke the build, before I submit the patch for review. :)

Landed on cvs trunk.