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

VERIFIED FIXED in mozilla1.9beta1

Status

()

defect
--
major
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Gavin, Assigned: jaas)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla1.9beta1
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

Comment 2

12 years ago
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?

Comment 4

12 years ago
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.

Comment 6

12 years ago
(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
(Assignee)

Updated

12 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Updated

12 years ago
Target Milestone: --- → mozilla1.9beta1

Comment 9

12 years ago
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
(Assignee)

Updated

12 years ago
Depends on: 398514
(Assignee)

Comment 12

12 years ago
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.
(Assignee)

Comment 14

12 years ago
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).
(Assignee)

Comment 16

12 years ago
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.
(Assignee)

Comment 18

12 years ago
Posted 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?
(Assignee)

Updated

12 years ago
Attachment #285316 - Flags: review? → review?(masayuki)
Josh:

Why don't you call keyDown simply?
(Assignee)

Comment 20

12 years ago
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.
(Assignee)

Comment 22

12 years ago
Posted 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)
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 24

12 years ago
Posted 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+
(Assignee)

Updated

12 years ago
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?
(Assignee)

Comment 27

12 years ago
Posted 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
(Assignee)

Comment 29

12 years ago
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+
(Assignee)

Comment 31

12 years ago
Posted patch fix v1.4Splinter Review
landed on trunk, this is the exact patch I landed
Attachment #285611 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 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

Comment 33

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

Updated

12 years ago
Blocks: 371757
You need to log in before you can comment on or make changes to this bug.