User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050831 Firefox/1.0+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050831 Firefox/1.0+ On Mac, Go -> Back "Command+[" and Go -> Forward "Command+]" keyboard shortcuts do not work at certain circumstances on Finnish keyboard. Reproducible: Always Steps to Reproduce: 1. Launch Firefox/Deer Park 2. Go to some page so that there's some page to go back 3. Press "Command+[" and/or "Command+]" Actual Results: Nothing happens, it won't go back or forward. NOTE! I'm able to get this working by first clicking Go -> Back with my mouse, then the keyboard shortcut starts working magically. Same with Forward. Expected Results: Back and Forward shortcuts should work as you know how. Note that on Swedish/Finnish keyboard "[" is typed Option+8, so you have to press Command+Option+8 for Back shortcut. Not sure how this is done with English keyboard. Maybe totally different.

Is this a problem on branch? What's the regression window? The most likely culprits are bug 249136 and bug 258285?

(In reply to comment #1) > Is this a problem on branch? What's the regression window? > > The most likely culprits are bug 249136 and bug 258285? The standard mac shortcuts for forward/back is "cmd+]"/"cmd+[" on en-US. Since ] is alt+9 and [ is alt+8 on a Swedish/Finnish keyboard this turns out to be a problem: 1) There's no way to map the shortcut to, for instance, cmd+alt+9 since it seems that some keypress events are horked on mac, see bug 280805. 2) The best way would be to re-map the shortcut to cmd+Ä and cmd+Ö, that is how it's done in Safari and Camino. This is currently blocked by bug 294286.

Ah, so this is not a regression. I'm just paranoid now.

I'm quite sure that the shortcuts doesn't work on any locale that do not have "[" and "]" among the alphabetic letters on the keyboard (because of bug. For locales that can't re-map the shortcut to something that makes sense (because of bug 294286), I suppose the only solution is to remove the shortcut. CC:ing some l10n folks who represent locales that I think are affected - ideally, we should do the same here. For a summary of the problem, see comment #2.

The switch to cocoa (bug 326469) seems to have taken care of bug 294286. Trunk-only.

FYI, I put patches in bug 406831 and bug 406834 for Swedish/Finnish locales.

The patch of bug 359638 was landed. However, '[' and ']' are needed Alt key on Swedish keyboard layout. The patch doesn't support Alt+Cmd key combination can access to the command. So, I need more work for this. But that should be easy.

This bug should be fixed by the patch of bug 429160.

This should fix this bug. Adding alt/alt+shift charCodes to the alternativeCharCodes. And they have Alt mask in the data. XBL will be able to ignore the alt key like shift key (only when the data has alt mask). This also fixes bug 429160. (The all parts is needed by this bug, therefore I moved this patch from bug 429160.)

Oops, I forgot an information. I cannot use Cmd+Alt+8 (Cmd+[) on Swedish keyboard layout. Maybe, the key combination is eaten by system (or utils?). I used debugger, but I cannot break in the related code at this case. However, Cmd+Alt+9 (Cmd+]) works fine for me.

(In reply to comment #11) > Oops, I forgot an information. I cannot use Cmd+Alt+8 (Cmd+[) on Swedish > keyboard layout. Maybe, the key combination is eaten by system (or utils?). Cmd+Opt+8 may be used for enable/disable zoom. To disable this system shortcut key, open [System Preferences] -> [Keyboard & Mouse] -> [Keyboard Shortcuts] -> [Universal Access] and turn off [Turn zoom on or off].

updating for latest trunk.

Karl: Would you review this? This patch should be landed.

This patch does not work fine for me... When I select Swedish keyboard layout and press Cmd+Opt+9, unshiftedChar: 0x39("9"), shiftedChar: 0x29(")"), shiftedCmdChar: 0x39("9"), unshiftedAltChar: 0xbb("ª"), shiftedAltChar: 0xe1("·"). "]" does not appears. Though I can input "]" into <input> elements with Opt+9. This is strange, but when I select French layout, generated chars seems to be fine. When I select French keyboard layout and press Cmd+Opt+9, unshiftedChar: 0x8d("ç"), shiftedChar: 0x39("9"), shiftedCmdChar: 0x39("9"), unshiftedAltChar: 0x82("Ç"), shiftedAltChar: 0xe7("Á"). OS: OS X 10.5.2 Keyboard: MacBook (Japanese Keyboard) Should I use Swedish keyboard instead of Japanese keyboard?

Are you using the patch of bug 430419? Cmd+Option was killed on current trunk.

fix a mistake and reset the |state| at KeyTranslate(). Sakai-san, how about this patch?

mmm... this means 'k' key was pressed.

Oops, the previous comment is a reply to: > unshiftedAltChar: 0xbb("ª"), shiftedAltChar: 0xe1("·")

(In reply to comment #19) > Oops, the previous comment is a reply to: > > > unshiftedAltChar: 0xbb("ª"), shiftedAltChar: 0xe1("·") > er, no. 0xe1 may be Alt+Shift+'.'. It's too strange... If that was a bug of 10.5, we cannot fix this bug on 1.9 on 10.5 :-( # Of course, on 10.4, the patch works fine.

(In reply to comment #15) > unshiftedAltChar: 0xbb("ª"), shiftedAltChar: 0xe1("·"). > unshiftedChar: 0x8d("ç"), shiftedChar: 0x39("9"), shiftedCmdChar: 0x39("9"), > unshiftedAltChar: 0x82("Ç"), shiftedAltChar: 0xe7("Á"). Are you intentionally reporting conversions from the mac/roman charset? http://www.unicode.org/Public/MAPPINGS/VENDORS/APPLE/ROMAN.TXT Should KeyTranslate return Unicode characters? (In reply to comment #14) > Would you review this? I like the strategy of storing the consumed modifiers in alternativeCharCodes. I need to think more about the details of implementation. + if (mod & NS_ALT_MASK && nativeKeyEvent->isMeta && + nativeKeyEvent->isAlt) { Is there a situation where mod & NS_ALT_MASK is true but isAlt is false? Is there a reason why this should be done only when isMeta?

(In reply to comment #21) > (In reply to comment #15) > > unshiftedAltChar: 0xbb("ª"), shiftedAltChar: 0xe1("·"). > > > unshiftedChar: 0x8d("ç"), shiftedChar: 0x39("9"), shiftedCmdChar: 0x39("9"), > > unshiftedAltChar: 0x82("Ç"), shiftedAltChar: 0xe7("Á"). > > Are you intentionally reporting conversions from the mac/roman charset? > http://www.unicode.org/Public/MAPPINGS/VENDORS/APPLE/ROMAN.TXT > Should KeyTranslate return Unicode characters? Oh, I forgot that. KeyTranslate should return non-Unicode character. I need to change there. (But '[' and ']' are in ASCII range, so, such bug should not be.) > (In reply to comment #14) > + if (mod & NS_ALT_MASK && nativeKeyEvent->isMeta && > + nativeKeyEvent->isAlt) { > > Is there a situation where mod & NS_ALT_MASK is true but isAlt is false? After this many regressions, the key handling on each widgets will be changed by other developers. Then, this will keep the patch's behavior. > Is there a reason why this should be done only when isMeta? right.

(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #14) > > + if (mod & NS_ALT_MASK && nativeKeyEvent->isMeta && > > + nativeKeyEvent->isAlt) { > > > > Is there a situation where mod & NS_ALT_MASK is true but isAlt is false? > > After this many regressions, the key handling on each widgets will be changed > by other developers. Then, this will keep the patch's behavior. er, no, this should be assertion.

(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (In reply to comment #14) > > > + if (mod & NS_ALT_MASK && nativeKeyEvent->isMeta && > > > + nativeKeyEvent->isAlt) { > > > > > > Is there a situation where mod & NS_ALT_MASK is true but isAlt is false? > > > > After this many regressions, the key handling on each widgets will be changed > > by other developers. Then, this will keep the patch's behavior. > > er, no, this should be assertion. Oops, I missed. You misunderstand that. When isAlt but !(mod & NS_ALT_MASK), that should not be processed.

(In reply to comment #24) > (In reply to comment #23) > Oops, I missed. You misunderstand that. When isAlt but !(mod & NS_ALT_MASK), > that should not be processed. Ugh, no, you're right. I'm being confused....

KeyTranslateToUnicode is added. That returns Unicode character of the result of KeyTranslate.

Comment on attachment 317897 [details] [diff] [review] Patch v2.0 + if (ch != nativeKeyEvent->charCode) { + nsShortcutCandidate key(ch, 0); + aCandidates.AppendElement(key); + } + +#ifdef XP_MACOSX + // Mac's alt key is sometimes just a modifier key (e.g., Alt+Enter), + // however, that is also used for switching the characters like Shift. + // Therefore, we can ignore the Alt key on Mac. + PRUint8 mod = nativeKeyEvent->alternativeCharCodes[i].mModifierKeys; + if (mod & NS_ALT_MASK) { + NS_ASSERTION(nativeKeyEvent->isAlt, "Alt key is not pressed"); + nsShortcutCandidate alternativeKey(ch, nsIDOMNSEvent::ALT_MASK); + aCandidates.AppendElement(alternativeKey); + } +#endif If (mod & NS_ALT_MASK), do you know any reason why nsShortCutCandidate(ch,0) also needs to be sent [in addition to nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK)]? (This is relevant to approach B below.) -- There are (at least) two general approaches to dealing with (alternate-char-dependent) consumed modifiers such as these. A. Store all (consumed and unconsumed) modifier keys in the event (isAlt, etc.), and store the consumed modifiers with each nsAlternativeCharCode; or B. Store the unconsumed (i.e. remaining) modifiers with each nsAlternativeCharCode, and set the modifiers in the event appropriately for the charCode in the event. The problem that I see with approach A is that the consumed modifiers are not stored for charCode. Currently Windows code removes consumed modifiers for charCode http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsKeyboardLayout.cpp&rev=3.12&mark=147,152,156-158#144 This may cause some problems because alternative characters are still sent with only the unconsumed modifiers. For example, if Ctrl-Shift-2 is pressed and ZWNJ is usually produced with that combination, then currently charCode is ZWNJ and only isShift (not isCtrl) is set, but "@" (or some other character) is still be sent as an alternativeCharCode, so this looks like (Shift-)@ instead of Ctrl-(Shift-)@. The advantage I see in Approach B is that it provides the ability to set charCode and isAlt, etc. on the DOM event according to what we guess is the most likely intended character/modifier combination. With approach B, the logic in nsXBLPrototypeHandler::ModifiersMatchMask could be simplified by passing in the unconsumed modifiers (and so aEvent would not be needed). This would need a helper function to get the modifier bit mask from the event for the non-alternate characters and keyCodes and mouse events. Even with approach A, the logic in nsXBLPrototypeHandler::ModifiersMatchMask can be simplified by making the modifiers in nsXBLPrototypeHandler consistent with nsIDOMNSEvent. e.g. const PRInt32 nsXBLPrototypeHandler::cMeta = nsIDOMNSEvent::META_MASK; const PRInt32 nsXBLPrototypeHandler::cMetaMask = cMeta << 4; -- + UInt8 buf = UInt8(ch); + CFStringRef str = CFStringCreateWithBytes(kCFAllocatorDefault, &buf, 1, + (CFStringEncoding)aEncoding, false); If KeyTranslate returns two characters then it would be safer to return NUL. (or, if you like, translate both characters to Unicode as they may be represented by a single Unicode character). + NSString* nsstr = (NSString*)str; I'm not familiar with this. Is there a reason not to use reinterpret_cast? Or could using CFStringGetLength and CFStringGetCharacterAtIndex make the cast unnecessary? + ch = [nsstr length] > 0 ? [nsstr characterAtIndex:0] : 0; Only return characterAtIndex:0 when length = 1. + if (unshiftedAltChar || shiftedAltChar) { + nsAlternativeCharCode altCharCodes(unshiftedAltChar, shiftedAltChar, + NS_ALT_MASK); + outGeckoEvent->alternativeCharCodes.AppendElement(altCharCodes); + } Can we be (reasonably) sure that unshiftedAltChar differs from unshiftedChar? If not then these should only be added when they differ from the alt-free versions. -- Why does this block bug 429510? The Cmd+Shift situation doesn't need this code does it?

(In reply to comment #27) > If (mod & NS_ALT_MASK), do you know any reason why > nsShortCutCandidate(ch,0) also needs to be sent > [in addition to nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK)]? > (This is relevant to approach B below.) Alt key also used an modifier key. E.g., Alt+Enter. > + UInt8 buf = UInt8(ch); > + CFStringRef str = CFStringCreateWithBytes(kCFAllocatorDefault, &buf, 1, > + (CFStringEncoding)aEncoding, > false); > > If KeyTranslate returns two characters then it would be safer to return NUL. > (or, if you like, translate both characters to Unicode as they may be > represented by a single Unicode character). ?? KeyTranslate returns one character. If the keyboard layout can input multibyte characters, it should be used UCKeyTranslate. > Why does this block bug 429510? > The Cmd+Shift situation doesn't need this code does it? They need to change same point in nsChildView.mm. So, if you like, I can add the code for bug 429510 to the next patch of this bug.

(In reply to comment #28) > (In reply to comment #27) > > If (mod & NS_ALT_MASK), do you know any reason why > > nsShortCutCandidate(ch,0) also needs to be sent > > [in addition to nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK)]? > > (This is relevant to approach B below.) > > Alt key also used an modifier key. E.g., Alt+Enter. I had hoped that mod & NS_ALT_MASK == 0 in that situation. Why should it be non-zero? > KeyTranslate returns one character. If the keyboard layout can input multibyte > characters, it should be used UCKeyTranslate. "The KeyTranslate function returns the values that correspond to one or possibly two characters that are generated by the specified virtual key code." http://developer.apple.com/documentation/Carbon/Reference/Event_Manager/Reference/reference.html#//apple_ref/c/func/KeyTranslate > > Why does this block bug 429510? > > The Cmd+Shift situation doesn't need this code does it? > > They need to change same point in nsChildView.mm. So, if you like, I can add > the code for bug 429510 to the next patch of this bug. Please keep the patches separate. Whichever patch is ready first can land first. I don't expect the modifications then required for the other patch will be too complicated.

(In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > If (mod & NS_ALT_MASK), do you know any reason why > > > nsShortCutCandidate(ch,0) also needs to be sent > > > [in addition to nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK)]? > > > (This is relevant to approach B below.) > > > > Alt key also used an modifier key. E.g., Alt+Enter. > > I had hoped that mod & NS_ALT_MASK == 0 in that situation. > Why should it be non-zero? Ah, Do you mean that nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK) also fires the nsShortCutCandidate(ch, 0)? If so, you're right.

(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > (In reply to comment #27) > > > > If (mod & NS_ALT_MASK), do you know any reason why > > > > nsShortCutCandidate(ch,0) also needs to be sent > > > > [in addition to nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK)]? > > > > (This is relevant to approach B below.) > > > > > > Alt key also used an modifier key. E.g., Alt+Enter. > > > > I had hoped that mod & NS_ALT_MASK == 0 in that situation. > > Why should it be non-zero? > > Ah, Do you mean that nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK) also > fires the nsShortCutCandidate(ch, 0)? > If so, you're right. er, no. Even if you meant so, we should use nsShortCutCandidate(ch, 0) first. Because Cmd+Opt+foo should be processed then Cmd+foo.

(In reply to comment #31) > (In reply to comment #30) > > Ah, Do you mean that nsShortCutCandidate(ch, nsIDOMNSEvent::ALT_MASK) also > > fires the nsShortCutCandidate(ch, 0)? Yes that's what I'm asking about. > er, no. Even if you meant so, we should use nsShortCutCandidate(ch, 0) first. > Because Cmd+Opt+foo should be processed then Cmd+foo. If Opt+bar provides foo, then I can't think of any reason why we'd need to process Cmd+Opt+foo (as well as Cmd+foo and Cmd+Opt+bar), but maybe you can?

(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > er, no. Even if you meant so, we should use nsShortCutCandidate(ch, 0) first. > > Because Cmd+Opt+foo should be processed then Cmd+foo. > > If Opt+bar provides foo, then I can't think of any reason why we'd need to > process Cmd+Opt+foo (as well as Cmd+foo and Cmd+Opt+bar), but maybe you can? Oh, I see. I agree.

* XBL and nsContentUtils are using modifier keys in their param. * nsContentUtils::GetModifierKeysFor returns the modifier key flags from DOM event. * nsChildView.mm also uses CapsLock state and NumLock state for (UC)KeyTranslate. * Only using CFString APIs in KeyTranslateToUnicode(). * KeyTranslateToUnicode() can process multibyte result.

Please ignore |NS_IS_INPUT_EVENT| in nsGUIEvent.h.

(In reply to comment #17) > Created an attachment (id=317733) [details] > Patch v1.1 > > fix a mistake and reset the |state| at KeyTranslate(). > > Sakai-san, how about this patch? I've tested v2.0, but does not work for me. The problem seems in GetScriptManagerVariable(). Input Source ::GetScriptManagerVariable(smKCHRCache) French 0x7b1d23 German 0x7b2a0f Swedish 0x7b11ff Finnish 0x7b11ff Canadian English 0x7b11ff Irish 0x7b11ff Austrian 0x7b11ff Brazilian 0x7b11ff Portuguese 0x7b11ff When I use French or German layout, the return value of KeyTranslate reflects what layout is used. But when I use Swedish, Finnish and so on, the return value of KeyTranslate does not reflect what layout is used.

Comment on attachment 318203 [details] [diff] [review] Patch v3.0 Thanks, Masayuki. This feels tidier to me. nsContentUtils: + * Get the modifier keys as the flags from aDOMEvent. "...keys as flags..." + PRUint8 realModKeys = GetModifierKeysFor(aDOMEvent); On Windows, these are not actually all the real modifiers. Passing unconsumed modifiers to nsAlternativeCharCode and storing them there looks like the best approach to me, but this patch as it is now is a step in the right direction, and so I think I'd be happy to take this even without that change. (Maybe this could change when the Windows code is fixed up.) + // removed from the "real" modifier keys. That is "actual" modifier keys. "unconsumed" rather than "actual" sounds clearer to me. + nsAlternativeCharCode &altChar = + nativeKeyEvent->alternativeCharCodes[i]; Use "const nsAlternativeCharCode &altChar" to make it clear that there is no intention to modify nativeKeyEvent->alternativeCharCodes[i]. -- nsXBLPrototypeHandler: + enum { USE_EVENT_MODIFIER = PR_UINT8_MAX }; + // if aCharCode is not zero, it is used instead of the charCode of aKeyEvent. PRBool KeyEventMatched(nsIDOMKeyEvent* aKeyEvent, PRUint32 aCharCode = 0, - PRBool aIgnoreShiftKey = PR_FALSE); + PRUint8 aModifierKeys = USE_EVENT_MODIFIER); Consider PRBool KeyEventMatched(PRUint32 aKeyCode, PRUint32 aCharCode, PRUint8 aModifierKeys); It would mean moving the nsContentUtils::GetModifierKeysFor call from KeyEventMatched to nsXBLKeyEventHandler::HandleEvent and nsXBLWindowKeyHandler::WalkHandlersInternal, but would make USE_EVENT_MODIFIER unnecessary. +nsXBLPrototypeHandler::ModifiersMatchMask(PRUint8 aModifierKeys) PRUint8 modifiersToMatch = mKeyMask >> 4; return (mKeyMask & modifiersToMatch) == (aModifierKeys & modifiersToMatch); -- nsGUIEvent: + // This modifier keys are ignored at finding the matching command. "These modifier keys..." + // Note that don't set MS_SHIFT_MASK to mModifierKeys. "Note that mModifierKeys should not include NS_SHIFT_MASK." + PRUint32 mModifierKeys; If you don't change these to unconsumed modifiers, then how about mConsumedModifiers to clarify the distinction from nsShortcutCandidate? -- nsChildView: + PRUint32 baseMod = ::GetCurrentKeyModifiers() & + (alphaLock | kEventKeyModifierNumLockMask); Better to use [aKeyEvent modifierFlags] with NSAlphaShiftKeyMask NSNumericPadKeyMask as that information was recorded at the time of the event. (The current state may now be different.) http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSEvent_Class/Reference/Reference.html#//apple_ref/doc/c_ref/NSAlphaShiftKeyMask + NS_ToLower(unshiftedChar) == NS_ToLower(shiftedCmdChar)) { NS_ToLower will be truncating PRUint32 to char, I think. Perhaps this test should only be for nsAlternativeCharCode(unshiftedChar, shiftedChar). nsAlternativeCharCode(unshiftedAltChar, shiftedAltChar, + NS_ALT_MASK) should still be added for Dvorak-QWERTY layouts shouldn't it? (It also looks like this test will fail when unshiftedChar is not ASCII as shiftedCmdChar looks like it is often a QWERTY character in that case. Perhaps a more Dvorak-QWERTY-specific test is needed, but that can be a separate bug if you like.)

(In reply to comment #36) > Input Source ::GetScriptManagerVariable(smKCHRCache) > French 0x7b1d23 > German 0x7b2a0f Thanks for this. It might be possible that the cache still has the same pointer but now contains a different layout, though the behavior you report makes that seem unlikely. If Apple are no longer supporting the old APIs then we may need to dlopen the necessary library on 10.5 to get access to http://developer.apple.com/documentation/TextFonts/Reference/TextInputSourcesReference/Reference/reference.html#//apple_ref/c/func/TISCopyCurrentKeyboardLayoutInputSource and http://developer.apple.com/documentation/TextFonts/Reference/TextInputSourcesReference/Reference/reference.html#//apple_ref/c/func/TISGetInputSourceProperty for the uchr data http://developer.apple.com/documentation/TextFonts/Reference/TextInputSourcesReference/Reference/reference.html#//apple_ref/doc/c_ref/kTISPropertyUnicodeKeyLayoutData But let's get this working for 10.4 first because most of the code will be shared with 10.5.

> Perhaps this test should only be for > nsAlternativeCharCode(unshiftedChar, shiftedChar). > nsAlternativeCharCode(unshiftedAltChar, shiftedAltChar, + NS_ALT_MASK) > should still be added for Dvorak-QWERTY layouts shouldn't it? Dvorak-QWERTY's cmd key also changes the layout even if Option is pressed. The checking is needed. So, this patch doesn't perfect for such layout. But now, it should be ok. And 10.5 issue should be separated to another bug. Because this bug is not a blocker, but this patch have some important code, that is needed the blocker bug (bug 429510).

Comment on attachment 318337 [details] [diff] [review] Patch v3.1.1 nsAlternativeCharCode: + PRUint32 mModifierKeys; Please change the name of this as it really means the opposite of the member of nsShortcutCandidate within the same name. I suggest mConsumedModifiers for nsAlternativeCharCode. nsXBLPrototypeHandler: PRBool KeyEventMatched(nsIDOMKeyEvent* aKeyEvent, + PRUint32 aCharCode, + PRUint8 aModifierKeys); It would be nice to replace nsIDOMKeyEvent* aKeyEvent with PRUint32 aKeyCode as that is the only part of the event that needs to be used, but this is not essential. Only do this if you want to. (In reply to comment #39) > Dvorak-QWERTY's cmd key also changes the layout even if Option is > pressed. Oh, wow. I'm pleased I'm not using that layout. > The checking is needed. So, this patch doesn't perfect for such > layout. But now, it should be ok. Yes, it's hard to know what's perfect for that layout, but this should be good for most layouts. + PRBool isCmdSwitchLayout = + !NS_IsAscii(unshiftedChar) || !NS_IsAscii(shiftedCmdChar) || + NS_ToLower(char(unshiftedChar)) == NS_ToLower(char(shiftedCmdChar)); + if (!outGeckoEvent->isMeta || isCmdSwitchLayout) { I'm happy with the end result of this logic but IIUC isCmdSwitchLayout is really a isNotCmdSwitchLayout. How about? PRBool isCmdSwitchLayout = NS_IsAscii(unshiftedChar) && NS_IsAscii(shiftedCmdChar) && NS_ToLower(char(unshiftedChar)) != NS_ToLower(char(shiftedCmdChar)); if (!outGeckoEvent->isMeta || !isCmdSwitchLayout) { Otherwise good.

fix a nit.

Comment on attachment 318346 [details] [diff] [review] Patch v3.1.2 Oops, sorry. You were reviewed the previous patch...

Sorry, the previous patch has a debug code.

we need to add a keyboard navigation litmus suite for these kind of tests

Comment on attachment 318395 [details] [diff] [review] Patch v3.2.1 This looks good to me, thanks. (I trusting interdiff here.) However, this was discussed at the Firefox meeting this morning, and drivers would like to split up the patch as much as possible. I think a good way to split this up would be to first do only the KeyTranslateToUnicode and baseMod changes (together with isCmdSwitchLayout which goes with baseMod). The unshiftedAltChar/shiftedAltChar parts could then be landed separately with the XP code. I'll give these two parts a complete look over (to check that interdiff didn't hide anything), but its worth getting Josh's review on the first part (as he would know more than I about the Apple APIs).

o.k. I give up to fix this bug at Gecko1.9. I'll retry to fix this next Fx release.

(In reply to comment #36) > The problem seems in GetScriptManagerVariable(). > > Input Source ::GetScriptManagerVariable(smKCHRCache) > French 0x7b1d23 > German 0x7b2a0f > Swedish 0x7b11ff > Finnish 0x7b11ff > Canadian English 0x7b11ff > Irish 0x7b11ff > Austrian 0x7b11ff > Brazilian 0x7b11ff > Portuguese 0x7b11ff I wonder whether ::GetResource('kchr', keyLayoutID) would work any differently?

(In reply to comment #49) > I wonder whether ::GetResource('kchr', keyLayoutID) would work any differently? GetResource didn't work fine for me. I don't know the reason.

Updating for latest trunk. And I rewrite the patch of nsChildView.mm. (XP part is not changed.) This patch appends an alternative character when both Option key and Command key are pressed. Option+Command key layout is same as Option key layout in most keyboard layouts. However, there are some special cases. 1. On Greek, Opt+Cmd keybaord layout is different from Opt keyboard layout. I'm not sure the reason. 2. On non-ASCII keyboard layout, e.g., on Arabic, Hebrew, Russian, Opt+Cmd keyboard layout is same as Cmd keyboard layout, so, looks like Opt key is ignored. 3. Dvorak-QWERTY like layout. I think that we don't need to think the case 1. Because the characters which only can inputted with Option key should not be used for shortcut key combination. So, most native applications should not have such key combination. By the reason, most users don't know Opt+Cmd uses different keyboard layout, probably. Therefore, most users expect that Opt+Cmd should work as Cmd+(Opt+foo). For the case 2, we already add the alternative characters which is resolved from only with Cmd key. So, we can improve the accessibility by adding Optioned characters simply. Case 3 is difficult. We cannot guess what character is expected with the keyboard layout. If we can assume that Opt+Cmd keyboard layout is always same as US keyboard layout, we can use the US keyboard layout. However, I don't have good idea for checking it dynamically. Therefore, I add to get the actual Cmd+Opt keyboard layout dynamically. This case is not in the previous patch. # of course, this patch is not for 1.9.1. Karl, if you are busy on 1.9.1, you don't need to review this now.

Comment on attachment 359275 [details] [diff] [review] Patch v4.0 >+ // nsShortcutCandidate::mModifierKeys means the mCharCode should be tried to >+ // execute a command with the key state. Something like "nsShortcutCandidate::mModifierKeys describes the modifier key state that should be used with mCharCode when matching shortcuts"? >+ // >+ // If nsAlternativeCharCode::mModifierKeys is not zero, the charCode was >+ // generated with some modifier keys. There, the modifier keys should be >+ // removed from the "real" modifer keys. That is "unconsumed" modifier keys. Maybe you mean "nsShortcutCandidate::mModifierKeys" here, or do you mean: "If nsAlternativeCharCode::mConsumedModifiers is not zero, the charCode was modified by some modifier keys. There, these consumed modifier keys should be removed from the set of all modifier keys, leaving the "unconsumed" modifier keys." if (!HasASCIIDigit(aCandidates)) { for (PRUint32 i = 0; i < len; ++i) { PRUint32 ch = nativeKeyEvent->alternativeCharCodes[i].mShiftedCharCode; if (ch >= '0' && ch <= '9') { nsShortcutCandidate key(ch, PR_FALSE); aCandidates.AppendElement(key); break; } } } PR_FALSE is not right here. I guess we need a test to check this. I first thought that realModKeys & ~nativeKeyEvent->alternativeCharCodes[i].mConsumedModifiers would be sensible, but that brings up the question of what happens when mUnshiftedCharCode should have a different set of consumed modifiers to mShiftedCharCode. In all other cases in this function only mShiftedCharCode is used as a candidate when isShift and only mUnshiftedCharCode when !isShift, so mConsumedModifiers can be set appropriately according to isShift. The best thing I can think of is simply to only add this candidate when mConsumedModifiers == 0, in which case using key(ch, realModKeys). I don't think we really care about considering ASCII digits from a different Shift-state when Option is required (as well as the Shift) to generate the ASCII digit. (If the layout is such that Option must be used as well as Shift to generate an ASCII digit, then the user can press Option and Shift together). Have you thought about how nsAlternativeCharCode::mConsumedModifiers should be considered in GetAccessKeyCandidates (and maybe callers)? >+ // "Meta+']'". Note that mConsumedModifiers should not include NS_SHIFT_MASK. >+ // Because shift key state is processed by >+ // nsContentUtils::GetAccelKeyCandidates complexly. ... "NS_SHIFT_MASK, because shift" ... >- UInt32 lockState = 0; >- if (nsCocoaUtils::GetCocoaEventModifierFlags(aKeyEvent) & NSAlphaShiftKeyMask) >- lockState |= alphaLock; >- if (nsCocoaUtils::GetCocoaEventModifierFlags(aKeyEvent) & NSNumericPadKeyMask) >- lockState |= kEventKeyModifierNumLockMask; >+ const UInt32 kLockState = GetCurrentKeyLockState(aKeyEvent); >- UInt32 shiftLockMod = shiftKey | lockState; >+ const UInt32 kShiftLockMod = shiftKey | kLockState; The leading "k"s make these (and others) look like fixed global constant (but they are only constant during this execution of the function, and only an approximation of what the state may have been when the key was pressed.) Is there a need to change the variable names? >+ // Note that Option key should be used as a part of shortcut key on Mac, >+ // it only should be used for switching the keyboard layout like Shift >+ // key. Therefore, Alt consumed key conbination should be preferred. Do you mean "Option key should _not_ be used"? >+ // Some keyboard layouts changes the layout at Cmd+Option, e.g., Greek. "layouts change" >+ // However, we don't need to worry about that. Because the characters "that, because" >+ // which are in Option layout should not be used shortcut keys. So, Do you mean "Cmd+Option" layout? >+ // other native applications should not have such accesskeys, probably. >+ // Therefore, most users don't know the characters which can be sent at >+ // Cmd+Option keys. So, we can assume that most users want to input the >+ // optioned characters without Cmd key. "most users want to input the character from the Option layout, without modification from the Cmd key."? >+ PRUint32 cmdedAltChar = >+ GetUniCharFromKeyTranslate(kt, key, kAltLockMod); >+ const UInt32 kAltShiftLockMod = shiftKey | kAltLockMod; >+ PRUint32 cmdedAltShiftChar = >+ GetUniCharFromKeyTranslate(kt, key, kAltShiftLockMod); Naming altChar/altShiftChar would be more consistent with other variable names. (I see that cmd is involved for Dvorak-QWERTY but see below.) >+ // However, when current keyboard layout is Dvorak like, we cannot >+ // use Option keyboard layout characters. Instead of them, we should >+ // try to get Cmd+Option keyboard layout. (In reply to comment #51) > Case 3 is difficult. We cannot guess what character is expected with the > keyboard layout. If we can assume that Opt+Cmd keyboard layout is always same > as US keyboard layout, we can use the US keyboard layout. However, I don't have > good idea for checking it dynamically. Therefore, I add to get the actual > Cmd+Opt keyboard layout dynamically. This case is not in the previous patch. I don't feel convinced that it is worth adding code to do this. It seems to me that Dvorak-QWERTY provides the Option-layout char code from a QWERTY layout so that the event for Cmd+Option+key on Dvorak-QWERTY looks the same as the event for Cmd+Option+key on a QWERTY layout. I doubt the user has really memorized the positions of the QWERTY Option layout and expects Cmd to start making Option behave differently. I think this is similar to the Greek case. If we are going to treat Option as a consumed modifier for this layout and send Cmd+char, then I think char would be best determined from Option+key. BTW, on Dvorak-QWERTY we don't append altCharCodes(cmdedChar, cmdedShiftChar). Is there a reason why we shouldn't do this for Cmd+Option+key? i.e. should we provide Cmd+Option+(QWERTY-char)? >+ if (cmdedAltChar || cmdedAltShiftChar) { >+ nsAlternativeCharCode altCharCodes(cmdedAltChar, cmdedAltShiftChar, >+ NS_ALT_MASK); >+ outGeckoEvent->alternativeCharCodes.AppendElement(altCharCodes); I think these should be appended with the ALT modifier marked as consumed in this way only when ALT actually changed the character; Otherwise ALT is just being ignored. For the space key, for example, ALT does not modify the character so Cmd+Alt+Space should not activate the same shortcuts as Cmd+Space. As discussed above, which character to test for the change should probably depend on isShift. >+ // bug 306585 >+ if (navigator.platform.indexOf("Mac") == 0) { >+// test currently does not work, getting the Swedish layout fails on 10.4 These tests should really be running, and I don't think it would be too difficult to get them running. I think it just needs removal of ::GetResource('kchr', kt.mLayoutID) (which doesn't work) and use of KLGetKeyboardLayoutWithIdentifier/KLGetKeyboardLayoutProperty as indicated here: // There are no know cases where GetResource succeeds here, and so // tests (gOverrideKeyboardLayout.mOverrideEnabled) currently end up // with no keyboard layout. KLGetKeyboardLayoutWithIdentifier and // KLGetKeyboardLayoutProperty with kKLKCHRData would be useful here. kt.mKchr.mHandle = ::GetResource('kchr', kt.mLayoutID); if (!kt.mKchr.mHandle && !gOverrideKeyboardLayout.mOverrideEnabled) kt.mKchr.mHandle = (char**)::GetScriptManagerVariable(smKCHRCache);

Masayuki, are you still working on that page? You haven't had responded for over a half year now. I see a similar problem with the German keyboard layout. '[' and ']' are reachable via Cmd+Alt and don't trigger an action. Only the menu entry flashes once. The shortcut works when the history menu is open. Is this covered by this but?

(In reply to comment #53) > I see a similar problem with the German keyboard layout. '[' and ']' are > reachable via Cmd+Alt and don't trigger an action. Only the menu entry flashes > once. The shortcut works when the history menu is open. Is this covered by this > but? Ditto for the "Italian - Pro" keyboard layout.

See also bug 437519 about French.

Just downloaded 3.6 RC2 and see that this issue is still around (I have English as the preferred language in Mac OS X but use the ”Swedish Pro” keyboard layout. When pressing the keyboard shortcut for ”Back” (cmd alt/option 8) the History menu just blinks and nothing happens. But if one first pulls down the History menu by clicking on it it works! This is the main reason for why I don't want to switch to Firefox as my main browser!

I have just downloaded firefox 13 and now I seen to have got the problem. My shortcuts for going back/forward are cmd+ö/cmd+ä (swedish keyboard). Before in firefox 12 everything worked fine, by pressing cmd+ö would bring me back a page. But now (in firefox 13), when pressing cmd+ö just makes the history menu blinks and that's it, but it works if I first press history and then press cmd+ö, just as this bug discribes. As seen this bug has been around for 7 years I hope it will be taken care of soon.

Have you tried Alt+Left for Back and Alt+Right for Forward? With me they work every time (on Linux) and they ought to be more portable across platforms and locales.

(In reply to Tony Mechelynck [:tonymec] from comment #58) > Have you tried Alt+Left for Back and Alt+Right for Forward? With me they > work every time (on Linux) and they ought to be more portable across > platforms and locales. No, it doesn't work. Luckily I found a workaround, I used a thirdparty software (bettertouchtools) which let me assign a shortcut for two-finger-swipes (in mac you can use two-finger-swipes for going back/forward). It's not a great solution but it works.

(In reply to Fredrik from comment #57) > I have just downloaded firefox 13 and now I seen to have got the problem. My > shortcuts for going back/forward are cmd+ö/cmd+ä (swedish keyboard). Before > in firefox 12 everything worked fine, by pressing cmd+ö would bring me back > a page. But now (in firefox 13), when pressing cmd+ö just makes the history > menu blinks and that's it, but it works if I first press history and then > press cmd+ö, just as this bug discribes. As seen this bug has been around > for 7 years I hope it will be taken care of soon. Fredrik, please see comment #2. Back/fwd used to be cmd+alt+8 / cmd+alt+9 for Swedish. This bug is about these kind of key combos not working. Because of this bug (and that it felt smarter to change to something simpler) we changed to cmd+Ö /cmd+Ä in bug 406831 (4.5 years ago). So, I think you should file a new bug if cmd+ö/ä doesn't work for you (it should probably go in Mozilla Localizations, sv-SE).

Same problem with german version since v.13: Keyboard shortcuts in German version for Page-forward (CMD-Ä) and Page back (CMD-Ö) do not work since version 13. Same problem with v14.0.1. Works perfect in v.12.

(In reply to Gulliver from comment #61) > Same problem with german version since v.13: > > Keyboard shortcuts in German version for Page-forward (CMD-Ä) and Page back > (CMD-Ö) do not work since version 13. Same problem with v14.0.1. > Works perfect in v.12. File a new bug, it should be really another bug.

> File a new bug, it should be really another bug. Done: https://bugzilla.mozilla.org/show_bug.cgi?id=1515464