Closed Bug 374076 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: Gavin, Assigned: jaas)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

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.
Duplicate of this bug: 375231
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. 
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.
Hardware: PC → All
Flags: blocking-firefox3?
Ctrl+Tab for switching (non-browser) tabs is broken in seamonkey xpfe builds as well.
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.
(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
(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?
Assignee: nobody → joshmoz
Component: Tabbed Browser → Widget: Cocoa
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: tabbed.browser → cocoa
Flags: blocking1.9?
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?
Blocks: cocoa
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9beta1
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.

Duplicate of this bug: 390430
Well, the good news is that this WFM on 10.5 9a466 :)

Bad news, still seeing it on 10.4. Adjusting the summary appropriately.
Summary: Ctrl+Tab shortcut for tab switching is broken (Mac) → [10.4] Ctrl+Tab shortcut for tab switching is broken
Blocks: 395980
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
Depends on: 398514
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.
(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.
Why is that a deal breaker? Nothing would be different from Gecko's POV.
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).
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.
Agreed; I was assuming the former.
Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #285316 - Flags: review?
Attachment #285316 - Flags: review? → review?(masayuki)
Josh:

Why don't you call keyDown simply?
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.
Attached patch fix v1.1 (obsolete) — Splinter Review
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.
Attachment #285316 - Attachment is obsolete: true
Attachment #285499 - Flags: review?(cbarrett)
Attachment #285499 - Flags: review?(cbarrett) → review?(smichaud)
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.
Attachment #285499 - Flags: review?(smichaud) → review+
Attached patch fix v1.2 (obsolete) — Splinter Review
Oops, forgot to re-diff after my last change. This includes the second fix I described for v1.1.
Attachment #285499 - Attachment is obsolete: true
Attachment #285515 - Flags: review?(smichaud)
Comment on attachment 285515 [details] [diff] [review]
fix v1.2

Looks fine to me.
Attachment #285515 - Flags: review?(smichaud) → review+
Attachment #285515 - Flags: superreview?(roc)
+    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?
Attached patch fix v1.3 (obsolete) — Splinter Review
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.
Attachment #285515 - Attachment is obsolete: true
Attachment #285611 - Flags: superreview?(roc)
Attachment #285515 - Flags: superreview?(roc)
+    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
Yes, it is still alive via strong ref (autorelease pool strong ref or explicit).
Comment on attachment 285611 [details] [diff] [review]
fix v1.3

assuming comment fixed
Attachment #285611 - Flags: superreview?(roc) → superreview+
Attached patch fix v1.4Splinter Review
landed on trunk, this is the exact patch I landed
Attachment #285611 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Hmm, so on Tiger, Ctrl+Tab will activate on key up from now on?  That's unfortunate for perceived performance.
Blocks: 371757
You need to log in before you can comment on or make changes to this bug.