429217 - [Mac]Regression: Cmd+{ and Cmd+} will not switch tabs

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Louise]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041421 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041421 Minefield/3.0pre ID:2008041421

⌘+{ 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.




Reproducible: Always

Steps to Reproduce:
1.Open more than one tab in single window.
2.Hit Cmd+{ or Cmd+}
Actual Results:  
Mac gives a beep

Expected Results:  
Firefox should switch between tabs.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

We didn't block on bug 417176, so I don't think we'd block on this bug, either, as much as it pains me.

Bug 359638 broke a lot of stuff, though, so I expect that the regression will be addressed (I've put a reference to it on that bug, too)

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #0)
> 2.Hit Cmd+{ or Cmd+}

Is the Shift key down?

Comment 3

[philippe (part-time)]

(In reply to comment #2)
> (In reply to comment #0)
> > 2.Hit Cmd+{ or Cmd+}
> 
> Is the Shift key down?
> 

In my case, yes (Standard Apple JIS aluminium keyboard).

Comment 4

[jacob berkman]

Attachment: handle new charCode values

Tweaking the extension I used in bug 417176, based on the info in bug 359638, led me to this patch.

If the latter is sticking around, could someone r? this patch appropriately?

Comment 5

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

Bug 359638 is a blocker, so we shouldn't need that patch.

Comment 6

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

No that should not be able to fix this bug. Because the charCode is '[' or ']', not '{' or '}'. We need to create new <key/>.

Comment 7

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

Sorry, bug 359638's state confused me. What do you mean by "add a new key"? Do you mean that the behavior change was expected, and that we should take Jacob's patch?

Comment 8

[Louise]

(In reply to comment #2)
> (In reply to comment #0)
> > 2.Hit Cmd+{ or Cmd+}
> 
> Is the Shift key down?
> 

yes, in MacBook US keyboard(Cmd+Shift+[).  I don't know how the shortcuts are dealed in other languages' keyboard layout.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #7)
> Sorry, bug 359638's state confused me. What do you mean by "add a new key"?

I think Masayuki is suggesting that this be implemented using a XUL <key>
element because the XBL key event handler has special code to use extra
information in the native key event which is not available to javascript.
I agree that a <key> is the best way to do this but...

(In reply to comment #6)
> No that should not be able to fix this bug. Because the charCode is
> '[' or ']', not '{' or '}'.

The behavior here has changed and I fear this behavior change could affect
other javascript code.  This new behavior is also inconsistent with
other platforms IIUC.

Masayuki, I know we agreed that we should send the character code from the
event, but this particular case (Cmd+Shift) seems to be a deficiency of the
"characters" field of the NSEvent, which is provided for through the
charactersIgnoringModifiers field.

It seems that when Cmd+Shift are down, "characters" is always the same as when
on Cmd is down.  So this field seems to be of little use in the Cmd+Shift
situation.

I would recommend setting charCode from charactersIgnoringModifiers whenever
Cmd+Shift are down and shiftedCmdChar == unshiftedChar.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> It seems that when Cmd+Shift are down, "characters" is always the same as when
> on Cmd is down.

I mean _only_ Cmd is down.

Comment 11

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

The fact that bug 359638 made us see different charCodes for these events scares me a lot from a compatibility point of view - it looks like a number of bugs have been caused by the change, and who knows how much out-of-tree code is depending on the old behavior. Was there no lower-impact way of fixing bug 359638?

Comment 12

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

> Was there no lower-impact way of fixing bug 359638?

If I know that, I did it.

Comment 13

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

(In reply to comment #12)
> > Was there no lower-impact way of fixing bug 359638?
> 
> If I know that, I did it.

On Windows and Linux, when Ctrl key is pressed, the may send the ASCII control characters. Then, we can replace the charCode to ASCII normal characters like Cocoa.

widget/src/cocoa/nsChildView:
> 3890     // convert control-modified charCode to raw charCode (with appropriate case)
> 3891     if (outGeckoEvent->isControl && outGeckoEvent->charCode <= 26)
> 3892       outGeckoEvent->charCode += (outGeckoEvent->isShift) ? ('A' - 1) : ('a' - 1);

Otherwise, we should keep the current behavior. But this issue is really another bug.

Comment 14

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

karlt:

I filed bug 429510. I will replace the charCode at most cases like previous behavior.

Comment 15

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

fixed by bug 432389.

-> FIXED

Comment 16

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7715 to Litmus in both the FFT and the Mac keyboard specific test run.