186591 - View Image in New Tab/Window (use Tabbed Browsing preference semantics)

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Thomas F. O'Connell]

rather than re-using the existing window, View Image should follow whatever
semantics are set in the Tabbed Browsing preference.

or, the function should have its own preference specifying an appropriate
behavior for it. View Image is a great new feature, but it's frustrating to have
it overwrite the page on which the image appeared.

Comment 1

[Mike Pinkerton (not reading bugmail)]

smfr? thoughts?

Comment 2

[Stephane Moureau]

There is no Tabbed Browsing options which View Image can use.
The closest one is "Loading a page requested by another application:".
Either by adding a similar group for View Image or by extending the current one:
"Loading a page requested by another application or by View Image:"
I prefer the last one, extending.

Another solution to have an almost uniform behavior like for Links in the
Contextual menu is:
1.View Image
2.View Image in a New Window
3.View Image in a New Tab

1. would be the equivalent to Cmd-Clicking a link as you can not Cmd-Click a
simple picture because it's in a Contextual menu and that some image might
contains a link.

Comment 3

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

*** Bug 247110 has been marked as a duplicate of this bug. ***

Comment 4

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

Based on the duplicate I just marked, and Stephane's comment 2 which is what I'm
guessing is the approved implementation, I'm going to slightly modify the summary.

Was: View Image should use Tabbed Browsing preference semantics
Now: View Image in New Tab/Window (use Tabbed Browsing preference semantics)

Comment 5

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

Instead of polluting the context menu or prefs with another option, we should use the Cmd shortcut like we do everywhere else for "open in new".  Anything other/more than that seems like a WONTFIX.

Comment 6

[Chris Lawson (gone)]

What Smokey said.

I can probably take a look at this sometime soon-ish.

cl

Comment 7

[Chris Lawson (gone)]

Attachment: kicks to a new window with cmd, toggles bg/fg with shift

If cmd is held down, we kick the image view to a new tab/window, respecting loadInBG preference.

If shift is ALSO held down, we toggle the loadInBG preference when opening the new tab/window.

If shift is held down by itself, we do nothing, since toggling that pref has no meaning for the frontmost tab/window.

cl

Comment 8

[Mark Mentovai]

Comment on attachment 216545 [details] [diff] [review]
kicks to a new window with cmd, toggles bg/fg with shift

Note the problems you had during testing with your crummy third-party mouse software: events for button 2 were all coming through as though control was held down and no other modifiers were down, regardless of the actual state of the modifiers.

>+  BOOL loadInTab = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
>+  BOOL loadInBG  = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];

You can move these down into the NSCommandKeyMask block.

>+    if ([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask) {
>+      if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) {

Don't do [[NSApp currentEvent] modifierFlags] twice, get it once and store the result in a variable.

Add a comment about shift flipping the default behavior.

The extra logic for the shift-flip looks like bloat.  How about this:

BOOL shiftIsDown = modifiers & NSShiftKeyMask;
if (loadInTab) {
  [self openNewTabWithURL:urlStr
                 referrer:referrer
         loadInBackground:(shiftIsDown ? !loadInBG : loadInBG) // shift key toggles
              allowPopups:NO];
}
else {
  [self openNewWindowWithURL:urlStr
                    referrer:referrer
            loadInBackground:(shiftIsDown ? !loadInBG : loadInBG) // shift key toggles
                 allowPopups:NO];
}

Comment 9

[Mark Mentovai]

Or, even better:

if (modifiers & NSShiftKeyMask) 
  loadInBG = !loadInBG;

then just use loadInBG normally without testing for the shift in openNewWhatever.

Comment 10

[Chris Lawson (gone)]

Attachment: addresses mento's comments

For the record:

Kensington's MouseWorks software eats the modifierFlags on NSEvents when a key combination (like control-click) is assigned to a specific mouse button. A bug has been filed with Kensington's tech support on this problem. They're in beta testing for version 3.0, so there's an outside chance this could get fixed by the time it goes final.

Other third-party mouse drivers may or may not have the same problem, but that should probably be our first question if anyone complains this doesn't work.

cl

Comment 11

[Mark Mentovai]

Comment on attachment 216547 [details] [diff] [review]
addresses mento's comments

>+    if (modifiers & NSCommandKeyMask) {
>+      BOOL loadInTab = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
>+      BOOL loadInBG  = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
>+      if (modifiers & NSShiftKeyMask) {
>+        loadInBG = !loadInBG; // shift key should toggle the foreground/background pref as it does elsewhere

This should be the ONLY thing in the enclosing conditional, because this:

>+        if (loadInTab)
>+          [self openNewTabWithURL:urlStr referrer:referrer loadInBackground:loadInBG allowPopups:NO];
>+        else
>+          [self openNewWindowWithURL:urlStr referrer:referrer loadInBackground:loadInBG allowPopups:NO];
>+      }

and this:

>+      else {
>+        if (loadInTab)
>+          [self openNewTabWithURL:urlStr referrer:referrer loadInBackground:loadInBG allowPopups:NO];
>+        else
>+          [self openNewWindowWithURL:urlStr referrer:referrer loadInBackground:loadInBG allowPopups:NO];
>+      }
>+    }

are identical.

Comment 12

[Chris Lawson (gone)]

Attachment: third time's a charm?

Heh. Oops. Guess I shoulda seen that, huh? This one's fixed.

cl

Comment 13

[Mark Mentovai]

Comment on attachment 216590 [details] [diff] [review]
third time's a charm?

OK, good.  I don't "View Image" often, but I agree that it should respect the pref.

>+    unsigned int modifiers = [[NSApp currentEvent] modifierFlags];
>+        
>+    if (modifiers & NSCommandKeyMask) {

Checkin person, please check the blank line above in without the eight (why eight?) spaces.

>+        loadInBG = !loadInBG; // shift key should toggle the foreground/background pref as it does elsewhere

And change the comment to read "shift key toggles the foreground/background pref".

Setting r? target to where you seem to have wanted it.

Comment 14

[Stuart Morgan]

Comment on attachment 216590 [details] [diff] [review]
third time's a charm?

r=me, although it seems odd that I have to be holding the key(s) down when I bring up the context menu, not just when I select the menu item.  If I'm not the only one seeing that, perhaps a follow-up bug is in order.

Comment 15

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

Yeah, I confirm Stuart's issue.  We don't make you do that in normal menus (i.e., I can pull down the menu w/o any modifiers and then hold down the modifier right before selecting the item), so we shouldn't make you do that here (nor, for that matter, in any of the CM cases where it makes sense to support the tabbed browsing sematics) in order to be consistent.

Since this is the first CM item we're making obey the semantics, I'd prefer it was fixed now (rather than filed, perhaps another CM item fixed ad interim, and then have to go back and fix multiple cases), but I don't know how hairy that code/coding might be.

Comment 16

[Chris Lawson (gone)]

(In reply to comment #14)
> (From update of attachment 216590 [details] [diff] [review] [edit])
> r=me, although it seems odd that I have to be holding the key(s) down when I
> bring up the context menu, not just when I select the menu item.  If I'm not
> the only one seeing that, perhaps a follow-up bug is in order.

I see that too. No idea why...

cl

Comment 17

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

The reason for the odd behavior Stuart saw in comment 14 may well be that Chris's patch was already using the [NSApp currentEvent] stuff that was later added Camino-wide (and which caused bug 333765).

If that's the case, and since we're now broken a la comment 14 everywhere, maybe we should go ahead and land this now to prevent bitrot (if it's not already bitrotted) with the understanding that the fix for bug 333765 will fix this, too, as part of fixing the comment 14-type issues "everywhere"?

Comment 18

[Håkan Waara]

Checked in with a screwed up checkin comment (thanks cross-commit!)

Comment 19

[Chris Lawson (gone)]

Verified in latest trunk nightly.