427433 - Command-T should always open a new tab

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Chris Lawson (gone)]

Gruber recently blogged about some shortcomings of Firefox 3, and Beltzner followed that up with a post of his own. One thing that jumped out at me from both of them, which also has p-safari implications, is that Command-T doesn't do anything unless a browser window is frontmost. (See also bug 427421, the Firefox version.)

Safari's behaviour when the Downloads or Preferences window is active -- which I prefer -- is to bring the frontmost browser window forward and make a new tab in it. That part I like.

What I think is silly is that Safari puts a new tab on a browser window (that is frontmost, but still in the background) when the Activity window is frontmost. To my mind, it would make more sense to bring the browser window to the front each time.

I think we should at least consider doing this (rather than beeping as we do now), especially since it's something that Safari users will expect to Just Work™.

Comment 1

[Christopher Henderson]

Taking. I have a semi-working patch. Just need to allow a new tab if the prefs window is frontmost, and clean up the logic in MainController's newTab: method.

Comment 2

[Christopher Henderson]

Attachment: Fix v1.0

Adds checks for the Downloads or Prefs windows being frontmost. It may be more efficient to retain the downloads and prefs controllers when MainController is inited.

Comment 3

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

Does this do the right thing if a view-source (or other chromeless) window is frontmost?

Comment 4

[Christopher Henderson]

Attachment: Fix v1.1

Thanks for pointing that out.

Rethinking the logic allows me to greatly simplify the new tab feature. The New Tab menu item is always enabled. If there is a full-chrome window, it will open a new tab in it (even if it is not the frontmost window). If not, a new window will be created.

Comment 5

[philippe (part-time)]

Any reason this is still UNCO ?

That patch works nicely, btw (10.5).

Comment 6

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

> Any reason this is still UNCO ?

Because we have to decide if we want this behavior.

Comment 7

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

[12:43pm] pinkerton: seems reasonable to me
[12:43pm] pinkerton: cmd-n always works
[12:43pm] pinkerton: seems like cmd-t should always work as well

Comment 8

[Mike Pinkerton (not reading bugmail)]

yes, we want this behavior :D

Comment 9

[Sean Murphy]

Comment on attachment 338487 [details] [diff] [review]
Fix v1.1

>   // only enable newTab if there is a browser window frontmost, or if there is no window
>   // (i.e., disable it for non-browser windows and popups).
>-  if (action == @selector(newTab:))
>-    return (((browserController && ([NSApp mainWindow] == [self frontmostBrowserWindow]))) ||
>-            ![NSApp mainWindow]);
>+  if (action == @selector(newTab:)) {
>+    return YES;
>+  }

The comment above will need to be updated here.  While I think it's best to be explicit, we could also just remove the check entirely, as the default return is to enable the menu item in question.

Either way, nice fix! r=murph with the above addressed.

Comment 10

[Christopher Henderson]

Attachment: Fix v1.2

Same as Fix v1.1, but with the modified comment.

Comment 11

[Christopher Henderson]

One place this doesn't enable New Tab in is when Tabspose is active in the frontmost browser window. I'm not sure whether changing the behaviour is a good idea, as we couldn't add a new tab to that window which is what the user would expect.

Comment 12

[Stuart Morgan]

Comment on attachment 340087 [details] [diff] [review]
Fix v1.2

>+  if ([self frontmostBrowserWindow]) {
>+      [[[self frontmostBrowserWindow] delegate] newTab:aSender];
>+      [[self frontmostBrowserWindow] makeKeyAndOrderFront:aSender];

frontmostBrowserWindow does a non-trivial amount of work; don't call it three times to get the same value every time.

>+  // enable newTab everywhere
>+  if (action == @selector(newTab:)) {
>+    return YES;
>+  }

This is the assumed behavior, so there's no need for this block at all.

Comment 13

[Christopher Henderson]

Attachment: Fix v1.3

Puts the frontmost browser window into a variable, instead of calling the method three times. Removes the newTab validation block.

Comment 14

[Stuart Morgan]

Comment on attachment 340294 [details] [diff] [review]
Fix v1.3

sr=smorgan

Comment 15

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

Landed on cvs trunk.

I filed bug 457041 on the pre-existing regression I discovered while doing pre-commit checks for this checkin.