374076 - [10.4] Ctrl+Tab shortcut for tab switching is broken

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Using Ctrl+Tab/Ctrl+Shift+Tab for tab switching is broken on the trunk, for Mac. Mano suspected it may be a regression from the cocoa widgets switch.

I'm pretty surprised that I would be the first person to report this, but I can't find a dupe, and neither Mano nor mconnor think this was an intentional change (in fact it was re-added for 2.0 in bug 264787), so here we are.

Comment 2

[is+mozilla]

Thanks for marking my bug as duplicate. Is it possible to switch to Hardware field from "PC" to "Mac"? I didn't think to use the combination of "OS X" OS and "PC" hardware in my searching. 

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Hardware="PC" and OS="Mac OS X" is what Bugzilla prefills for Intel Macs, by default. You should just ignore the Hardware field when looking for bugs by operating system.

Comment 4

[Stefan [:stefanh]]

Ctrl+Tab for switching (non-browser) tabs is broken in seamonkey xpfe builds as well.

Comment 5

[Tony Mechelynck [:tonymec]]

Stefan: What do you mean by "non-browser" tabs? (Sorry if I'm being dense.)

Everyone: Do Ctrl-PgUp and Ctrl-PgDn also not work on Macs? IIUC, they are supposed to also walk tabs. At least, they work on my Linux system, where the (kde) window manager intercepts Ctrl-Tab and Ctrl-Shift-Tab, both of which therefore never reach any non-pure-text app.

Comment 6

[Stefan [:stefanh]]

(In reply to comment #5)
> Stefan: What do you mean by "non-browser" tabs? (Sorry if I'm being dense.)

Tabs in, for example, Page Info

Comment 7

[Tony Mechelynck [:tonymec]]

(In reply to comment #6)
> (In reply to comment #5)
> > Stefan: What do you mean by "non-browser" tabs? (Sorry if I'm being dense.)
> 
> Tabs in, for example, Page Info
> 
Have you tried Ctrl-PgDn (or Cmd-PgDn) there? Does it work if you do?

Comment 8

[(not currently active) Ted Mielczarek]

Ok, regression window is 2006-09-28 to 2006-09-29, this points a finger at bug 326469.  Josh, gavin said you might know what the problem was?

Comment 9

[jrblier]

I confirm that for regular web pages tabs, on MacBook Pro with Gran Paradiso a6, Ctrl-PgDn and Ctrl-PgUp do work for regular web page tabs. Ctrl-tab and Not Cmd-PgDn don't work.

For Preferences and Page info, there is no way to switch tabs from the keyboard.

Comment 11

[Colin Barrett [:cbarrett]]

Well, the good news is that this WFM on 10.5 9a466 :)

Bad news, still seeing it on 10.4. Adjusting the summary appropriately.

Comment 12

[Josh Aas]

The little test app on bug 398514 makes it easy to see the problem here. On
10.4 Cocoa doesn't send an NSKeyDown event for control-tab, as that is special
for first responder control. On 10.5 we get a performKeyEquivalent: message
with an NSKeyDown event for control-tab, which is why this bug doesn't show up
on 10.5.

So, on 10.4 we just don't get an NSKeyDown control-tab event from cocoa. We
might be able to do a crazy appshell hack to find the event and post it
somehow, but I'm not sure that'll work or what it would do to embedding, and if
it did work it would be messy.

Probably the best thing to do here is we get the NSKeyUp event from cocoa for
control-tab send an NS_KEY_DOWN event into gecko before the NS_KEY_UP event
gets sent into gecko. The only consequence of that is that tabs will switch on
key up, not key down. This will be cleaner if we do it along with or after the
changes in bug 398514.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> Probably the best thing to do here is we get the NSKeyUp event from cocoa for
> control-tab send an NS_KEY_DOWN event into gecko before the NS_KEY_UP event
> gets sent into gecko. The only consequence of that is that tabs will switch on
> key up, not key down.

This is a deal breaker for bug 395980.

Comment 14

[Josh Aas]

Why is that a deal breaker? Nothing would be different from Gecko's POV.

Comment 15

[Dão Gottwald [:dao]]

Does "on key up" relate to Ctrl or to Tab?
Ctrl would mean that you could never bring that panel up, Tab would mean that you couldn't scroll quickly through the tabs (which would be tenable I think).

Comment 16

[Josh Aas]

It would make the latter impossible, scrolling through a bunch of tabs without ever bringing the key up. However, that seems like an unlikely behavior and not worth a huge effort to work around this bug in a better way, sorry.

Comment 17

[Dão Gottwald [:dao]]

Agreed; I was assuming the former.

Comment 18

[Josh Aas]

Attachment: fix v1.0

There is probably some code consolidation that can be done in our key handling, but since we're about to make more changes soon I think it is best not to prematurely optimize and stick with clarity for now. Once we know more about what our final key handling structure is going to be we can consolidate some code into shared functions.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Josh:

Why don't you call keyDown simply?

Comment 20

[Josh Aas]

keyDown: does stuff we don't need to do. And if the first responder changes in response to the key down event we don't want to just return we want to call keyUp: on the new first responder.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 285316 [details] [diff] [review]
fix v1.0

o.k.

Comment 22

[Josh Aas]

Attachment: fix v1.1

This updated patch fixes two bugs in the original. First of all if an NS_KEY_DOWN event sent from keyUp: changed focus the call to keyUp: in the new first responder would send another NS_KEY_DOWN event. Secondly, if keyUp: called keyUp: after a first responder change, the original keyUp: call would continue on to send another keyUp: event.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 285499 [details] [diff] [review]
fix v1.1

I haven't tested this.  But I can't find any logical errors ... so it
seems fine to me.

Comment 24

[Josh Aas]

Attachment: fix v1.2

Oops, forgot to re-diff after my last change. This includes the second fix I described for v1.1.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 285515 [details] [diff] [review]
fix v1.2

Looks fine to me.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    NSResponder* resp = [[self window] firstResponder];
+    if (resp != (NSResponder*)self) {

Does this not-focused path get taken if the key-down event tears down the window?

+    nsKeyEvent geckoEvent(PR_TRUE, NS_KEY_PRESS, nsnull);
+    [self convertCocoaKeyEvent:nativeKeyDownEvent toGeckoEvent:&geckoEvent];

What if this event tears down the window?

+    if ((!geckoEvent.isChar || geckoEvent.isControl) &&
+        !nsTSMManager::IsComposing()) {

How can isControl be false here, or geckoEvent.isChar be true here?

Comment 27

[Josh Aas]

Attachment: fix v1.3

Add additional safety checks for window/view destruction and don't do the isChar/isControl checks when we already know which value they must have.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (!mGeckoChild)
+      return;

How do we know that 'this' is still alive? Is someone holding a strong ref to it?

+    // if this is a non-letter keypress, or the control key is down,
+    // dispatch the keydown to gecko, so that we trap delete,
+    // control-letter combinations etc before Cocoa tries to use
+    // them for keybindings.

Fix comment

Comment 29

[Josh Aas]

Yes, it is still alive via strong ref (autorelease pool strong ref or explicit).

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 285611 [details] [diff] [review]
fix v1.3

assuming comment fixed

Comment 31

[Josh Aas]

Attachment: fix v1.4

landed on trunk, this is the exact patch I landed

Comment 32

[Stephen Donner [:stephend] Not actively reading bugmail]

I've verified that:

* ctrl + shift + tab cycles backwards from the currently selected tab
* ctrl + tab cycles forward from the currently selected tab

works as expected in both:

PPC: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre

Intel: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre

Verified FIXED

Comment 33

[Jesse Ruderman]

Hmm, so on Tiger, Ctrl+Tab will activate on key up from now on?  That's unfortunate for perceived performance.