STEPS TO REPRODUCE 1. load testcase 2. type Alt+Shift++ ACTUAL RESULTS Nothing happens EXPECTED RESULTS The text input with the '+' becomes focused (background changes to green) Regression window: Works in Firefox trunk build 2006-07-16-22 on Windows XPSP2 Broken in Firefox trunk build 2006-07-18-04 on Windows XPSP2 Most likely caused by bug 339723

WFM in my environment. What's your keyboard layout?

Wow, but I can see same problem with 1,2,3 and 0 in JIS keyboard layout.

(In reply to comment #2) > What's your keyboard layout? Swedish (se-latin1). We should probably use MapVirtualKey also on trunk, that is, adopt the patch in bug 351310.

(In reply to comment #4) > We should probably use MapVirtualKey also on trunk Turns out: it's already there - but still doesn't work somehow. Seems unrelated to bug 339723, though, as we seem to consequently use the shifted accesskey where per bug 351310 it should be the unshifted one. Requesting blocking1.9 as shipping in this state would make us more inconsistent still (where Firefox 3 behaves arbitrarily differently than Firefox 2).

This testcase should work consistently on most keyboard layouts (whereas accesskey="+" is unshifted for Mats but shifted for the rest of us).

Masayuki, do you have time to look at this?

Masayuki, can you tell us if this needs to block?

This is adhoc approach. When both alt and shift is downed, the VK_OEM_* keys meaning character is used for the keypress event. Of course, we should get the unshifted character at that time, but I'm not sure how to get that. I think that we should send both shifted/unshifted characters at one keypress event. And shortcut key handler should use both keys for finding the executed item. However, we don't have much time now. We need to find simple way for this bug.

(In reply to comment #8) > Masayuki, can you tell us if this needs to block? I'm not sure this should block the release. Because accesskey attribute of HTML is not good feature, it has some issues. However, if this bug is occurred in major site, this should be blocker.

Ere: the patch fixes two bugs: 1. I misunderstood VK_OEM_1, maybe. Probably it means ';' or ':' key. We assume that the key is NS_VK_SEMICOLON, therefore we should use ';' for it. 2. I didn't reset numOfShiftStates, it should be reset.

Comment on attachment 306610 [details] [diff] [review] Patch v1.0 Sorry, this patch has a obsolete part...

Note that I have no idea for other punctuation keys.

This is really better.

Comment on attachment 306693 [details] [diff] [review] Patch v2.0 Wait, I found accesskey handling method in ESM. I'll attach a better patch.

I believe that this approach is best way for intl accesskey/shortcutkey handling. An keyevent has shiftedCharCode and unshiftedCharCode (only when Alt or Ctrl is pressed). And accesskey handler (ESM) and shortcut key handler (XBLPrototypeHandler) are checking unshifteCharCode, shiftedCharCode and charCode as this order. Therefore, both characters become accessible. So, we can fix this bug on all keyboard layout. (Note that if both shifted and unshifted characters are accesskey (or shortcut key), the unshifted character is not used, but it is limitation of us.)

If roc agrees this approach, I'll create the patch of mac/gtk2 part.

> - keyCode(0), charCode(0), isChar(0) > + keyCode(0), charCode(0), unshiftedCharCode(0), isChar(0) > { > } > > /// see NS_VK codes > PRUint32 keyCode; > /// OS translated Unicode char > PRUint32 charCode; > + // OS translated Unicode char which is used for accesskey and accelkey > + // handling. If these are 0, charCode will be used, which is traditional > + // behavior. > + PRUint32 shiftedCharCode; > + PRUint32 unshiftedCharCode; Oops, shiftedCharCode is not initialized, it will be fixed in next patch after review.

Comment on attachment 306895 [details] [diff] [review] Patch v3.0 no test?

(In reply to comment #20) > (From update of attachment 306895 [details] [diff] [review]) > no test? What did you mean? I tested the patch on Win32. Note that the uninitialized member is always re-initialized on nsWindow::DispatchKeyEvent.

I'm sure the patch is tested. I meant that if you don't add a regression test for tinderbox, this will just break again.

The actual behavior depends on the keyboard layout. So, thinderbox might be able to test only US keyboard layout :-(

There's no way to use window utils to synthesize events that would be produced by a non-US keyboard layout? If so, how hard would it be to add such a way?

That still wouldn't test the widget end of things, but perhaps we should add some test-only widget service or something...

ok, if we assume that the all tinderbox machines are US keyboard layout, we can create the tests, maybe. However, I'm not sure how to test it, how to emulate the key inputs?

Maybe, my approach can fix bug 411647, bug 398264, bug 306585, bug 401086 and bug 88380. # but we need more work for bug 398264. Because both accel + 6 and accel + - are used on Fx.

The general idea of passing multiple keycodes sounds good to me, but why don't we try dispatching the true charcode first and use the unshifted/shifted variants as a fallback, wouldn't that make more sense?

(In reply to comment #28) > The general idea of passing multiple keycodes sounds good to me, but why don't > we try dispatching the true charcode first and use the unshifted/shifted > variants as a fallback, wouldn't that make more sense? I don't think so. Because accesskeys and accelkeys are combination of modifier keys and another *key*. And if there is accesskeys which is batting on some keyboard layout in web contents (e.g., '1' and '!' on US keyboard layout), the priority is changed by the accesskey modifier combination. I.e., Windows and Linux users can access to '!' element by Alt+Shift+1, but Mac users can access to '1' by Alt+1. This difference is not good thing.

Including Mac part and GTK2 part.

re-nominating to b1.9. I can fix this soon, probably.

I found bug 414130. I think we should not overwrite nsKeyEvent.charCode, because we don't need to overwrite for accesskeys and accelkeys. Maybe, I should do it in next patch. Roc: Please review XP part. The needed changes of bug 414130 are in each platform code.

(In reply to comment #31) > re-nominating to b1.9. I can fix this soon, probably. What has changed that you think the blocking decision should change?

Note, that in bug 399939 I change the same code.

(In reply to comment #33) > (In reply to comment #31) > > re-nominating to b1.9. I can fix this soon, probably. > > What has changed that you think the blocking decision should change? Because I found a way for fixing this bug completely. I didn't have good idea at that time. (In reply to comment #34) > Note, that in bug 399939 I change the same code. I think that your patch is not needed. Because my patch fixes the similar bugs on Win32/Linux/Mac.

removing the code of modifying charCode. This should fix bug 414130 and bug 399939.

(In reply to comment #35) > (In reply to comment #34) > > Note, that in bug 399939 I change the same code. > > I think that your patch is not needed. Because my patch fixes the similar bugs > on Win32/Linux/Mac. Oh, a part of your patch might be needed. > Localized ui with <alt+latin> I'm not sure this works fine by my patch. > Localized ui with <alt+non-Latin> This should work fine. Because the latest patch sends non-modified charCode. > Eng ui and <alt+non-latin> is ok too. This should work fine. Because the unshiftedCharCode should have latin char.

(In reply to comment #37) > (In reply to comment #35) > > (In reply to comment #34) > > > Note, that in bug 399939 I change the same code. > > > > I think that your patch is not needed. Because my patch fixes the similar bugs > > on Win32/Linux/Mac. > > Oh, a part of your patch might be needed. > I guess the full patch is required :) Because there described a bit another problem (but Karl wrote a bit about «shifting»). > > Localized ui with <alt+latin> > > I'm not sure this works fine by my patch. I've a looked the code: it doesn't. We convert all codes to Latin for shortcut events. > > Localized ui with <alt+non-Latin> > > This should work fine. Because the latest patch sends non-modified charCode. > > > Eng ui and <alt+non-latin> is ok too. > > This should work fine. Because the unshiftedCharCode should have latin char. I think it's fine now too, becouse at this time all codes are converted to Latin. My code is on higher level. It gets events and send to your one. And you did something like core if I'm not mistaken.

(In reply to comment #38) > I guess the full patch is required :) > Because there described a bit another problem (but Karl wrote a bit about > «shifting»). Did you mean case of Cyrillic? If so, cannot we resolve it on nsEventStateManager and nsXBLPrototypeHandler? It might help non-Linux platforms. > > > Localized ui with <alt+latin> > > > > I'm not sure this works fine by my patch. > I've a looked the code: it doesn't. We convert all codes to Latin for shortcut > events. Thanks. But I think we should not fire NS_KEYPRESS event many times at one key inputting. We only put one or more non-latin chars to new member (nsKeyEvent::localizedChars?).

mmm. The l10n resource should also have latin accesskeys, right? On Windows and Mac, the case cannot be fixed by us, probably. (The native applications how to fix this??)

(In reply to comment #39) > (In reply to comment #38) > > I guess the full patch is required :) > > Because there described a bit another problem (but Karl wrote a bit about > > «shifting»). > > Did you mean case of Cyrillic? If so, cannot we resolve it on > nsEventStateManager and nsXBLPrototypeHandler? It might help non-Linux > platforms. I mean all non-Latin characters (Cyrillic,Greek, Arabic). We speak much about Cyrillic because it's my native, but it's an example. I think we can't. It's very platform dependent: we should know about what keyboard layouts user has (and we find correct skortcut_key between them). After finishing Linux patch I will start to code Windows patch. If there is such problem on Mac I won't be able to solve it. > Thanks. But I think we should not fire NS_KEYPRESS event many times at one key > inputting. We only put one or more non-latin chars to new member > (nsKeyEvent::localizedChars?). See https://bugzilla.mozilla.org/show_bug.cgi?id=399939#c31 They say (karlt and roc) it's ok. Or do you mean something else? Didn't found what nsKeyEvent::localizedChars is.

(In reply to comment #41) > (In reply to comment #39) > > (In reply to comment #38) > > > I guess the full patch is required :) > > > Because there described a bit another problem (but Karl wrote a bit about > > > «shifting»). > > > > Did you mean case of Cyrillic? If so, cannot we resolve it on > > nsEventStateManager and nsXBLPrototypeHandler? It might help non-Linux > > platforms. > > I mean all non-Latin characters (Cyrillic,Greek, Arabic). We speak much about > Cyrillic because it's my native, but it's an example. > I think we can't. It's very platform dependent: we should know about what > keyboard layouts user has (and we find correct skortcut_key between them). > After finishing Linux patch I will start to code Windows patch. If there is > such problem on Mac I won't be able to solve it. Mac doesn't have accesskeys in menus, but HTML contents might have them. We need to fix on Mac too. > > Thanks. But I think we should not fire NS_KEYPRESS event many times at one key > > inputting. We only put one or more non-latin chars to new member > > (nsKeyEvent::localizedChars?). > > See https://bugzilla.mozilla.org/show_bug.cgi?id=399939#c31 > They say (karlt and roc) it's ok. > Or do you mean something else? > Didn't found what nsKeyEvent::localizedChars is. I don't think it's ok. Because web applications might check the key events. Your approach might changes the behavior of web applications.

(In reply to comment #42) > I don't think it's ok. Because web applications might check the key events. > Your approach might changes the behavior of web applications. Spoke with roc. Dispatching several events is bad idea. Did you test your patch with menus? Alt+MenuKey? I have building problems to test it. Let's cooperate to see what may be done. I will try to understand your changes carefully. I think if your approach failed (for my problem) we may «raise» some code that checks shortcuts and only then send event.

(In reply to comment #37) > > Eng ui and <alt+non-latin> is ok too. > > This should work fine. Because the unshiftedCharCode should have latin char. I can't see where the Latin character would come from. It looks like this patch removes the gdk_keymap_get_entries_for_keycode code that found the Latin code. Am I missing something? It also seems to be that command keys and menu access keys should only be looking at charCode or keyCode. I see that unshiftedCharCode and shiftedCharCode are necessary for the unfortunate situation where the usual meaning of shift (to change the level) has be taken to indicate a document (HTML) access key.

(In reply to comment #40) > mmm. The l10n resource should also have latin accesskeys, right? On Windows and > Mac, the case cannot be fixed by us, probably. (The native applications how to > fix this??) On 1.8 the charCode was obtained from the selected layout. It is nice to have menu accesskeys that are part of the strings for the menuitems (when such characters are available on the keyboard).

(In reply to comment #39) > We only put one or more non-latin chars to new member > (nsKeyEvent::localizedChars?). This may be a possibility, but I can't see a good way to resolve the issue of precedence of layouts. (last point in bug 399939 comment 36) (In reply to comment #42) > > Thanks. But I think we should not fire NS_KEYPRESS event many times at one > > key inputting. > I don't think it's ok. Because web applications might check the key events. > Your approach might changes the behavior of web applications. Is this significantly different from doing nsEventStateManager::ExecuteAccessKey multiple times or nsXBLKeyEventHandler::HandleEvent calling ExecuteHandler on multiple nsXBLPrototypeHandlers? I can see that it is happening at a different level but I don't know what difference this would make.

(In reply to comment #44) > (In reply to comment #37) > > > Eng ui and <alt+non-latin> is ok too. > > > > This should work fine. Because the unshiftedCharCode should have latin char. > > I can't see where the Latin character would come from. It looks like this > patch removes the gdk_keymap_get_entries_for_keycode code that found the Latin > code. Am I missing something? ok, I'll check it. > It also seems to be that command keys and menu access keys should only be > looking at charCode or keyCode. I don't think so. Because we cannot map chars to keycodes in XP level. And if we check the charCode first, the testcase of this bug doesn't work fine. I believe that we should check unshiftedCharCode first. If so, the preferred access key doesn't depend on whether the key combination has shift key. (Of course, the preferred access key depends on keyboard layout. But we cannot fix it, probably.) (In reply to comment #46) > (In reply to comment #39) > > We only put one or more non-latin chars to new member > > (nsKeyEvent::localizedChars?). > > This may be a possibility, but I can't see a good way to resolve the issue of > precedence of layouts. (last point in bug 399939 comment 36) I agree to your order in bug 399939 comment 36. O.K. This bug should fix the access keys and shortcut keys issue on *single* keyboard layout environment. And bug 399939 should fix the similar issue on *multiple* keyboard layout environment.

Comment on attachment 307497 [details] [diff] [review] Patch v4.0 The new patch will come.

We should fix this bug first. This patch improves accessibility of access keys/accel keys for *one* keyboard layout. And next, we need to fix bug 399939 for the users who are using two or more keyboard layouts. This patch includes: 1. access keys of web contents handling improvements (nsEventStateManager) 2. access keys of menu bar handling improvements (nsMenuBarFrame) 3. accel keys handling improvements (nsXBLPrototypeHandler) Note that this patch doesn't include the improvements for poped up menus. Because we have not been implementing any improvements for them (Alt/Ctrl/Meta keys are not pressed at that time...). And it supports incremental search, so, we cannot fix this on it, easily. This issue should be out of scope of this bug. nsKeyEvent has a new member accessCharCodes that is nsString. The key handlers check the codes from first char to last char in accessCharCodes. (Win and Mac send two characters, Linux sends 4 characters.) This flexible length array approach will help bug 399939. But PRUnichar is PRUint16. So, we cannot support non-BMP characters for accesskeys. We need the class of array of PRUint32... However, now, this should not be actual problem. Because non-BMP characters should not be used for access keys. And I also change the Linux part from previous patch. Now, that sends the current keyboard layout's unshifted character and shifted character. And also sends the alphabets inputtable keyboard layout's unshifted character and shifted character. By this changes, this patch should not have any regressions on Win/Mac/Linux. (Win and Mac don't care the non-current keyboard layout characters sending.)

-u8pw patch.

Masayuki, tested your patch: good job, cool. alt+Rus works with En menu, but alt+Latin with Rus menu doesn't as you told (my bug).

(In reply to comment #51) > alt+Rus works with En menu, but alt+Latin with Rus menu doesn't as you told (my > bug). Yes. I didn't try the latter case. It's really bug 399939.

er, on gtk2, if the first character and second character are in ASCII range, we should not try to get another keyboard layout, I think...

(In reply to comment #53) > er, on gtk2, if the first character and second character are in ASCII range, we > should not try to get another keyboard layout, I think... > See https://bugzilla.mozilla.org/show_bug.cgi?id=399939#c16

minor updating: 1. checking whether the nsIDOMEvent is keypress event in nsMenuBarFrame::FindMenuWithShortcut and nsXBLWindowKeyHandler::WalkHandlersInternal. Because keydown/keyup event's GetCharCode outputs the very many warning during typing the text. 2. checking whether the first/second accessCharCodes are ASCII. Because on my environment, the previous patch sends non-related keyboard layout keys. I think that we should choose the preferred keyboard layout in bug 399939.

Comment on attachment 308019 [details] [diff] [review] Patch v5.1 (-u8pw) oops, this patch ignores the white space changes.

I want karlt to review this stuff. I will sr.

I think the real bug here is that (by default) Shift is used to indicate a document access key on Windows and Linux (bug 349716). This makes it impossible to distinguish which level to use in guessing which char code the user intends. I haven't read all the comments in bug 349716, but it sounds like Shift was introduced to distinguish from chrome access keys. The real solution IMO is to use something different to indicate a document access key. http://wiki.mozilla.org/AccessKeys_solution seems a good solution to me, but I'm not suggesting doing this for Gecko 1.9. (In reply to comment #47) > > It also seems to be that command keys and menu access keys should only be > > looking at charCode or keyCode. > > I don't think so. Because we cannot map chars to keycodes in XP level. In the situations where we can't tell whether the user intended the char code from level 0 or 1, then the best we can do AFAIK is try both. However, I don't want to introduce this problem in situations where currently we can tell which char code was selected. Currently we should be able to distinguish the intention of the user with default preference settings for accel keys (Ctrl on Windows and Linux), chrome/menu access keys (Alt on Windows/Linux), and content access keys on Mac. With many non-default preference settings, we should be able to distinguish all cases, and I think many people change their settings because they don't like the Alt-Shift combination: http://kb.mozillazine.org/About:config_entries#UI..2A > And if we check the charCode first, the testcase of this bug doesn't work > fine. I believe that we should check unshiftedCharCode first. If so, the > preferred access key doesn't depend on whether the key combination has shift > key. Personally I would expect the shifted code if I had the Shift key down but we could debate that until the cows come home. What I think is important is that we don't introduce a bug on Mac, just because we haven't solved the problem with default preferences on other platforms. > (Of course, the preferred access key depends on keyboard layout. But we > cannot fix it, probably.) Yes, that is a symptom of using Shift to indicate an access key.

Comment on attachment 308019 [details] [diff] [review] Patch v5.1 (-u8pw) To minimize the impact of the issue above, I suggest using an unshiftedCharCode field on nsKeyEvent, and only check this field if the access modifier mask contains Shift. I would only bother implementing this check in nsEventStateManager as this is the only place that needs to handle this with default preferences. And I wouldn't bother trying to provide multiple unshiftedCharCodes: changing the level could be quite confusing if changing the group also. accessCharCodes looks useful for providing Latin codes when in a non-Latin layout. It only needs to be an array (nsString) if we are going to provide codes from several layouts. However, as we are not going to be able to achieve correct precedence of several layouts, I suspect it will be best if we end up having only latinCharCode and localCharCode (from a layout supporting the language of the localization). latinCharCode could be used for accel keys, localCharCode for menu/chrome access keys, and perhaps both for document access keys. None of this paragraph is really this bug, so it may be easier to do this separately from the unshiftedCharCode. + PRBool ExecuteAccessKey(nsAString& aAccessKeys, PRBool aIsTrustedEvent); //--------------------------------------------- A blank line between these lines would be nice nsXBLWindowKeyHandler::WalkHandlersInternal(nsIDOMEvent* aEvent, + for (PRUint32 i = 0; i < accessKeys.Length() || i == 0; ++i) { Should this loop be aborted (after the inner loop) when currHandler->ExecuteHandler is called (or maybe if EventMatched succeeds), or is there a reason why this is different from nsXBLKeyEventHandler::HandleEvent? Do you know why nsXBLKeyEventHandler::HandleEvent doesn't return on successful return from handler->ExecuteHandler (but nsXBLWindowKeyHandler does)? nsMenuBarFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent) if (nsXULPopupManager::IsValidMenuItem(PresContext(), current, PR_FALSE)) { // Get the shortcut attribute. nsAutoString shortcutKey; current->GetAttr(kNameSpaceID_None, nsGkAtoms::accesskey, shortcutKey); if (!shortcutKey.IsEmpty()) { - // We've got something. - PRUnichar letter = PRUnichar(charCode); // throw away the high-zero-fill - if ( shortcutKey.Equals(Substring(&letter, &letter+1), - nsCaseInsensitiveStringComparator()) ) { - // We match! - return (currFrame->GetType() == nsGkAtoms::menuFrame) ? - static_cast<nsMenuFrame *>(currFrame) : nsnull; + PRInt32 index = accessKeys.Find(shortcutKey); Do we know for sure that there will only be one frame that IsValidMenuItem? If not, then having the accessKeys loop outside the frame loop would be better. + if (keys[i].level != 0 && keys[i].level != 1) continue; Do you know of cases where level is not an element of {0,1}? (I wouldn't expect any. AFAIK level only depends on Shift.) + GdkEventKey tmpEvent = *aEvent; + tmpEvent.keyval = GDK_a; + tmpEvent.group = keys[i].group; + tmpEvent.state = keys[i].level ? guint(GDK_SHIFT_MASK) : 0; + ch = nsConvertCharCodeToUnicode(&tmpEvent); + if (ch && ch < 0xFF) { Is this necessary? It looks like nsConvertCharCodeToUnicode will always produce ch = 'a'. + if (group != aEvent->group) { + event.accessCharCodes.Append(PRUnichar(ch)); This should only be added if it is a Latin char code (because that was the criteria used for selecting this group). Ideally it should only be added if the char code wasn't available in the selected layout. (By pressing a different key, the user has indicated that they did not want this code.) gdk_keymap_get_entries_for_keyval could be used for this. nsNativeKeyBindings::KeyPress probably should try the Latin code as well as char code too. I haven't looked at the windows or cocoa widget code. I'm not so familiar with that, but I can have a look when the other issues are resolved.

(In reply to comment #60) > + if (keys[i].level != 0 && keys[i].level != 1) > continue; > > Do you know of cases where level is not an element of {0,1}? > (I wouldn't expect any. AFAIK level only depends on Shift.) Found this in gdk_keymap_get_entries_for_keyval /* The "classic" non-XKB keymap has 2 levels per group */ key.group = i / 2; key.level = i % 2; The xkb path uses XkbKeyGroupsWidth which doesn't seem to have such a limit, so level can be > 1. Do you think we should consider layouts with lower case "a" on level 1 (instead of the normal 0)?

(In reply to comment #60) > (From update of attachment 308019 [details] [diff] [review]) > To minimize the impact of the issue above, I suggest using an > unshiftedCharCode field on nsKeyEvent, and only check this field if the access > modifier mask contains Shift. No, if we only send unshiftedCharCode, we cannot fix bug 401086. Ctrl++ is Ctrl+; on JIS keyboard. So, we need shiftedCharCode for the issue. And I think the similar problem is on some other keyboard layouts. And note that charCode should *not* have shiftedCharCode. Because we must not modify charCode even if the users pressed Ctrl/Alt/Meta keys. See bug 414130. > I would only bother implementing this check in > nsEventStateManager as this is the only place that needs to handle this with > default preferences. ESM is handling the accesskeys for contents. We need to check them also at accel handling and accesskeys handling for menus. > accessCharCodes looks useful for providing Latin codes when in a non-Latin > layout. It only needs to be an array (nsString) if we are going to provide > codes from several layouts. However, as we are not going to be able to > achieve correct precedence of several layouts, I suspect it will be best if we > end up having only latinCharCode and localCharCode (from a layout supporting > the language of the localization). latinCharCode could be used for accel > keys, localCharCode for menu/chrome access keys, and perhaps both for document > access keys. None of this paragraph is really this bug, so it may be easier > to do this separately from the unshiftedCharCode. No, I need two new fields for this bug. So, the array approach makes to simpler code. I only can use for loop for unshifted, shifted and charCode. And bug 399939 doesn't need to change the XP code by this approach. > nsXBLWindowKeyHandler::WalkHandlersInternal(nsIDOMEvent* aEvent, > > + for (PRUint32 i = 0; i < accessKeys.Length() || i == 0; ++i) { > > Should this loop be aborted (after the inner loop) when > currHandler->ExecuteHandler is called (or maybe if EventMatched succeeds), or > is there a reason why this is different from > nsXBLKeyEventHandler::HandleEvent? > > Do you know why nsXBLKeyEventHandler::HandleEvent doesn't return on successful > return from handler->ExecuteHandler (but nsXBLWindowKeyHandler does)? > > nsMenuBarFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent) I had same question. However, I think that we should not change current behavior... > + if (keys[i].level != 0 && keys[i].level != 1) > continue; > > Do you know of cases where level is not an element of {0,1}? > (I wouldn't expect any. AFAIK level only depends on Shift.) http://www-eleves-isia.cma.fr/documentation/GtkDoc/gdk/gdk-keyboard-handling.html#GDKKEYMAPKEY 0 is unshifed, 1 is shifted. that is not always so?? > I haven't looked at the windows or cocoa widget code. I'm not so familiar > with that, but I can have a look when the other issues are resolved. Don't worry. I'll request the reviews to Ere and Josh who are experts for the code. (In reply to comment #61) > Do you think we should consider layouts with lower case "a" on level 1 (instead > of the normal 0)? I'm not sure. However, I think we don't need to think so. Because the current code is using 0. But I cannot find the any bug reports for that. (In reply to comment #59) > > And if we check the charCode first, the testcase of this bug doesn't work > > fine. I believe that we should check unshiftedCharCode first. If so, the > > preferred access key doesn't depend on whether the key combination has shift > > key. > > Personally I would expect the shifted code if I had the Shift key down but we > could debate that until the cows come home. > > What I think is important is that we don't introduce a bug on Mac, just > because we haven't solved the problem with default preferences on other > platforms. By my order, the web designers don't need to think the platform differences and the pref settings for Gecko1.9. Therefore, I believe that we should always prefer the unshifted charCode. (If we check the shiftedCharCode first when the shift key is pressed, mac and others are not same behavior in default settings.)

(In reply to comment #62) > (In reply to comment #60) > > To minimize the impact of the issue above, I suggest using an > > unshiftedCharCode field on nsKeyEvent, and only check this field if the > > access modifier mask contains Shift. > > No, if we only send unshiftedCharCode, we cannot fix bug 401086. You can check charCode (or other codes) too. But don't use unshiftedCharCode unless the access modifier mask for the shortcut contains Shift. > Ctrl++ is Ctrl+; on JIS keyboard. No. Ctrl++ (with Shift) and Ctrl+; are distinct key combinations. > So, we need shiftedCharCode for the issue. And I think the similar problem > is on some other keyboard layouts. And note that charCode should *not* have > shiftedCharCode. Because we must not modify charCode even if the users > pressed Ctrl/Alt/Meta keys. See bug 414130. It sounds a good idea to not modify charCode. But isn't (or shouldn't) charCode normally the same as shiftedCharCode when Shift is down? Ctrl-Shift-2 on MS Persian keyboard layouts in bug 414130 is an exception, but is this common enough to warrant storing a separate code? > > I would only bother implementing this check in nsEventStateManager as this > > is the only place that needs to handle this with default preferences. > > ESM is handling the accesskeys for contents. We need to check them also at > accel handling and accesskeys handling for menus. On the keyboard layouts in bug 401086, it is possible to type + with Ctrl. The problem seems to be that the combination is not being detected. But that can and should be fixed without removing the distinction between Ctrl++ (with Shift) and Ctrl+; The distinction between shifted and unshifted shortcuts is used to distinguish between Ctrl-h History Sidebar and Ctrl-Shift-H Show All History, and was used for Ctrl-i Page Info and Ctrl-Shift-I Dom Inspector. This bug is different because it is not possible to type a 0 (for example) with Shift and Alt, on most keyboards. > > Do you know why nsXBLKeyEventHandler::HandleEvent doesn't return on successful > > return from handler->ExecuteHandler (but nsXBLWindowKeyHandler does)? > > > > nsMenuBarFrame::FindMenuWithShortcut(nsIDOMKeyEvent* aKeyEvent) > > I had same question. However, I think that we should not change current > behavior... Yes. If we have no reason to change, let's not change it. > > > + if (keys[i].level != 0 && keys[i].level != 1) > > continue; > 0 is unshifed, 1 is shifted. that is not always so?? AFAIK that is a safe assumption. AFAIK every key with more than one level has level 1 accessed through Shift. I assume level > 1 is accessed with "Alt Gr", although I haven't checked and there could be exceptions, but I don't think that's important. I'm just trying to understand why you are checking that the level is 0 or 1? > (In reply to comment #61) > > Do you think we should consider layouts with lower case "a" on level 1 (instead > > of the normal 0)? > > I'm not sure. However, I think we don't need to think so. Because the current > code is using 0. But I cannot find the any bug reports for that. A check for level == 0 only would consider only layouts with lower case "a" on level 0, which seems simpler and safer, but I don't know whether it makes any difference in practice. > (In reply to comment #59) > > What I think is important is that we don't introduce a bug on Mac, just > > because we haven't solved the problem with default preferences on other > > platforms. > > By my order, the web designers don't need to think the platform differences > and the pref settings for Gecko1.9. Therefore, I believe that we should > always prefer the unshifted charCode. (If we check the shiftedCharCode first > when the shift key is pressed, mac and others are not same behavior in > default settings.) I'm OK if you want to check unshifted first in situations where we can't distinguish what the user intended, because some key combination is going to be unavailable, and we cannot tell which is the best one to prefer for the situation. It is unfortunate that web designers need to aware of bugs in browsers, but I can't justify introducing more bugs just to make designers more aware. The designer may not even test with Gecko at all, and so we should do our best. Note that the Shift-Alt combination really seems to have been introduced to get document access keys out of the way of chrome access keys. Document access keys are often not very visible and can surprise a user when they override chrome access keys. The change was not made to make access keys more usable. People who frequently used document access keys did not like the change, but were told that they could restore the behavior through preferences, so many of these people will have changed their preferences. We should not extend this bug to make it impossible to fix through preferences.

(In reply to comment #63) > > So, we need shiftedCharCode for the issue. And I think the similar problem > > is on some other keyboard layouts. And note that charCode should *not* have > > shiftedCharCode. Because we must not modify charCode even if the users > > pressed Ctrl/Alt/Meta keys. See bug 414130. > > It sounds a good idea to not modify charCode. But isn't (or shouldn't) > charCode normally the same as shiftedCharCode when Shift is down? Yes. > Ctrl-Shift-2 on MS Persian keyboard layouts in bug 414130 is an exception, but > is this common enough to warrant storing a separate code? I think yes. If we modify the charCode for known keyboard layouts, there are the serious risk for some keyboard layout users. And we need to add the non-modified case codes for the exceptions on each platforms, it's too ugly. I think that if we don't modify the charCode, we can get the simple and non-risky code.

> (In reply to comment #62) > > (In reply to comment #60) > On the keyboard layouts in bug 401086, it is possible to type + with Ctrl. > The problem seems to be that the combination is not being detected. But that > can and should be fixed without removing the distinction between Ctrl++ (with > Shift) and Ctrl+; No, Ctrl+Shift+; on JIS keyboard layout and Ctrl++ should not be same. If I allow that, I need more risky changes in nsXBLPrototypeHandler. And it seems that it's difficult. > The distinction between shifted and unshifted shortcuts is used to distinguish > between Ctrl-h History Sidebar and Ctrl-Shift-H Show All History, and was used > for Ctrl-i Page Info and Ctrl-Shift-I Dom Inspector. The current patch checks strictly for the modifier key combination. so, this worry will be only occurred in your suggesting approach. Now and my patch, when modifier key combination is not matched, the key matching is ignored. However, if we ignore the shift key state of modifier key combination, we need to check whether the shifted acceleration is there. But that is harder than my approach. I think that this and bug 401086 are not different bug. Because shift key being needed for a non-alphabet character depends on keyboard layout. So, we should ignore the shift state for checking the such *key* being pressed. If we prefer the 'char' for accesskey/accelkey checking, that way is too difficult.

mmmmm, bug 422927 is a conflict Cmd+` (OS default, after handling our accelkeys) and Cmd+- (Of course, our accelkey)...

I still haven't seen a clear explanation of why this should block.

(In reply to comment #67) > I still haven't seen a clear explanation of why this should block. It fixes the most part of Bug 399939, which has blocking 1.9 flag. And since this bug is't fixed we can't do anything with bug 399939. Also it changes the code from Bug 69230, which is very important. So, rerequest blocking-1.9 flag.

O.K. I changed my mind. Karl, please review the concept of this patch and check the gtk2 part. (and also nsContentUtils::DOMEventToNativeKeyEvent) This patch is using two approaches: 1. access keys (web contents, menu items of menubar and xul widgets): Always ignoring the shift key state, and trying to find the target content with following order: 1.1 unshifted charcodes 1.2 shifted charcodes 1.3 charCode 2. accel keys depends on the shift key state. 2.1 When shift key is not pressed, trying to check as following order: 2.1.1 unshifted charcodes 2.1.2 charCode So, shifted charcodes are not used. 2.2 When shift key is pressed, trying to check as following order: 2.2.1 shifted charcodes 2.2.2 unshifted charcodes 2.2.3 charCode 2.2.4 shifted charcodes without shift key checking. I.e., Ctrl+; on JIS keyboard doesn't much to Ctrl++ (2.1). But Ctrl+Shift+; on JIS keyboard matches to Ctrl++ (Ctrl+Shift++ failed (2.2.1) -> Ctrl+Shift+; failed (2.2.2) -> Ctrl+Shift+(?) failed (2.2.3) Ctrl++ matched (2.2.4)). However, Ctrl+Shift+C doesn't match to Ctrl+C. Because at 2.2.4, ignoring the characters that are tried in 2.2.2 (unshifted charcodes). By this approach, we can fix many bugs on many keyboard layouts.

(In reply to comment #68) > (In reply to comment #67) > > I still haven't seen a clear explanation of why this should block. > > It fixes the most part of Bug 399939, which has blocking 1.9 flag. And since > this bug is't fixed we can't do anything with bug 399939. > Also it changes the code from Bug 69230, which is very important. Ok, thanks. This bug has an awful lot of comments to try to read for quick triage.

Comment on attachment 310126 [details] [diff] [review] Patch v6.0 I like the concept of adding additional information to the nsKeyEvent, and this enables us to be more versatile in how we handle the event (and hopefully fix two bugs at once rather than oscillating between which gets fixed or broken). But I think we need to be very careful about how the additional information is used. When more key/modifier combinations are accepted in one place, it increases the likelihood of accepting an unintended shortcut and masking the intended shortcut (which would be detected somewhere else). A situation where the wrong shortcut can be accidentally activated is worse than a situation where it is difficult to access a shortcut. I think we should only accept additional combinations when we have a good reason to do so. (In reply to comment #69) Thanks for this overview. It is helpful. > This patch is using two approaches: > > 1. access keys (web contents, menu items of menubar and xul widgets): > > Always ignoring the shift key state, and trying to find the target content > with following order: > > 1.1 unshifted charcodes > 1.2 shifted charcodes > 1.3 charCode I still think charCodes from different shift states should only be checked when it is impossible for the user to type that character at the same time has holding down the necessary modifiers (and even that is an unfortunate situation). But I'll say more about keys and characters. (Possibly a localCharCode from another layout and maybe an access-key-modifier-free char code could also be checked. These are not necessary for fixing this bug, but something maybe necessary to avoid regressions when fixing bug 414130.) > 2. accel keys depends on the shift key state. > > 2.1 When shift key is not pressed, trying to check as following order: > > 2.1.1 unshifted charcodes > 2.1.2 charCode If the layout requires Alt Gr to access the charCode (e.g. ] or > or ~ on a Canadian Multilingual layout), then this would pick up 0 or . instead. Is it necessary to check unshifted charCodes? Is that only for picking up Latin codes? > So, shifted charcodes are not used. Good. (I haven't yet studied the GTK code. Let's decide what the preferred behavior is, then look at the details, unless you want to put together a different patch that just addresses bug 414130.)

(In reply to comment #65) > > (In reply to comment #62) > > > (In reply to comment #60) > > On the keyboard layouts in bug 401086, it is possible to type + with Ctrl. > > The problem seems to be that the combination is not being detected. But > > that can and should be fixed without removing the distinction between > > Ctrl++ (with Shift) and Ctrl+; > > No, Ctrl+Shift+; on JIS keyboard layout and Ctrl++ should not be same. If I > allow that, I need more risky changes in nsXBLPrototypeHandler. And it seems > that it's difficult. They used to be the same. That is the regression that caused bug 401086 and others. > I think that this and bug 401086 are not different bug. Because shift key > being needed for a non-alphabet character depends on keyboard layout. So, we > should ignore the shift state for checking the such *key* being pressed. If > we prefer the 'char' for accesskey/accelkey checking, that way is too > difficult. nsKeyEvent and nsIDOMKeyEvent make a distinction between a key code which represents a physical key and char code which represents a character. IMO the same distinction should apply in the XUL "key" element between the "keycode" and "key" attributes (even though the distinction is not so clear from the attribute names). However, I understand that we disagree on that. I think that this is a decision that the UI team should make so I'll ask for their input.

Mike or Mike: Should the "key" attribute of the "key" element be interpreted as a physical key or as a character code? e.g. if '4' and '$' are on the same key, would we like to have a distinction between '4' and '$'? The same questions also apply to the "accesskey" attribute on the "menu" element and other chrome access keys. Should these be treated any differently? Could an "accesskey" attribute on a "menu" or other chrome be a digit or punctuation?

(In reply to comment #71) > (From update of attachment 310126 [details] [diff] [review]) > > This patch is using two approaches: > > > > 1. access keys (web contents, menu items of menubar and xul widgets): > > > > Always ignoring the shift key state, and trying to find the target content > > with following order: > > > > 1.1 unshifted charcodes > > 1.2 shifted charcodes > > 1.3 charCode > > I still think charCodes from different shift states should only be checked > when it is impossible for the user to type that character at the same time has > holding down the necessary modifiers (and even that is an unfortunate > situation). But I'll say more about keys and characters. I still don't think so. Alt+Shift+foo combination can be grouped to: 1. (Alt+Shift) + (foo) 2. (Alt) + (Shift+foo) I think we should use group 1, first. Because only foo key is pressed by the user's will. Next, we should use group 2. Because when we cannot find the target, we fail to guess the user's will. > > 2. accel keys depends on the shift key state. > > > > 2.1 When shift key is not pressed, trying to check as following order: > > > > 2.1.1 unshifted charcodes > > 2.1.2 charCode > > If the layout requires Alt Gr to access the charCode (e.g. ] or > or ~ on a > Canadian Multilingual layout), then this would pick up 0 or . instead. Do you think charCode should be first priority? But I'm not sure these non-letters are preferred by the authors of web contents rather than letters. > Is it necessary to check unshifted charCodes? Yes. Shift key is pressed by our spec. And the our behavior (in other words, our limitation) should depend only on keyboard layout. Not modifier mask. > Is that only for picking up Latin codes? I'm not sure what you think. It means less than 0xFF characters? If so, it's no good for some language people (e.g., Russian). Karlt: I said that "I changed my mind". I'm not thinking we prefer the key. Because the current (your) idea is strict, i.e., that is low risky of executed the wrong target. If shift is not pressed, we can ignore the Shift key. (accesskey is not so, because Alt+Shift is used for accesskey of web contents.) However, if shift key is pressed, it's pretty complex. We can have some senarios. 1. Shift key is a part of modifier ( (Ctrl+Shift) + foo ) 2. Shift key is used for shifting a key ( (Ctrl) + (Shift+foo) ) We should prefer group 1. But if that is failed, we can think that group 2 is true.

(In reply to comment #74) > (In reply to comment #71) > > > 2. accel keys depends on the shift key state. > > > > > > 2.1 When shift key is not pressed, trying to check as following order: > > > > > > 2.1.1 unshifted charcodes > > > 2.1.2 charCode > > > > If the layout requires Alt Gr to access the charCode (e.g. ] or > or ~ on a > > Canadian Multilingual layout), then this would pick up 0 or . instead. > > Do you think charCode should be first priority? But I'm not sure these > non-letters are preferred by the authors of web contents rather than letters. Oops, you were taking about accel keys. Sorry. We might be able to think that when Ctrl and Alt keys are pressed at one time, the charCode is an user's will. However, both modifier keys are should not be ignored on mask checking... So, I have no idea now...

(In reply to comment #75) > (In reply to comment #74) > > (In reply to comment #71) > > > > 2. accel keys depends on the shift key state. > > > > > > > > 2.1 When shift key is not pressed, trying to check as following order: > > > > > > > > 2.1.1 unshifted charcodes > > > > 2.1.2 charCode > > > > > > If the layout requires Alt Gr to access the charCode (e.g. ] or > or ~ on a > > > Canadian Multilingual layout), then this would pick up 0 or . instead. > > > > Do you think charCode should be first priority? But I'm not sure these > > non-letters are preferred by the authors of web contents rather than letters. > > Oops, you were taking about accel keys. Sorry. > > We might be able to think that when Ctrl and Alt keys are pressed at one time, > the charCode is an user's will. However, both modifier keys are should not be > ignored on mask checking... So, I have no idea now... Wait, AltGr is same as Alt+Ctrl. So, the users cannot type AltGr needed characters with modifier keys. (Ctrl + AltGr + foo combination cannot be there...) Such bug should not be fixed by this bug. I think that we need to change the accel keys for their keyboard layout users...

(In reply to comment #71) > (I haven't yet studied the GTK code. Let's decide what the preferred behavior > is, then look at the details, unless you want to put together a different > patch that just addresses bug 414130.) I confirmed that bug 414130 is also fixed by my latest patch (I tested on Windows).

(In reply to comment #76) [...] > Wait, AltGr is same as Alt+Ctrl. So, the users cannot type AltGr needed > characters with modifier keys. (Ctrl + AltGr + foo combination cannot be > there...) Such bug should not be fixed by this bug. I think that we need to > change the accel keys for their keyboard layout users... > I think this is platform-dependent. In particular, on my X11 system with fr_BE keyboard, Ctrl+AltGr+$ is Ctrl+] (and AltGr+$ is ]) but Ctrl+$ is something else (Ctrl+$, I think, in programs for which that has a meaning, otherwise just $). So obviously _this_ keyboard interface makes a difference between AltGr and Ctrl+AltGr in at least _some_ cases. (On this keyboard, AltGr is not obtained by hitting Alt and Ctrl together, it is a distinct key, on the right side of the space bar, symmetrical of Alt which is on the left side.)

(In reply to comment #74) > Karlt: > > I said that "I changed my mind". I'm not thinking we prefer the key. Because > the current (your) idea is strict, i.e., that is low risky of executed the > wrong target. If shift is not pressed, we can ignore the Shift key. Sorry, I hadn't understood what exactly you'd changed you mind on, because I still saw different characters from different Shift states on the same key being tested, but maybe I understand what you are saying here: > (accesskey is not so, because Alt+Shift is used for accesskey of web > contents.) If you are saying that we can't ignore the Shift with Alt because we need to distinguish from Alt+Shift, then let me think about that a bit. > However, > if shift key is pressed, it's pretty complex. We can have some senarios. > > 1. Shift key is a part of modifier ( (Ctrl+Shift) + foo ) > 2. Shift key is used for shifting a key ( (Ctrl) + (Shift+foo) ) > > We should prefer group 1. But if that is failed, we can think that group 2 is > true. Do you still think this for accel keys? (I can see the logic for access keys because Shift+Alt is a distinct required modifier with default preferences.) IMO shift should only be a required modifier on accel keys if it is a modifier on a letter with upper and lower case forms. And a case-insensitive comparison (or similar) would be enough to handle this. (In reply to comment #76) > (In reply to comment #75) > > (In reply to comment #74) > > > (In reply to comment #71) > > > > > 2. accel keys depends on the shift key state. > > > > > > > > > > 2.1 When shift key is not pressed, trying to check as following order: > > > > > > > > > > 2.1.1 unshifted charcodes > > > > > 2.1.2 charCode > > > > > > > > If the layout requires Alt Gr to access the charCode (e.g. ] or > or ~ on a > > > > Canadian Multilingual layout), then this would pick up 0 or . instead. > > > > > > Do you think charCode should be first priority? Yes. And I think we should be wary about also checking a code from a non-AltGr level when AltGr is down. > > > But I'm not sure these non-letters are preferred by the authors of web > > > contents rather than letters. Hopefully not. But if the user is pressing AltGr, I think we can be safe in assuming that they intend the code produced by AltGr. > > We might be able to think that when Ctrl and Alt keys are pressed at one > > time, the charCode is an user's will. However, both modifier keys are > > should not be ignored on mask checking... So, I have no idea now... > > Wait, AltGr is same as Alt+Ctrl. So, the users cannot type AltGr needed > characters with modifier keys. (Ctrl + AltGr + foo combination cannot be > there...) Such bug should not be fixed by this bug. I think that we need to > change the accel keys for their keyboard layout users... I understood that AltGr produced the same character as Alt+Ctrl on Windows but does using AltGr really make IS_VK_DOWN(NS_VK_ALT) and IS_VK_DOWN(NS_VK_CONTROL) true? Even if so, I think that we should fix this bug in a way that doesn't prevent users accessing the accel characters through AltGr on platforms that have a separate AltGr modifier.

(In reply to comment #77) > (In reply to comment #71) > > (I haven't yet studied the GTK code. Let's decide what the preferred > > behavior is, then look at the details, unless you want to put together a > > different patch that just addresses bug 414130.) > > I confirmed that bug 414130 is also fixed by my latest patch (I tested on > Windows). Thanks, I'm not denying that your patch fixes bug 414130. I'm just making a suggestion that perhaps a subset of this patch could fix bug 414130 without attempting to fix this bug. I'm not demanding that you do that, but I wondered whether that might be a way to get some of this landed faster and get some thorough testing by users sooner.

(In reply to comment #68) > Also it changes the code from Bug 69230, which is very important. Do you mean that this reverts the fix to bug 69230? We shouldn't do that.

Regarding access keys (Alt on Windows/Unix): I did some looking to see what non-letter access keys were used in chrome using grep -hw accesskey **/en-US/**/*.dtd | grep ENTITY | sed 's/.*accesskey *"\(.\)".*/\1/' | sort -u on a tree that contained browser and (few month old) suite files. The only non-letter access keys that I found were digits: browser/locales/en-US/chrome/browser/migration/migration.dtd: <!ENTITY importFromNetscape4.accesskey "4"> browser/locales/en-US/chrome/browser/preferences/advanced.dtd: <!ENTITY useSSL3.accesskey "3"> <!ENTITY useTLS1.accesskey "1"> editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd: <!ENTITY heading1.accesskey "1"> <!ENTITY heading2.accesskey "2"> <!ENTITY heading3.accesskey "3"> <!ENTITY heading4.accesskey "4"> <!ENTITY heading5.accesskey "5"> <!ENTITY heading6.accesskey "6"> editor/ui/locales/en-US/chrome/dialogs/EditorImageMap.dtd: <!ENTITY zoomone.accesskey "1"> <!ENTITY zoomtwo.accesskey "2"> <!ENTITY zoomthree.accesskey "4"> security/manager/locales/en-US/chrome/pippki/pref-ssl.dtd: <!ENTITY enable.ssl3.accesskey "3"> suite/locales/en-US/chrome/mailnews/addressbook/abCardOverlay.dtd: <!ENTITY Custom1.accesskey "1"> <!ENTITY Custom2.accesskey "2"> <!ENTITY Custom3.accesskey "3"> <!ENTITY Custom4.accesskey "4"> I haven't checked other localizations or other apps, but it looks like we are unlikely to be in a situation where two different chrome access keys are on the same key. That means that the suggested approach of checking shifted and unshifted characters from the same key for chrome accesskeys is unlikely to produce major problems for chrome. It may seem a little unnatural to need to use Alt without Shift to access a digit on a French keyboard, but currently digit access keys don't seem to be accessible at all on such keyboards, so the shifted/unshifted character approach seems to be a step forward. The other thing that is significant about the non-letter chrome access keys currently used is that none of them look like they are active at the same time as content. The reason for requiring the Shift modifier for content access keys was to distinguish from chrome access keys, but, if there are no non-letter chrome access keys active at the same time as content, then non-letter content access keys don't need to be distinguished and thus the Shift state could be used to distinguish between the different characters on the same key. So AFAICT the ideal solution (from a UI point of view) for keyboards with bicameral scripts would be to treat letter characters without Shift only as chrome access keys, but accept letter characters with Shift and non-letter characters with or without Shift as content access keys. This wouldn't work so well for keyboards with unicase scripts. So some means of detecting these keyboards would be needed, and a preference might be the best we could do here. Currently it looks like most letter content access keys for unicase scripts are not accessible at all, but the shifted/unshifted character approach would make these accessible. Similarly non-letter characters from Shifted levels (on any keyboard) are not available at all for content access keys, so the shifted/unshifted approach would be a step forward here as it would at least make these access keys available when the un-Shifted character is not in use. So in conclusion, I'm happy that the shifted/unshifted character approach for access keys is a step forward. I'm still not happy about shifted and unshifted codes being preferred to charCode when charCode comes from a level > 1 (AltGr). I suspect the best thing to do when AltGr is down might be to just not fill in the unshifted and shifted codes. + nsString GetAccessKeyCandidates(PRBool aIgnoreShiftKey) + { + nsAutoString codes; + if (aIgnoreShiftKey) { + codes = unshiftedCharCodes; + codes += shiftedCharCodes; I think we want to prefer (shifted) codes from the preferred layout over unshifted codes from other layouts. PRUint32 charCode; + // OS translated Unicode chars which are used for accesskey and accelkey + // handling. The handlers will try from first character to last character. + // XXX this should be array of PRUint32. + nsString shiftedCharCodes; + nsString unshiftedCharCodes; What is the motivation for making unshifted and shiftedCharCodes an nsString rather than a nsTArray<PRUint32>? Wouldn't PRUint32 be more consistent with charCode and isn't that where we'd eventually like to be?

Regarding accel keys (Ctrl on Windows/Unix): Comment 79 and bug 401086 comment 32 still apply but I have another question: @@ -3970,29 +3970,60 @@ nsContentUtils::DOMEventToNativeKeyEvent + aNativeEvent->nativeEvent = GetNativeEvent(aDOMEvent); + if (aGetCharCode) { - keyEvent->GetCharCode(&aNativeEvent->charCode); + nsKeyEvent* nativeKeyEvent = + static_cast<nsKeyEvent*>(aNativeEvent->nativeEvent); + nsAutoString chars(nativeKeyEvent->GetAccessKeyCandidates(PR_FALSE)); + aNativeEvent->charCode = chars.IsEmpty() ? 0 : chars.CharAt(0); Can we be sure that GetNativeEvent doesn't return NULL here? If so, why? It feels to me like this logic should be done in nsNativeKeyBindings, as that is the place that knows best which code it wants.

(In reply to comment #79) > > (accesskey is not so, because Alt+Shift is used for accesskey of web > > contents.) > > If you are saying that we can't ignore the Shift with Alt because we need to > distinguish from Alt+Shift, then let me think about that a bit. Yes. We *should* ignore the shift state for access keys. Shift key is needed for to distinguish the keypress is for chrome or web contents. So, the shift key already has the special meaning. If we don't ignore the shift key state, the users cannot use shifted characters for chrome access keys, and also they cannot use unshifted characters for web content access keys. > > However, > > if shift key is pressed, it's pretty complex. We can have some senarios. > > > > 1. Shift key is a part of modifier ( (Ctrl+Shift) + foo ) > > 2. Shift key is used for shifting a key ( (Ctrl) + (Shift+foo) ) > > > > We should prefer group 1. But if that is failed, we can think that group 2 is > > true. > > Do you still think this for accel keys? > (I can see the logic for access keys because Shift+Alt is a distinct required > modifier with default preferences.) Yes. If there are both Ctrl+Shift+; and Ctrl++, they are same key combination on JIS keyboard layout. We cannot fix the conflict issue by this bug. However, I think we should prefer the Ctrl+Shift+;, first. And Ctrl+(Shift+;) should be retried next. > (In reply to comment #76) > > (In reply to comment #75) > > > (In reply to comment #74) > > > > (In reply to comment #71) > > > > > > 2. accel keys depends on the shift key state. > > > > > > > > > > > > 2.1 When shift key is not pressed, trying to check as following order: > > > > > > > > > > > > 2.1.1 unshifted charcodes > > > > > > 2.1.2 charCode > > > > > > > > > > If the layout requires Alt Gr to access the charCode (e.g. ] or > or ~ on a > > > > > Canadian Multilingual layout), then this would pick up 0 or . instead. > > > > > > > > Do you think charCode should be first priority? > > Yes. > And I think we should be wary about also checking a code from a non-AltGr > level when AltGr is down. > > > > > But I'm not sure these non-letters are preferred by the authors of web > > > > contents rather than letters. > > Hopefully not. But if the user is pressing AltGr, I think we can be safe in > assuming that they intend the code produced by AltGr. > > > > We might be able to think that when Ctrl and Alt keys are pressed at one > > > time, the charCode is an user's will. However, both modifier keys are > > > should not be ignored on mask checking... So, I have no idea now... > > > > Wait, AltGr is same as Alt+Ctrl. So, the users cannot type AltGr needed > > characters with modifier keys. (Ctrl + AltGr + foo combination cannot be > > there...) Such bug should not be fixed by this bug. I think that we need to > > change the accel keys for their keyboard layout users... > > I understood that AltGr produced the same character as Alt+Ctrl on Windows but > does using AltGr really make IS_VK_DOWN(NS_VK_ALT) and > IS_VK_DOWN(NS_VK_CONTROL) true? maybe, but I'm not sure. http://mxr.mozilla.org/mozilla/source/widget/src/windows/nsKeyboardLayout.h#51 > Even if so, I think that we should fix this bug in a way that doesn't prevent > users accessing the accel characters through AltGr on platforms that have a > separate AltGr modifier. I'm not sure the AltGr cases. Because JIS keyboard layout doesn't have AltGr. So, I don't have the feeling of AltGr key using, I'm not sure what behavior is natural for the users.

(In reply to comment #84) > (In reply to comment #79) > > If you are saying that we can't ignore the Shift with Alt because we need to > > distinguish from Alt+Shift, then let me think about that a bit. > > Yes. We *should* ignore the shift state for access keys. Shift key is needed > for to distinguish the keypress is for chrome or web contents. So, the shift > key already has the special meaning. If we don't ignore the shift key state, > the users cannot use shifted characters for chrome access keys, and also they > cannot use unshifted characters for web content access keys. We are saying the same thing here in different ways :) We (unfortunately) need to ignore the Shift state when choosing a character, because we can't ignore the Shift state when checking whether the modifier sets match. > > > > However, > > > if shift key is pressed, it's pretty complex. We can have some senarios. > > > > > > 1. Shift key is a part of modifier ( (Ctrl+Shift) + foo ) > > > 2. Shift key is used for shifting a key ( (Ctrl) + (Shift+foo) ) > > > > > > We should prefer group 1. But if that is failed, we can think that group > > > 2 is true. > > > > Do you still think this for accel keys? > > Yes. If there are both Ctrl+Shift+; and Ctrl++, they are same key combination > on JIS keyboard layout. We cannot fix the conflict issue by this bug. However, > I think we should prefer the Ctrl+Shift+;, first. And Ctrl+(Shift+;) should be > retried next. We see this differently though. Supporting key=";" and modifiers="shift,accel" would be a new feature IMO, as AFAICT this has never been supported in the past, and I'm not convinced that it would be a useful feature (bug 401086 comment 32). [This proposed implementation also (as you indicated IIUC) is different to what I understand is the intended meaning of modifiers="shift any" (bug 401086 comment 29).] If you really think that this would be a useful feature, then please submit another bug, give an example of how it should be used, and explain how you believe "shift any" should be interpreted. The bug reported here can be fixed without this change.

Comment on attachment 310126 [details] [diff] [review] Patch v6.0 + nsString GetAccessKeyCandidates(PRBool aIgnoreShiftKey) How about GetShortcutCharCandidates so as not to imply that this is only used for access keys and to make it clear that characters rather than keys are returned? Regarding the GTK code: I suggest checking other layouts only when level is 0 or 1 (not AltGr). These comments from Comment 60 haven't been addressed: + GdkEventKey tmpEvent = *aEvent; + tmpEvent.keyval = GDK_a; + tmpEvent.group = keys[i].group; + tmpEvent.state = keys[i].level ? guint(GDK_SHIFT_MASK) : 0; + ch = nsConvertCharCodeToUnicode(&tmpEvent); + if (ch && ch < 0xFF) { Is this necessary? It looks like nsConvertCharCodeToUnicode will always produce ch = 'a'. + if (group != aEvent->group) { + event.accessCharCodes.Append(PRUnichar(ch)); This should only be added if it is a Latin char code (because that was the criteria used for selecting this group). Ideally it should only be added if the char code wasn't available in the selected layout. (By pressing a different key, the user has indicated that they did not want this code.) gdk_keymap_get_entries_for_keyval could be used for this.

(In reply to comment #86) > (From update of attachment 310126 [details] [diff] [review]) > + GdkEventKey tmpEvent = *aEvent; > + tmpEvent.keyval = GDK_a; > + tmpEvent.group = keys[i].group; > + tmpEvent.state = keys[i].level ? guint(GDK_SHIFT_MASK) : > 0; > + ch = nsConvertCharCodeToUnicode(&tmpEvent); > + if (ch && ch < 0xFF) { > > Is this necessary? It looks like nsConvertCharCodeToUnicode will > always produce ch = 'a'. Why? ch looks like returns non-ASCII chars with some keyboard layouts? E.g., Russian keyboard layout returns a Cyrillic character, right? > + if (group != aEvent->group) { > > + event.accessCharCodes.Append(PRUnichar(ch)); > > This should only be added if it is a Latin char code (because that was the > criteria used for selecting this group). ok. > Ideally it should only be added if the char code wasn't available in the > selected layout. (By pressing a different key, the user has indicated that > they did not want this code.) gdk_keymap_get_entries_for_keyval could be used > for this. ??

(In reply to comment #85) > (In reply to comment #84) > > > > However, > > > > if shift key is pressed, it's pretty complex. We can have some senarios. > > > > > > > > 1. Shift key is a part of modifier ( (Ctrl+Shift) + foo ) > > > > 2. Shift key is used for shifting a key ( (Ctrl) + (Shift+foo) ) > > > > > > > > We should prefer group 1. But if that is failed, we can think that group > > > > 2 is true. > > > > > > Do you still think this for accel keys? > > > > Yes. If there are both Ctrl+Shift+; and Ctrl++, they are same key combination > > on JIS keyboard layout. We cannot fix the conflict issue by this bug. However, > > I think we should prefer the Ctrl+Shift+;, first. And Ctrl+(Shift+;) should be > > retried next. > > We see this differently though. > > Supporting key=";" and modifiers="shift,accel" would be a new feature IMO, as > AFAICT this has never been supported in the past, and I'm not convinced that it > would be a useful feature (bug 401086 comment 32). > [This proposed implementation also (as you indicated IIUC) is different to what > I understand is the intended meaning of modifiers="shift any" (bug 401086 > comment 29).] ok. maybe, I understand.

(In reply to comment #87) > (In reply to comment #86) > > (From update of attachment 310126 [details] [diff] [review] [details]) > > + GdkEventKey tmpEvent = *aEvent; > > + tmpEvent.keyval = GDK_a; > > + tmpEvent.group = keys[i].group; > > + tmpEvent.state = keys[i].level ? guint(GDK_SHIFT_MASK) : > > 0; > > + ch = nsConvertCharCodeToUnicode(&tmpEvent); > > + if (ch && ch < 0xFF) { > > > > Is this necessary? It looks like nsConvertCharCodeToUnicode will > > always produce ch = 'a'. > > Why? ch looks like returns non-ASCII chars with some keyboard layouts? E.g., > Russian keyboard layout returns a Cyrillic character, right? keyval = GDK_a means that the hardware-key/group/level combination produces the character U+0061 LATIN SMALL LETTER A. gdk-keymap-get-entries-for-keycode has already found the keys to produce Latin "a". See also: http://library.gnome.org/devel/gdk/stable/gdk-Keyboard-Handling.html#gdk-keymap-get-entries-for-keycode The code is also making an assumption that the order of the keys is such that the first found is the right one. Do you have any reason to believe this? I think it would be safest to choose the lowest group from the keys. > > > + if (group != aEvent->group) { > > > > + event.accessCharCodes.Append(PRUnichar(ch)); > > > > This should only be added if it is a Latin char code (because that was the > > criteria used for selecting this group). > > ok. > > > Ideally it should only be added if the char code wasn't available in the > > selected layout. (By pressing a different key, the user has indicated that > > they did not want this code.) gdk_keymap_get_entries_for_keyval could be used > > for this. > > ?? This may not be important. Hopefully the !isASCII and a IsBasicLatinLetterOrNumeral test for [a-zA-Z0-9] on the characters from the Latin group will be enough to make this unnecessary. We don't want to pick up punctuation from another group (unless we check that the character could not have been produced with the current group). Actually the isASCII name is probably not quite accurate as ASCII is <= 0x7F. But <= 0xFF is what we want as that helps with ö U+00F6 LATIN SMALL LETTER O WITH DIAERESIS on the Hungarian keyboard: http://en.wikipedia.org/wiki/Keyboard_layout#Hungary isLatin is probably a better name for isASCII.

(In reply to comment #89) > gdk-keymap-get-entries-for-keycode has already found the keys to produce Latin > "a". I mean gdk_keymap_get_entries_for_keyval.

(In reply to comment #89) > keyval = GDK_a means that the hardware-key/group/level combination produces > the character U+0061 LATIN SMALL LETTER A. > gdk-keymap-get-entries-for-keycode has already found the keys to produce Latin > "a". There is an example in the gtk2/nsWindow.cpp (for But 699). > isLatin is probably a better name for isASCII. There is already such routine in the gtk2/nsWindow.cpp.

Sorry for the delay. This patch changes: * nsKeyEvent::GetAccessCharCandidates and nsKeyEvent::GetShortcutCharCandidates are separated. * nsKeyEvent::GetAlternateShortcutCharCandidates is appended for retry without shift key state on accel key handling. * shiftedCharCodes and unshiftedCharCodes are nsTArray<PRUint32> now. * nsKeyEvent::GetShortcutCharCandidates doesn't return unshiftedCharCodes if shift key is pressed. * nsKeyEvent::GetAlternateShortcutCharCandidates doesn't return the same chars (case-insensitive) with unshiftedCharCodes. * fixing the bottom of comment 82, comment 83 and comment 86 issues.

* And charCode is most preferred now.

Oops, sorry the previous file is old version, this is latest. (only changes windows part.)

Comment on attachment 312957 [details] [diff] [review] Patch v7.0 @@ -3968,29 +3968,64 @@ nsContentUtils::DOMEventToNativeKeyEvent if (aGetCharCode) { - keyEvent->GetCharCode(&aNativeEvent->charCode); + nsKeyEvent* nativeKeyEvent = + static_cast<nsKeyEvent*>(aNativeEvent->nativeEvent); + if (nativeKeyEvent) { + nsAutoTArray<PRUint32, 10> chars; + nativeKeyEvent->GetShortcutCharCandidates(chars); + aNativeEvent->charCode = chars.IsEmpty() ? 0 : chars[0]; + } else { + keyEvent->GetCharCode(&aNativeEvent->charCode); + } } else { aNativeEvent->charCode = 0; } Is this still required? It looks like chars[0] from GetShortcutCharCandidates and keyEvent->GetCharCode should provide the same character? +nsContentUtils::GetAccelKeyCandidates(nsIDOMEvent* aDOMEvent, + nsKeyEvent** aNativeKeyEvent, + nsTArray<PRUint32>& aCandidates, + PRBool& aIsShiftDown) +{ + NS_ASSERTION(aNativeKeyEvent, "aNativeKeyEvent is null"); NS_PRECONDITION is better here IIUC. http://developer.mozilla.org/en/docs/NS_PRECONDITION + nsAutoString eventType; + aDOMEvent->GetType(eventType); + if (!eventType.EqualsLiteral("keypress")) + return PR_FALSE; + nsCOMPtr<nsIDOMKeyEvent> key(do_QueryInterface(aDOMEvent)); I wonder whether testing for keypress is necessary? Would it matter if only a zero character was returned? Would just checking "key" be sufficient? + PRBool ExecuteAccessKey(nsTArray<PRUint32>& aAccessCharCodes, + PRBool aIsTrustedEvent); //--------------------------------------------- A blank line before the // would be nice. +nsEventStateManager::ExecuteAccessKey(nsTArray<PRUint32>& aAccessCharCodes, - nsAutoString accKey(aEvent->charCode); + accessKey = ToLowerCase(PRUnichar(aAccessCharCodes[i])); Hmm. I see the UTF-32/16 conversion was already a truncation but some sort of IS_IN_BMP() check here would be nice. Case already seems to handled in IsAccessKeyTarget and GetAccessCharCandidates so I don't think ToLowerCase should be necessary here. nsXBLKeyEventHandler::HandleEvent(nsIDOMEvent* aEvent) + PRBool isKeyPressEvent = + nsContentUtils::GetAccelKeyCandidates(aEvent, &nativeKeyEvent, + accessKeys, isShift); + + for (PRUint32 i = 0; i < accessKeys.Length() || i == 0; ++i) { + PRUnichar ch = accessKeys.IsEmpty() ? 0 : accessKeys[i]; + if (ExecuteMatchedHandlers(key, ch, PR_FALSE)) + return NS_OK; } + if (!isKeyPressEvent || !nativeKeyEvent || !isShift) + return NS_OK; + // If shift key is being pressed, we should retry without shift key state. + nsAutoTArray<PRUint32, 10> altAccessKeys; + nativeKeyEvent->GetAlternateShortcutCharCandidates(altAccessKeys); + for (PRUint32 i = 0; i < altAccessKeys.Length(); ++i) { + if (ExecuteMatchedHandlers(key, altAccessKeys[i], PR_TRUE)) + return NS_OK; How about this for just one loop (and see bug 401086 comment 55) nsAutoTArray<PRBool,10> isShiftConsumed; // (maybe PRPackedBool?) nsContentUtils::GetAccelKeyCandidates(aEvent, accessKeys, isShiftConsumed); for (PRUint32 i = 0; i < accessKeys.Length(); ++i) { if (ExecuteMatchedHandlers(key, accessKeys[i], isShiftConsumed[i])) return NS_OK; } with accessKeys[0] == 0 and isShifConsumed[0] == false when there is a keyCode not a charCode? +#include "nsUnicharUtils.h" I couldn't see this used, but maybe I missed something? - PRBool KeyEventMatched(nsIDOMKeyEvent* aKeyEvent); + // 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); [I thought KeyEventMatched(PRUint32 aKeyCode, PRUint32 aCharCode, PRUint32 aModfiers) would be nice but making aModifiers from the nsIDOMEvent is a bit awkward.] +#include "nsIPrivateDOMEvent.h" + nsCOMPtr<nsIPrivateDOMEvent> privKeyEvent(do_QueryInterface(aKeyEvent)); Not used. @@ -224,24 +225,32 @@ nsMenuBarListener::KeyPress(nsIDOMEvent* + PRBool hasAccessKeyCandidates = !!charCode; Is !! a common convert-to-boolean operator? I'm more comfortable with "charCode != 0". + // return the lower cased charCode candidates for access keys. + void GetAccessCharCandidates(nsTArray<PRUint32>& aCharCandidates) + { + NS_ASSERTION(aCharCandidates.IsEmpty(), "aCharCandidates must be empty"); NS_PRECONDITION + if (charCode) + aCharCandidates.AppendElement(charCode); There is no case conversion here but I guess the lower case character will be added below. Consider whether looping over aCharCandidates at the end to do the conversion may be better. + for (PRUint32 i = 0; i < unshiftedCharCodes.Length(); ++i) { + PRUint32 ch = unshiftedCharCodes[i]; + if (ch >= 'A' && ch <= 'Z') + ch += 0x20; Case in non-Latin bicameral scripts should be treated similarly. The following should be good enough: if(IS_IN_BMP(ch)) code = ToLowerCase(PRUnichar(code)) + void GetAlternateShortcutCharCandidates(nsTArray<PRUint32>& aCharCandidates) This could be merged into GetShortcutCharCandidates using the API suggested in my comments on nsXBLKeyEventHandler::HandleEvent. + { + NS_ASSERTION(isShift, "Shift is not pressed"); + NS_ASSERTION(aCharCandidates.IsEmpty(), "aCharCandidates must be empty"); NS_PRECONDITION + aCharCandidates.Clear(); + for (PRUint32 i = 0; i < shiftedCharCodes.Length(); ++i) { + PRUint32 ch = shiftedCharCodes[i]; + // If the char is an alphabet, the shift key state should not be ignored. + // E.g., Ctrl+Shift+C should not execute Ctrl+C. + if ((ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z')) + continue; This should be done in a less basic-Latin-specific way, probably with a case-insensitive comparison. + // And checking the charCode is in unshiftedCharCode too. + // E.g., for Ctrl+Shift+(Plus of Numpad) should not run Ctrl+Plus. + if (unshiftedCharCodes.IndexOf(ch) == kNotFound) + aCharCandidates.AppendElement(shiftedCharCodes[i]); Is there a reason to check all unshifted character codes? Wouldn't just checking the corresponding code from the same layout be better? (bug 401086 comment 55) if (event.isControl || event.isAlt || event.isMeta) { + // We shold send both shifted char and unshifted char, + // all keyboard layout users can use all keys. + // Don't change event.charCode. On some keyboard layouts, + // ctrl/alt/meta keys are used for inputting some characters. + PRUint32 ch; + // unshifted charcode of current keyboard layout. + ch = GetCharCodeFor(aEvent, GdkModifierType(0), aEvent->group); + PRBool isLatin = (ch <= 0xFF); + if (ch) + event.unshiftedCharCodes.AppendElement(ch); + // shifted charcode of current keyboard layout. + ch = GetCharCodeFor(aEvent, GDK_SHIFT_MASK, aEvent->group); + isLatin = isLatin && (ch <= 0xFF); + if (ch) + event.shiftedCharCodes.AppendElement(ch); I think it is safest to add characters from empty or shift-only modifier states (level 0 and 1 I hope) only when the level is 0 or 1. (If there are use cases where another character is desired then I think that is best considered separately.) I think gdk_keymap_translate_keyboard_state would be the best way to get the level. (It may be possible by detecting modifiers but that may not be reliable.) nsNativeKeyBindings::KeyPress still needs to try Latin characters also.

(In reply to comment #95) > (From update of attachment 312957 [details] [diff] [review]) > @@ -3968,29 +3968,64 @@ nsContentUtils::DOMEventToNativeKeyEvent > > if (aGetCharCode) { > - keyEvent->GetCharCode(&aNativeEvent->charCode); > + nsKeyEvent* nativeKeyEvent = > + static_cast<nsKeyEvent*>(aNativeEvent->nativeEvent); > + if (nativeKeyEvent) { > + nsAutoTArray<PRUint32, 10> chars; > + nativeKeyEvent->GetShortcutCharCandidates(chars); > + aNativeEvent->charCode = chars.IsEmpty() ? 0 : chars[0]; > + } else { > + keyEvent->GetCharCode(&aNativeEvent->charCode); > + } > } else { > aNativeEvent->charCode = 0; > } > > Is this still required? > It looks like chars[0] from GetShortcutCharCandidates and > keyEvent->GetCharCode should provide the same character? If charCode is zero, they are not same character. > + nsAutoString eventType; > + aDOMEvent->GetType(eventType); > + if (!eventType.EqualsLiteral("keypress")) > + return PR_FALSE; > + nsCOMPtr<nsIDOMKeyEvent> key(do_QueryInterface(aDOMEvent)); > > I wonder whether testing for keypress is necessary? > Would it matter if only a zero character was returned? > Would just checking "key" be sufficient? No, if that is not keypress event, nsIDOMKeyEvent::GetCharCode calls NS_WARNING. We should not do nothing if that is keydown/keyup event. > + // And checking the charCode is in unshiftedCharCode too. > + // E.g., for Ctrl+Shift+(Plus of Numpad) should not run Ctrl+Plus. > + if (unshiftedCharCodes.IndexOf(ch) == kNotFound) > + aCharCandidates.AppendElement(shiftedCharCodes[i]); > > Is there a reason to check all unshifted character codes? > Wouldn't just checking the corresponding code from the same layout be better? > (bug 401086 comment 55) This checking is needed. Because Numpad's plus and Shift + Numpad's plus return same '+'. If unshifted charcode has same characters, we should not ignore the shift key state. > if (event.isControl || event.isAlt || event.isMeta) { > + // We shold send both shifted char and unshifted char, > + // all keyboard layout users can use all keys. > + // Don't change event.charCode. On some keyboard layouts, > + // ctrl/alt/meta keys are used for inputting some characters. > + PRUint32 ch; > + // unshifted charcode of current keyboard layout. > + ch = GetCharCodeFor(aEvent, GdkModifierType(0), aEvent->group); > + PRBool isLatin = (ch <= 0xFF); > + if (ch) > + event.unshiftedCharCodes.AppendElement(ch); > + // shifted charcode of current keyboard layout. > + ch = GetCharCodeFor(aEvent, GDK_SHIFT_MASK, aEvent->group); > + isLatin = isLatin && (ch <= 0xFF); > + if (ch) > + event.shiftedCharCodes.AppendElement(ch); > > I think it is safest to add characters from empty or shift-only modifier > states (level 0 and 1 I hope) only when the level is 0 or 1. (If there are > use cases where another character is desired then I think that is best > considered separately.) What's level is 0 or 1? If you meant about the current event, it's impossible. If you meant about the result of GetCharCodeFor, I'm not sure how to do it. > I think gdk_keymap_translate_keyboard_state would be the best way to get the > level. (It may be possible by detecting modifiers but that may not be > reliable.) > > nsNativeKeyBindings::KeyPress still needs to try Latin characters also. ??

(In reply to comment #96) > (In reply to comment #95) > > (From update of attachment 312957 [details] [diff] [review] [details]) > > @@ -3968,29 +3968,64 @@ nsContentUtils::DOMEventToNativeKeyEvent > > Is this still required? > > It looks like chars[0] from GetShortcutCharCandidates and > > keyEvent->GetCharCode should provide the same character? > > If charCode is zero, they are not same character. Can you give me an example please to help me understand the need for this? Is there a situation when the charCode (with modifiers) is zero but modifier-free or shift-only key combinations produce characters and we should be using those characters? > > nsNativeKeyBindings::KeyPress still needs to try Latin characters also. > > ?? The gtk2 version of nsNativeKeyBindings::KeyPress currently sends the Latin characters and this works well for GTK in many situations. If we make no changes there this patch as it is will revert to sending the character from the current layout, which would seem a regression for Cyrillic/Greek/Hebrew/etc keyboard users. Probably something like the XBL accel key handling would work well here (because we are careful to only have other character codes from different scripts). i.e. call gtk_bindings_activate both for the characters from the current layout and from the Latin layout. But if this work is done here then there would be no need to do anything in DOMEventToNativeKeyEvent. (I haven't looked at what the other platform implementations of nsINativeKeyBindings do or should do, sorry.)

(In reply to comment #96) > (In reply to comment #95) > > (From update of attachment 312957 [details] [diff] [review] [details]) > > I think it is safest to add characters from empty or shift-only modifier > > states (level 0 and 1 I hope) only when the level is 0 or 1. (If there are > > use cases where another character is desired then I think that is best > > considered separately.) > > What's level is 0 or 1? If you meant about the current event, it's impossible. > If you meant about the result of GetCharCodeFor, I'm not sure how to do it. If AltGr (or possibly another modifier) is used to produce another character (and so the level will be >= 2 I hope) then I think it is safest not to start using characters that would only be produced without AltGr. Something like this should work I think: if (event.isControl || event.isAlt || event.isMeta) { gint level; if (gdk_keymap_translate_keyboard_state(NULL, aEvent.hardware_keycode, GdkModifierType(aEvent.state), aEvent.group, NULL, NULL, &level, NULL) && level < 2) {

(In reply to comment #97) > (In reply to comment #96) > > (In reply to comment #95) > > > (From update of attachment 312957 [details] [diff] [review] [details] [details]) > > > @@ -3968,29 +3968,64 @@ nsContentUtils::DOMEventToNativeKeyEvent > > > > Is this still required? > > > It looks like chars[0] from GetShortcutCharCandidates and > > > keyEvent->GetCharCode should provide the same character? > > > > If charCode is zero, they are not same character. > > Can you give me an example please to help me understand the need for this? > Is there a situation when the charCode (with modifiers) is zero but > modifier-free or shift-only key combinations produce characters and we should > be using those characters? I don't know the actual example. However, modifier+foo key generates the what charCode depends on the platform. And the result of nsKeyEvent::GetShortcutCharCandidates *can* have the different charCode in the first element. I think the way is safe for any platforms. However, the next part of your comment issue is good point. I'll try to think about the issue. At that time, this issue is not problem, I think.

(In reply to comment #98) > (In reply to comment #96) > > (In reply to comment #95) > > > (From update of attachment 312957 [details] [diff] [review] [details] [details]) > > > > I think it is safest to add characters from empty or shift-only modifier > > > states (level 0 and 1 I hope) only when the level is 0 or 1. (If there are > > > use cases where another character is desired then I think that is best > > > considered separately.) > > > > What's level is 0 or 1? If you meant about the current event, it's impossible. > > If you meant about the result of GetCharCodeFor, I'm not sure how to do it. > > If AltGr (or possibly another modifier) is used to produce another character > (and so the level will be >= 2 I hope) then I think it is safest not to start > using characters that would only be produced without AltGr. Something like > this should work I think: > > if (event.isControl || event.isAlt || event.isMeta) { > gint level; > if (gdk_keymap_translate_keyboard_state(NULL, > aEvent.hardware_keycode, > GdkModifierType(aEvent.state), > aEvent.group, > NULL, NULL, &level, NULL) && > level < 2) { I have a worry. The level is not 0 and 1 when Ctrl/Alt/Meta key is pressed? Ctrl/Alt/Meta keys don't change the level?

(In reply to comment #100) > I have a worry. The level is not 0 and 1 when Ctrl/Alt/Meta key is pressed? > Ctrl/Alt/Meta keys don't change the level? (I haven't tested to be honest but) I would expect that Ctrl/Alt/Meta normally do not change the level. There may be some rare exceptions on some layouts, but even on those layouts where Ctrl/Alt/Meta might be required to produce a different character, I am concerned about the combination for producing that character being used also as a shortcut, as that could surprise some users.

(In reply to comment #96) > (In reply to comment #95) > > (From update of attachment 312957 [details] [diff] [review]) > > + nsAutoString eventType; > > + aDOMEvent->GetType(eventType); > > + if (!eventType.EqualsLiteral("keypress")) > > + return PR_FALSE; > > + nsCOMPtr<nsIDOMKeyEvent> key(do_QueryInterface(aDOMEvent)); > > > > I wonder whether testing for keypress is necessary? > > Would it matter if only a zero character was returned? > > Would just checking "key" be sufficient? > > No, if that is not keypress event, nsIDOMKeyEvent::GetCharCode calls > NS_WARNING. We should not do nothing if that is keydown/keyup event. OK. Thanks. Consider whether doing the test in nsXBLWindowKeyHandler::WalkHandlersInternal is an option as aEventType is available there, but don't worry if it's awkward. > > > + // And checking the charCode is in unshiftedCharCode too. > > + // E.g., for Ctrl+Shift+(Plus of Numpad) should not run Ctrl+Plus. > > + if (unshiftedCharCodes.IndexOf(ch) == kNotFound) > > + aCharCandidates.AppendElement(shiftedCharCodes[i]); > > > > Is there a reason to check all unshifted character codes? > > Wouldn't just checking the corresponding code from the same layout be better? > > (bug 401086 comment 55) > > This checking is needed. Because Numpad's plus and Shift + Numpad's plus > return same '+'. If unshifted charcode has same characters, we should not > ignore the shift key state. What I'm trying to work out is whether it's best to consider only the corresponding unshiftedCharCode from the same layout or consider all unshiftedCharCodes. if(unshiftedCharCodes[i] case-insensitive-!= shiftedCharCodes[i]) { aCharCandidates.AppendElement(shiftedCharCodes[i]); would also handle the Ctrl+Shift+(Plus of Numpad) situation, and it feels more natural to me to only think about one layout at a time. For example, some layouts have Hindu-Arabic numerals (0-9) on level 1 (i.e Shift is required to access them). If there is a layout with a non-Latin character at level 0 on the same key as a numeral but another layout has the numeral on level 0, then checking all unshifted characters would mean that Ctrl-Shift-1 would not match key="1" modifiers="accel". Another case to consider is a Hebrew keyboard with Hebrew(/Latin) and US Latin layouts. In Hebrew layout, Shift changes the script. http://en.wikipedia.org/wiki/Hebrew_keyboard If the user presses Ctrl-Shift-A while in Hebrew layout, should that match Ctrl-A? With checking all layouts for "a", it would not. Maybe that's best - I don't know. If only the Hebrew layout is considered (i.e. only the corresponding unshifted character), then I guess my suggestion for nsXBLKeyEventHandler in Comment 95 would be better changed to look for a Ctrl-Shift-A handler before a Ctrl-A handler, something like: nsAutoTArray<PRBool,10> isShiftConsumed; // (maybe PRPackedBool?) nsContentUtils::GetAccelKeyCandidates(aEvent, accessKeys, isShiftConsumed); for (PRUint32 i = 0; i < accessKeys.Length(); ++i) { if (ExecuteMatchedHandlers(key, accessKeys[i], PR_FALSE)) return NS_OK; if (isShiftConsumed[i] && ExecuteMatchedHandlers(key, accessKeys[i], PR_TRUE)) return NS_OK; } + // return the lower cased charCode candidates for access keys. + void GetAccessCharCandidates(nsTArray<PRUint32>& aCharCandidates) + { + NS_ASSERTION(aCharCandidates.IsEmpty(), "aCharCandidates must be empty"); + if (charCode) + aCharCandidates.AppendElement(charCode); + for (PRUint32 i = 0; i < unshiftedCharCodes.Length(); ++i) { + PRUint32 ch = unshiftedCharCodes[i]; + if (ch >= 'A' && ch <= 'Z') + ch += 0x20; + aCharCandidates.AppendElement(ch); + } + for (PRUint32 i = 0; i < shiftedCharCodes.Length(); ++i) { + PRUint32 ch = shiftedCharCodes[i]; + if (ch >= 'A' && ch <= 'Z') + ch += 0x20; + // Don't append the charCode is already appended. + if (aCharCandidates.IndexOf(ch) == kNotFound) + aCharCandidates.AppendElement(ch); + } + } I think shiftedCharCodes from the selected layout should have priority over unshiftedCharCodes in other layouts (at least when Shift is pressed, but probably even when not to be consistent). for (PRUint32 i = 0; i < unshiftedCharCodes.Length(); ++i) { PRUint32 ch = unshiftedCharCodes[i]; case-convert-ch; aCharCandidates.AppendElement(ch); PRUint32 ch = shiftedCharCodes[i]; case-convert-ch; // Don't append the charCode is already appended. if (aCharCandidates.IndexOf(ch) == kNotFound) aCharCandidates.AppendElement(ch); }

Thank you, Karl. This changes the previous patch as you said: 1. nsContentUtils::DOMEventToNativeKeyEvent is not changed (from current trunk). 2. nsNativeKeyBindings::KeyPress handle the alternative charCodes. 3. Supporting UCS4 in accesskey handling. 4. nsKeyEvent::GetAlternateShortcutCharCandidates is removed. 5. shiftedCharCodes/unshiftedCharCodes are not appended when AltGr is pressed on Win32 and Linux (and also Alt is pressed on Mac).

Comment on attachment 313914 [details] [diff] [review] Patch v8.0 The GTK and XP parts are looking pretty good to me. Just a few smaller things: +++ mozilla/content/base/public/nsContentUtils.h + * If this aDOMEvent is keypress event, this returns TRUE, otherwise FALSE. + * When this returns FALSE, the output params are not modified. It looks like the return code is no longer needed + * @param aShiftConsumed [out] the candidate charCodes should be consumed with + * shift key state. If this is true, the charCode Do you mean "the candidate char list should be considered together with shift key state"? Or perhaps "whether the shift key modifier was necessary to produce the corresponding character". +nsContentUtils::GetAccelKeyCandidates(nsIDOMEvent* aDOMEvent, + aShiftConsumed.AppendElement(PR_TRUE); I had suggested PR_FALSE. Did you decide on PR_TRUE intentionally? We don't have enough information to get this right all the time I guess, but I assumed that if there was no native event then this was probably a synthesized event and so we could be stricter as there were no keyboard-imposed limitations. nsXBLKeyEventHandler::HandleEvent(nsIDOMEvent* aEvent) + PRBool shift = PR_FALSE;; How about calling this ignoreShift or similar? "shift" kind of implies the opposite. +++ mozilla/layout/xul/base/src/nsMenuBarFrame.cpp +#include "nsIPrivateDOMEvent.h" + nsCOMPtr<nsIPrivateDOMEvent> privKeyEvent(do_QueryInterface(aKeyEvent)); Not used. Maybe nsIPrivateDOMEvent.h is not used in nsMenuBarListener.cpp too? + PRUint32 ch; + if (shortcutKey.Length() >= 2 && + NS_IS_HIGH_SURROGATE(shortcutKey[0]) && + NS_IS_LOW_SURROGATE(shortcutKey[1])) { + ch = SURROGATE_TO_UCS4(shortcutKey[0], shortcutKey[1]); + } else { + ch = ToLowerCase(shortcutKey[0]); + } I haven't checked whether non-BMP chars will match (the main thing I want to be sure of is that they don't match some other character), but ToLowerCase can be applied to an nsAString if you like, i.e. ToLowerCase(shortcutKey); Consider nsUTF8Utils.h too (SURROGATE_TO_UCS4 is OK if you prefer though). nsAutoString::const_iterator start, end; shortcutKey.BeginReading(start); shortcutKey.EndReading(end); PRUint32 ch = UTF16CharEnumerator::NextChar(start, end); + void GetShortcutCharCandidates(nsTArray<PRUint32>& aCharCandidates, + // If the char is an alphabet, the shift key state should not be + // ignored. E.g., Ctrl+Shift+C should not execute Ctrl+C. + if (IS_IN_BMP(ch) && + ToUpperCase(PRUnichar(ch)) != ToLowerCase(PRUnichar(ch))) + continue; + + // And checking the charCode is same as unshiftedCharCode too. + // E.g., for Ctrl+Shift+(Plus of Numpad) should not run Ctrl+Plus. + if (unshiftedCharCodes[i] == ch) + continue; Can these two tests be merged into one concept? AIUI it is not so much whether the character is a bicameral letter that is important but whether using the Shift key produces the same character (excluding case changes) as without the Shift key. PRUint32 unshiftedCh = unshiftedCharCodes[i]; if (ch == unshiftedCh || (IS_IN_BMP(ch) && IS_IN_BMP(unshiftedCh) && ToLowerCase(PRUnichar(ch)) == ToLowerCase(PRUnichar(unshiftedCh)))) continue; + NS_ASSERTION(aCharCandidates.Length() == aShiftConsumed.Length(), + "aCharCandidates and aShiftConsumed have differnt length"); + } differnt -> different. http://developer.mozilla.org/en/docs/NS_POSTCONDITION sounds appropriate here. +GetLayoutLevel(GdkEventKey *aEvent) I'm not so comfortable with including "Layout" in the name of this function, as it might be interpreted as WhichLayout and IIUC the level really relates to the key within the layout. How about GetKeyLevel (though I wish I could think of something better)? +++ mozilla/widget/src/gtk2/nsNativeKeyBindings.cpp + if (!(keyCode & 0x01000000) && I don't think we want this test. It wasn't done for aEvent.charCode. IIUC GTK uses (wc | 0x01000000) to represent some (possibly rare) keyvals. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/keysym2ucs.c&rev=1.6&mark=880-882#869 Checking that charCodes[i] is not 0 could be worthwhile though. /* gtk_bindings_activate_event is preferable, but it has unresolved bug: http://bugzilla.gnome.org/show_bug.cgi?id=162726 This comment should stay in KeyPress (rather than KeyPressInternal) as gtk_bindings_activate_event does the iterating over layouts.

ok, the concept of the patch is now fixed. so, I'm also requesting to review the windows part and cocoa part to Ere and Josh. Ere and Josh: This patch improves the acess key handling and acceleration key handling. 1. widget should not modify the nsKeyEvent::charCode. 2. On Windows, when Ctrl or Alt key is pressed, On Mac, when the Cmd or Ctrl key is pressed but Alt key is not pressed, widget should set the charCode of the modifier keys to nsKeyEvent::unshiftedCharCodes and nsKeyEvent::shiftedCharCodes. The XP code will also use them for the access/accel key handling.

Comment on attachment 314174 [details] [diff] [review] Patch v8.1 + if (IS_IN_BMP(ch) && IS_IN_BMP(unshiftCh) && + ToLowerCase(PRUnichar(ch)) == ToLowerCase(PRUnichar(unshiftCh))) + continue; Can we continue on ch == unshiftCh also please? if (ch == unshiftCh || (IS_IN_BMP(ch) && IS_IN_BMP(unshiftCh) && ToLowerCase(PRUnichar(ch)) == ToLowerCase(PRUnichar(unshiftCh)))) continue; r=me on the GTK parts, and on the XP parts with that change (though someone else looking over the XP parts could be helpful).

Comment on attachment 314174 [details] [diff] [review] Patch v8.1 + ::GetScriptVariable(::GetScriptManagerVariable(smKeyScript), + smScriptKeys); This is deprecated in 10.5. According to the Apple docs: "smKeyScript. To obtain the intended language associated with the user's current keyboard input source (plus other languages that can be input using it), use TISCopyCurrentKeyboardInputSource to get that input source, then pass it to TISGetInputSourceProperty with the kTISPropertyInputSourceLanguages key." + Handle handle = ::GetResource('uchr', keyLayoutID); Can you avoid using ::GetResource? That is outdated. If you aren't going to get rid of it, do you need to release it with ReleaseResource? Perhaps you don't as the docs say "Do not use this function to release a System resource that might be shared by several applications"? + UInt32 kbType = ::LMGetKbdType(); Also outdated. Can you avoid it? + ::KeyTranslate(handle, keyCode, &state) & charCodeMask); Also outdated. Please see docs for a modern replacement. The problem with a lot of these outdated calls is that we're just going to have to replace them in a month for 64-bit. This stuff isn't going to be available in 64-bit.

(In reply to comment #107) > (From update of attachment 314174 [details] [diff] [review]) > + ::GetScriptVariable(::GetScriptManagerVariable(smKeyScript), > + smScriptKeys); > > This is deprecated in 10.5. According to the Apple docs: > > "smKeyScript. To obtain the intended language associated with the user's > current keyboard input source (plus other languages that can be input using > it), use TISCopyCurrentKeyboardInputSource to get that input source, then pass > it to TISGetInputSourceProperty with the kTISPropertyInputSourceLanguages key." The new APIs cannot be used on 10.4, right? I think we cannot them until killing the 10.4 support. > + Handle handle = ::GetResource('uchr', keyLayoutID); > > Can you avoid using ::GetResource? That is outdated. If you aren't going to get > rid of it, do you need to release it with ReleaseResource? Perhaps you don't as > the docs say "Do not use this function to release a System resource that might > be shared by several applications"? > > + UInt32 kbType = ::LMGetKbdType(); > > Also outdated. Can you avoid it? > > + ::KeyTranslate(handle, keyCode, &state) & charCodeMask); > > Also outdated. Please see docs for a modern replacement. Do you know the alternative APIs for them? I'm not sure the other approaches... > The problem with a lot of these outdated calls is that we're just going to have > to replace them in a month for 64-bit. This stuff isn't going to be available > in 64-bit. Yeah, but I'm not sure our all code can be ported to 64bit. Especially, low level API used points.

Comment on attachment 314174 [details] [diff] [review] Patch v8.1 Ah, you're right. Those APIs I suggested are 10.5 only. r+ for the widget/src/cocoa changes

Josh: Please re-review this. I changed Cocoa part for bug 427797. The previous patch might execute wrong item if the keyboard layout is Dvorak-QWERTY. Because the previous patch creates the charCode list for 'o' key of Dvorak as: 'o', 's'. So, Cmd+S is used after Cmod+O failed. This patch checks Cmd+Shift+'foo' is same as non modified 'foo'. If Cmd key is pressed and they are different, the keyboard layout is switchable by Cmd key. Then, we don't add the alternative charCode. So, at above case, only 'o' is sent to Gecko.

Try server builds with Masayuki's patch v8.2: https://build.mozilla.org/tryserver-builds/2008-04-09_17:52-josh@mozilla.com-1207788662/

(In reply to comment #110) > The previous patch might execute wrong item if the keyboard layout is > Dvorak-QWERTY. Because the previous patch creates the charCode list for 'o' key > of Dvorak as: 'o', 's'. So, Cmd+S is used after Cmod+O failed. That's different from what I observed. Cmd+F opened the Find bar, not the Page Source window (Cmd+U). Strange. (In reply to comment #111) > Try server builds with Masayuki's patch v8.2: > > https://build.mozilla.org/tryserver-builds/2008-04-09_17:52-josh@mozilla.com-1207788662/ Works perfectly.

Comment on attachment 314624 [details] [diff] [review] Patch v8.2 Windows part looks ok to me.

Comment on attachment 314624 [details] [diff] [review] Patch v8.2 Thank you, Ere. Let's go to the final review process.

GetAccessCharCandidates and GetShortcutCharCandidates should not be inline. + void GetShortcutCharCandidates(nsTArray<PRUint32>& aCharCandidates, + nsTArray<PRBool>& aShiftConsumed) Instead of the parallel nsTArrays, I would define a struct which contains the keycode and the "ignore shift" flag and use a single nsTArray of those. Use the same struct in nsContentUtils::GetAccelKeyCandidates. What can we do for tests here? Can we synthesize key events to test command-key dispatching?

(In reply to comment #115) > What can we do for tests here? Can we synthesize key events to test command-key > dispatching? I think we don't need to care the synthesize events. Because this patch only helps the keyboard layout difference for some key combination. The synthesize events are not limited by the actual keyboard layouts. They can send any key combination that are needed.

So does that mean you can write automated tests for this? If so, please write some!

(In reply to comment #117) > So does that mean you can write automated tests for this? If so, please write > some! Can we emulate the native key events at automated tests??

using |nsTArray<nsShortcutCandidate>|.

Maybe we can't test the widget/ code, but surely we can test the rest of the code by synthesizing events with the right charcode field values.

+ for (PRUint32 i = 0; i < accessKeys.Length() || i == 0; ++i) { + PRUnichar ch = 0; + PRBool ignoreShift = PR_FALSE; + if (!accessKeys.IsEmpty()) { + ch = accessKeys[i].mCharCode; + ignoreShift = accessKeys[i].mIgnoreShift; Wouldn't this be simpler if you just did if (accessKeys.IsEmpty()) { ExecuteMatchedHandlers(key, 0, PR_FALSE); } else { ... } ? + void GetAccessCharCandidates(nsTArray<PRUint32>& aCharCandidates) + { + void GetShortcutCharCandidates(nsTArray<nsShortcutCandidate>& aCandidates) + { These are still inline. + // Widgets MUST set same length for both charCodes. (If the key doesn't + // generate the charCode, set 0 instead. + nsTArray<PRUint32> shiftedCharCodes; + nsTArray<PRUint32> unshiftedCharCodes; This should be a single array of structs too, then you don't have to worry about the lengths being different. Sorry I didn't catch this the first time around.

Moving the methods of nsKeyEvent to nsContentUtils. And combining nsKeyEvent::unshiftedCharCodes and nsKeyEvent::shiftedCharCodes to nsKeyEvent::alternativeCharCodes.

Comment on attachment 315469 [details] [diff] [review] Patch v9.0 Okay. I haven't really checked all the logic, I'm relying on Karl for that :-). We definitely need tests here. You can test everything except for the per-platform code by extending nsDOMWindowUtils::SendKeyEvent.

Comment on attachment 315469 [details] [diff] [review] Patch v9.0 This fixes very many accessibility bugs (see the blocking bugs of this). I believe that the concept of this patch is correct.

Roc: (In reply to comment #123) > (From update of attachment 315469 [details] [diff] [review]) > We definitely need tests here. You can test everything except for the > per-platform code by extending nsDOMWindowUtils::SendKeyEvent. Can I create the tests on mochitests? And are there some examples of the key handling tests?

Mochitests should work fine for testing this. I'm not sure that we have any accessskey specific tests, but toolkit/content/tests/widgets/window_menubar.xul for example does test that accesskeys work, using synthesizeKey from EventUtils.

For a (not so simple, but better than nothing) example, see the test case I wrote for bug 409604 (attachment 302552 [details] [diff] [review]).

This is a really important patch, I recommend giving approval ASAP. We should allow Masayuki to create tests post-landing so we don't have intl access keys broken for any more nightly builds.

yeah, I need more time for creating the test. but this patch should be landed ASAP. The patch is pretty large. This pending is not reasonable for other developers and testers. Gavin and Ehsan: Thank you for your advice! It seems that I don't need to create access key testing, because bug 409604's test is testing it. And I'm still thinking how to test the XBL command handlers :-(

Comment on attachment 315469 [details] [diff] [review] Patch v9.0 a=mconnor on behalf of 1.9 drivers

The tests in bug 409604 only test a small part of your code. You need to write a lot more tests to test things like the XBL event handler path and different combinations of shifted and unshifted keycodes. I'm OK with this landing now but these tests are really important, we need them now, not later. If my experience is any guide you will probably find bugs in this patch while writing the tests.

checked-in, and the all tests on Mac passed.

all tests passed, I'll post the tests. (trying to create them now...)

I'm not sure but seems like after the check-in, Bug 348472 "Command-Option-F should select the web search box" stopped working.

(In reply to comment #134) > I'm not sure but seems like after the check-in, Bug 348472 "Command-Option-F > should select the web search box" stopped working. please file a new bug. we cannot track the regression on this bug.

(In reply to comment #135) > please file a new bug. we cannot track the regression on this bug. Filed Bug 429160 – [Mac]Regression: Command-Option-F does not select search box

mmmm.... I'm creating the menubar accesskey testing, but I cannot access to child menus of my menubar with chrome access modifiers both on mochitest and chrome test at sending the synthesize key events. However, when I press the actual keys, they can access to the testcase's menubar with chrome access modifiers... it's strange...

This check-in broke hotkeys (Ctrl-F, Ctrl-C, Ctrl-V, etc.) in Russian locale Works in http://hourly-archive.localgho.st/win32/20080414_2052_firefox-3.0pre.en-US.win32.zip Fails in http://hourly-archive.localgho.st/win32/20080414_2241_firefox-3.0pre.en-US.win32.zip http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1208231520&maxdate=1208238059

Egor: Please file a new bug for the regression.

I'm going to put this back on the blocker list because it seems to be breaking lots of other hotkeys on multiple platforms. Doesn't look like our test coverage here is very good, eh?

This also apparently regressed bug 417176 (as reported in bug 359638)

Leaving this open just to track the tests is very confusing. Please file a new bug about writing tests for this.

Gavin: OK. I'll file a new bug for tests. But my current first priority is fixing the all regressions by this bug.

This caused a small leak, see bug 429233.

Given all the bustage this change has caused in our XUL code, this is certainly late-compat, and we should document what the new behavior is. Can somebody provide an overview of what changed?

(In reply to comment #145) > Given all the bustage this change has caused in our XUL code, this is certainly > late-compat, and we should document what the new behavior is. Can somebody > provide an overview of what changed? I agree. The nsIDOMKeyEvent::charCode value is changed when Alt/Ctrl/Meta keys are changed. The charCode was replaced in old code (I'm not sure that is same on Fx2). On Win32, replaced to [a-zA-Z0-9] when the key can input them without the modifiers. On Linux, maybe same as Win32. On Mac, always replaced to non-modifierd character (note that this behavior is the previous behavior, not Fx2). However, that breaks intl keyboard inputting. If the modifier keys are needed to input the Unicode characters, the behavior broke the correct behavior. Therefore, we don't replace the charCode value from native event. So, the charCode value depends on the platform (i.e., Ctrl+A may not generate same charCode on all platforms). So, Javascript cannot handle the key combination with Ctrl/Alt/Meta keys on keypress event. And the developers should not process directly. We need very very complex rules for handling the intl keyboard layouts, so, the gecko application develpers should use key element for command handling.

(In reply to comment #146) > However, that breaks intl keyboard inputting. If the modifier keys are needed > to input the Unicode characters, the behavior broke the correct behavior. > Therefore, we don't replace the charCode value from native event. So, the > charCode value depends on the platform (i.e., Ctrl+A may not generate same > charCode on all platforms). If some web apps are broken by this change, we should replace the charCode. Can somebody know such web apps? But even if we back to the behavior, when the original charCode is not ASCII character, we cannot replace at such case.

Gavin: I changed my mind. We should replace charCode in most cases for Web applications. # But XUL application should not use charCode when Ctrl/Alt/Meta is pressed. See bug 429510.

(In reply to comment #147) > If some web apps are broken by this change, we should replace the charCode. Can > somebody know such web apps? But even if we back to the behavior, when the > original charCode is not ASCII character, we cannot replace at such case. > Check Google Docs application with this patch. AFAIK it uses key-combinations.

Wow, the checked-in patch also has the part of bug 426501. Because that is changed the same cpp file. Sorry for the dangerous mistake. # The part was updated by the new patch of bug 426501, so, the wrong code were replaced by the correct code now.

Bug 306585 is not a regression from this bug. See bug 429211 comment 2.

(In reply to comment #59) > I think the real bug here is that (by default) Shift is used to indicate a > document access key on Windows and Linux (bug 349716). This makes it > impossible to distinguish which level to use in guessing which char code the > user intends. > The real solution IMO is to use something different to indicate a document > access key. http://wiki.mozilla.org/AccessKeys_solution seems a good solution > to me, Bug 303192.

I posted a document to MDC for key hell bugs. http://developer.mozilla.org/en/docs/Gecko_Keypress_Event Please fix the document by the native English speakers.

Is this bug fixed? I am on a U.S(QWERTY) keyboard on WinXP, ctrl+shift+dot and ctrl+shift+/ doesnt work for me in firefox3b5 and latest nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051406 Minefield/3.0pre) Please try the following, FF2 takes ctrl+shift+. while FF3 takes ctrl+shift+> instead. <key id="key-test-0" key=">" shift="true" modifiers="accel,shift" oncommand="alert('Pressed key: >');"/> <key id="key-test-1" key="?" modifiers="accel,shift" oncommand="alert('Pressed key: ?');"/> <key id="key-test-2" key="." modifiers="accel,shift" oncommand="alert('Pressed key: DOT');"/> <key id="key-test-3" key="/" modifiers="accel,shift" oncommand="alert('Pressed key: /');"/> Am i doing something wrong?

Oh, currently, key elements cannot handle Ctrl+Shift+unshiftedChar. That's a bug. But the report is too late for 1.9.0. Please file a new bug. It might break some extensions.

However, Ctrl+Shift+'>' should win to Ctrl+Shift+'.'. Because I have no idea that the Fx2 behavior is better.

I filed bug 433907 for comment 154, 155 and 156.