628918 - Make CTRL-F4 work in tabbmail (but not in OSX) and better keyboard tab navigation on Mac

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Philip Chee]

References:
  SM [Bug 521803] tabbrowser.xml: "Error: this.mPanelContainer is null", opening MailNews
  TB [Bug 511375] Add Ctrl-F4 keyboard shortcut to close tab.

  FX [Bug 264145] disable Control+F4 (Close Tab keyboard shortcut) on OS X.
  FX [Bug 299706] When a tab is focused, cmd+right/left-arrow drags it and goes back/forward in history.
  FX [Bug 332271] One cause of wrong tab closure identified in tab closure handling (Ctrl+F4 bubbles although being handled)
  FX [Bug 417176] better keyboard shortcuts for switching tabs on mac os x.
  FX [Bug 429217] [Mac]Regression: Cmd+{ and Cmd+} will not switch tabs.
  FX [Bug 102060] enable New Tab in the UI and implement tabbrowser context menus for closing and creating tabs, add CTRL+f4 for closing tabs. r=bryner, sr=brendan

Comment 1

[Philip Chee]

Attachment: Patch v1.0 CTRL-F4

Stefanh: Please check if the keys really work on a Mac (tabbrowser and tabmail).
Mnyromyr: Had to hijack your patch because mine needs it and yours appears to have stalled forever.

Bug 102060 Comment 2:

> Brendan Eich      2001-09-27 17:02:51 PDT
> 
> along with the ctrl-F4 patch hyatt showed me in person.

ctrl-F4 wasn't in the approved patch!

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpfe/global/resources/content/bindings&command=DIFF_FRAMESET&file=tabbrowser.xml&rev2=1.2&rev1=1.1

But was in the actual checkin!

> -          if (aEvent.ctrlKey && aEvent.keyCode == KeyEvent.DOM_VK_F4 &&
> -              this.tabbrowser.mTabBox.handleCtrlPageUpDown &&

[Bug 133503] [REPRO]Ctrl+F4 active during print preview
> "This patch hooks into the handleCtrlPageUpDown attribute set by browser.js"

Nothing sets handleCtrlPageUpDown these days in comm-central. As far as I can tell this was used by an older implementation of Print Preview but seems to have disappeared after some rewrite or other.

> +              this.tabbrowser.removeCurrentTab();
> +              aEvent.stopPropagation();
> +              aEvent.preventDefault();

[Bug 332271] One cause of wrong tab closure identified in tab closure handling (Ctrl+F4 bubbles although being handled)
This bug introduced stopPropagation/preventDefault

[Bug 417176] better keyboard shortcuts for switching tabs on mac os x.
[Bug 429217] [Mac]Regression: Cmd+{ and Cmd+} will not switch tabs.

> ⌘+{ and ⌘+} on US keyboards, standard keyboard shortcuts for switching tabs in
> OS X stopped working between build 20080414_2047 and
> 20080414_2130, regression of Bug 417176 possibly caused by Bug 359638.

In Bug 511375 Thunderbird added VK_F4 to close tabs in tabmail but asuth didn't like it living in tabbrowser and wanted it to use the standard XUL command/key so Jim (Jim who?) added it to mailWindowOverlay.xul instead.

Bug 332271 Comment 11:
> Mike Connor [:mconnor]      2006-04-03 09:29:32 PDT

> If we had this as a <key> in browser, anyone else using <tabbrowser/> would
> have to reimplement it.

In our case we have two considerations:
1. Our ultimate goal is to merge our tabbrowser and tabmail implementations so it makes sense to keep the code in both as similar as possible.
2. Keeping our tabbrowser close to the Firefox tabbrowser makes it easier to port patches over.

Comment 2

[Philip Chee]

Comment on attachment 507053 [details] [diff] [review]
Patch v1.0 CTRL-F4

Oops.
> Stefanh: Please check if the keys really work on a Mac (tabbrowser and tabmail).

Comment 3

[[e-mail]]

(In reply to comment #1)
> > ⌘+{ and ⌘+} on US keyboards, standard keyboard shortcuts for switching tabs in
> > OS X stopped working between build 20080414_2047 and
> > 20080414_2130, regression of Bug 417176 possibly caused by Bug 359638.
Do they mean browser tabs or all tabs?

Comment 4

[Stefan [:stefanh]]

(In reply to comment #2)
> > Stefanh: Please check if the keys really work on a Mac (tabbrowser and tabmail).

You mean that Ctrl+F4 shouldn't work, right? Or do you mean that I should check before the patch is applied?

Comment 5

[Philip Chee]

> Do they mean browser tabs or all tabs?
I think the mean Safari tabs:
<http://en.wikipedia.org/wiki/Table_of_keyboard_shortcuts#Tab_management>

Comment 6

[Philip Chee]

>> Stefanh: Please check if the keys really work on a Mac (tabbrowser and tabmail).

> You mean that Ctrl+F4 shouldn't work, right? Or do you mean that I should check
> before the patch is applied?
CTRL-F4 shouldn't work after the patch. Does it work before the patch?
Also check the tab navigation keys: ⌘+{ / ⌘+}

Comment 7

[Stefan [:stefanh]]

Comment on attachment 507053 [details] [diff] [review]
Patch v1.0 CTRL-F4

+                if (window.getComputedStyle(this, null).direction == "ltr")
Hmm, this.tabbrowser I presume?

(with the above changed)

Ctrl+F4 sort of worked (in browser) before the patch - the OS uses that combo, so unless you turn off the OS mapping it won't work. After the patch it doesn't work at all (good).

The new mac tab nav key combo works fine when using en-US kb layout - thanks! It doesn't work when using sv-SE and I don't expect it to work using de, to get the right key combo on those layouts you'll need to press Cmd+alt+shift+8 (or 9) and iirc that doesn't send the right charcodes.

Ctrl+tab doesn't work anymore in mailNews after the patch (that's sort of bad). I have some vague hope that we in the future could move to ctrl-tab in browser as well (for mac). Then it would be nice if Ctrl-tab still worked in MailNews.

Does Ctrl+tab still works on windows after your patch?

Comment 8

[Philip Chee]

Attachment: Patch v1.1 Mac should handleCtrlTab like Firefox.

> Stefan (stefanh)      2011-01-26 14:48:35 PST
> 
> Comment on attachment 507053 [details] [diff] [review]
> Patch v1.0 CTRL-F4
> 
> +                if (window.getComputedStyle(this, null).direction == "ltr")
> Hmm, this.tabbrowser I presume?
Oops. Yes.

> (with the above changed)
> 
> Ctrl+F4 sort of worked (in browser) before the patch - the OS uses that combo,
> so unless you turn off the OS mapping it won't work. After the patch it doesn't
> work at all (good).
Right.

> The new mac tab nav key combo works fine when using en-US kb layout - thanks!
Good.

> It doesn't work when using sv-SE and I don't expect it to work using de, to get
> the right key combo on those layouts you'll need to press Cmd+alt+shift+8 (or
> 9) and iirc that doesn't send the right charcodes.
Well better than nothing.

> Ctrl+tab doesn't work anymore in mailNews after the patch (that's sort of bad).
> I have some vague hope that we in the future could move to ctrl-tab in browser
> as well (for mac). Then it would be nice if Ctrl-tab still worked in MailNews.
> 
> Does Ctrl+tab still works on windows after your patch?

References:
http://developer.apple.com/library/mac/#documentation/userexperience/conceptual/applehiguidelines/XHIGKeyboardShortcuts/XHIGKeyboardShortcuts.html
Bug 169589 - [mac] ctrl-tab no longer moves btwn frames.
Bug 264787 - [Mac] Ctrl+Tab and Ctrl+Shift+Tab Next/Previous Tab Keyboard Shortcuts no longer work (worked in Firefox 1.0.x).

Here is what happens. tabmail inherits from tabbrowser. But the tabbrowser constructor falls over because it doesn't expect to be in tabmail. Because the constructor fails to run the following line does not get executed:

>            this.mTabBox.handleCtrlTab = !/Mac/.test(navigator.platform);

So Ctrl+tab in OSX only works in tabmail by accident. Since this bug fixes the constructor tabmail now follows the tabbrowser. See:

>        <constructor>
>          <![CDATA[
> -          this.mCurrentBrowser = this.mPanelContainer.firstChild.firstChild;
> -          this.mCurrentTab = this.tabContainer.firstChild;
>            document.addEventListener("keypress", this._keyEventHandler, false);
>            this.mTabBox.handleCtrlTab = !/Mac/.test(navigator.platform);
>            this.arrowKeysShouldWrap = /Mac/.test(navigator.platform);
> +          // Bail out early if we are in tabmail. See Bug 521803.
> +          if (!this.mPanelContainer)
> +            return;

This is Bug 169589 (blame says Neil). However in Firefox Bug 264787 the decision was reversed. See Bug 264787 Comment 24 to Bug 264787 Comment 31 for the reason why Firefox switched back.

Comment 9

[[e-mail]]

(In reply to comment #7)
> It doesn't work when using sv-SE and I don't expect it to work using de, to get
> the right key combo on those layouts you'll need to press Cmd+alt+shift+8 (or
> 9) and iirc that doesn't send the right charcodes.
Well, that and the alt key filtering...

Comment 10

[Stefan [:stefanh]]

(In reply to comment #9)
> (In reply to comment #7)
> > It doesn't work when using sv-SE and I don't expect it to work using de, to get
> > the right key combo on those layouts you'll need to press Cmd+alt+shift+8 (or
> > 9) and iirc that doesn't send the right charcodes.
> Well, that and the alt key filtering...

Yeah, 2 reasons for not working. otoh, But I don't think we mind here - imagine pressing 4 keys to move between tabs ;-)

Comment 11

[[e-mail]]

Comment on attachment 507391 [details] [diff] [review]
Patch v1.1 Mac should handleCtrlTab like Firefox.

Pending resolution of the Cmd+Alt+Shift+8 issue.

Comment 12

[Philip Chee]

> Pending resolution of the Cmd+Alt+Shift+8 issue.

Based on bug 264787 comment 28:

> Jesse Ruderman      2006-02-03 16:09:06 PST
> 
> It's really lame that there's no way to switch tabs with one hand on a
> Powerbook.  (Neither Cmd+Opt+Arrow nor Ctrl+PgDn can be used with one hand on a
> Powerbook keyboard.)
> 
> Switching tabs is much more common than keyboard-navigating between frames. 
> Ctrl+Tab was originally kept for switching frames on Windows, but it was
> changed to switching tabs when we realized this (see bug 136915 comment 59). 
> What makes Mac different?

And bug 264787 comment 31:

> Mike Connor [:mconnor]      2006-02-04 20:02:53 PST
> 
> Comment on attachment 169124 [details] [diff] [review]
> Restore keybinidng
> 
> wow, I don't remember being ok with wontfixing this, but I was Mac-deficient in
> the old days, so I might not have paid attention.  I guess I just don't browse
> one-handed on my powerbook that much...

It appears that ctrl-tab is the preferred keyboard shortcut on a Mac. So it's likely that users with non-US keyboards won't bother with Cmd+Alt+Shift+8 even if available. Stefan?

Comment 13

[Stefan [:stefanh]]

(In reply to comment #12)
> appears that ctrl-tab is the preferred keyboard shortcut on a Mac. So it's
> likely that users with non-US keyboards won't bother with Cmd+Alt+Shift+8 even
> if available. Stefan?

That's what I ment in comment #10. Who would care about pressing 4 keys at the same time in order to move between tabs?

Comment 14

[Stefan [:stefanh]]

Note btw that (iirc) the mac key combo for moving between frames in a webpage is Ctrl+tab, so we need to change that.

Comment 15

[Philip Chee]

> Note btw that (iirc) the mac key combo for moving between frames in a webpage
> is Ctrl+tab, so we need to change that.

What does Firefox do in this case?

Comment 16

[Stefan [:stefanh]]

(In reply to comment #15)
> > Note btw that (iirc) the mac key combo for moving between frames in a webpage
> > is Ctrl+tab, so we need to change that.
> 
> What does Firefox do in this case?

I can't find the code atm. The doc's indicate that FF use F6, our docs indicate that we use F6 for win/nix and Ctrl+tab for mac (and also F6, if available). Neil, any idea?

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 507391 [details] [diff] [review]
Patch v1.1 Mac should handleCtrlTab like Firefox.

>+          document.removeEventListener("keypress", this._keyEventHandler, false);
>+          // Bail out early if we are in tabmail. See Bug 521803.
>+          if (!this.mPanelContainer)
>+            return;
>+

Nit: Add a blank line before the comment too. At both places.

Comment 18

[Stefan [:stefanh]]

(In reply to comment #16) 
> I can't find the code atm. The doc's indicate that FF use F6, our docs indicate
> that we use F6 for win/nix and Ctrl+tab for mac (and also F6, if available).
> Neil, any idea?

Nm, found it (nsEventStateManager.cpp). So, when we switch to Ctrl+tab, we will still have F6 to move between frames. This key will also work for mac and it's in line with what Firefox does. It's not optional, since these F keys are in general reserved for the OS. F6 is still not taken, though - so I believe we're fine here.

Comment 19

[Stefan [:stefanh]]

Comment on attachment 507391 [details] [diff] [review]
Patch v1.1 Mac should handleCtrlTab like Firefox.

I tested this in both browser and mailNews - all key combos works fine on mac.

Comment 20

[Philip Chee]

> It's not optional, since these F keys are in
> general reserved for the OS. F6 is still not taken, though - so I believe we're
> fine here.

http://rationalpi.wordpress.com/2007/01/31/using-f-keys-in-mac-os-x/

Comment 21

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/f15efb863e07

Comment 22

[Stefan [:stefanh]]

(In reply to comment #20)
> http://rationalpi.wordpress.com/2007/01/31/using-f-keys-in-mac-os-x/

Yes, but it's not a solution - why would you want to turn off the OS shortcuts just to make a shortcut work in one application?

Comment 23

[Karsten Düsterloh]

Comment on attachment 507391 [details] [diff] [review]
Patch v1.1 Mac should handleCtrlTab like Firefox.

Removing obsolete feedback? flag.

(In reply to comment #22)
> (In reply to comment #20)
> > http://rationalpi.wordpress.com/2007/01/31/using-f-keys-in-mac-os-x/
> 
> Yes, but it's not a solution - why would you want to turn off the OS shortcuts
> just to make a shortcut work in one application?

Just because of that - the OS shouldn't block rare resources like shortcuts for rather minor functionality! 

And as you might have guessed, I've made the pref setting mentioned in that article years ago... ;-)

Comment 24

[Philip Chee]

> Removing obsolete feedback? flag.
Well I didn't want you to feel left out.

Comment 25

[Stefan [:stefanh]]

(In reply to comment #23)
> Just because of that - the OS shouldn't block rare resources like shortcuts for
> rather minor functionality! 

Well, the question what the OS should and shouldn't do isn't really in scope here. What we can conclude is that the OS reserves certain keys and we shouldn't use those keys ;-)

Comment 26

[Karsten Düsterloh]

We should not *require* (or expect) those keys to work, i.e. such a key shouldn't be the only way to reach often-used functionality. We can (and IMO should) still provide enhanced keyboard access for those who want it - after all, the OS pref setting is rather prominent and not exactly hidden...

But you're right, this (closed) bug is not the right place to discuss it.