User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_6) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.3 Safari/534.24 Build Identifier: Chrome has it and it's useful. Reproducible: Always Steps to Reproduce: 1. Right click on an image. Actual Results: The "Open Image In New Tab" entry is not there. Expected Results: The "Open Image In New Tab" entry is there.

Middle-click on the "View Image" menu item opens it in a new tab.

This patch will make the view image open in a new tab by default.

I don't think we want to change this behavior, at least not without a much better justification.

I actually thought we changed this a while ago, as "View Image" doesn't make it clear that it's going to replace the current page.

Loading the image in the current tab doesn't cause "dataloss" in any meaningful sense of the word, unless you're referring to the edge case of un-cached responses to POST submissions. This bug was originally about adding a menu item, so morphing it to cover a change in behavior of the existing menu item doesn't seem ideal.

Comment on attachment 577315 [details] [diff] [review] patch-v1 >+ openUILinkIn(viewURL, "tab", null, null, doc.documentURIObject ); This will open a background tab with browser.tabs.loadBookmarksInBackground set to false. I think it should always be a foreground tab. The following (untested) would probably do it: openUILinkIn(viewURL, "tab", { referrerURI: doc.documentURIObject, inBackground: false }); (We should probably file a bug on removing the browser.tabs.loadBookmarksInBackground use from openLinkIn and using it only in code that actually opens bookmarks.)

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5) > Loading the image in the current tab doesn't cause "dataloss" in any > meaningful sense of the word, unless you're referring to the edge case of > un-cached responses to POST submissions. We also don't restore plugin and web app states (e.g. zimbra). I'm only mentioning this for completeness. What I think is more relevant is that replacing the page seems inconvenient; my experience is that I usually go back manually after using View Image. > This bug was originally about adding a menu item, so morphing it to cover a > change in behavior of the existing menu item doesn't seem ideal. I'm pretty sure we don't want to fix this bug as-filed, as this would clutter up the context menu. I think we just need some sensible default behavior here.

Valid points for keeping and removing current behavior. Can I make this a pref?

See also bug 696747 comment 3 and 4.

<nigelb> dao: Hey, about the View Image bug, I'm not so sure any more, especially after discovering it respects Ctrl + click and Shift + click. <dao> nigelb: after discovering, right... it's unusual for context menus and not really discoverable <nigelb> Good point. <nigelb> I "discovered" that after looking at the code again + dolske's comment.

It looks to me like my context menus have a lot of items that aren't as context-sensitive as they could be - in particular, many items appear regardless of where the click is, so the menu is longer on a link or image than on a blank part of the page. Better segregation might make it less of a problem to have similar items like "open image in this tab" and "open image in new tab" There could also be a submenu at the top named something like "menu items open new tabs", with submenu items to change that to this tab or new window. That's basically adding a preference, and giving every menu an item which indicates the state of that preference and allows you to change it. Since the menus persist after the button is released, there could be another context menu for the context menu items, but that might be even weirder than modifier keys on context menu items. Since modifier keys are supported, the menu item text should change when the modifiers are pressed or released. The simplest thing might be to add to the menu item some indication that modifier keys are supported - maybe even a tooltip?

Comment on attachment 577329 [details] [diff] [review] patch-v2 inBackground: false isn't needed as of bug 707672