351310 - Use unshifted charCode/keyCode for accesskey handling

Status: RESOLVED FIXED

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

Description

[Simon Bünzli]

From bug 349716 comment #5:
> Numeric accesskeys are not working

This is a "regression" introduced with bug 340902.

One possible solution to this problem would be to determine the pressed accesskey by the keyCode instead of the charCode. This would make the keys independent of the Shift state and would render Alt+Shift+<number> operational again (while OTOH disabling special characters as accesskeys - this might however even be seen as an advantage because it forces web developers to stick to generally available keys).

Note that the behavior reported above is not a true regression, because numeric accesskeys have been broken already for certain keyboard layouts (e.g. fr-FR).

Caveat: While theoretically this should be simple to implement, it apparently isn't at all, because Gecko clears out the keyCode information before passing it to the accesskey handling code for almost all relevant keys.

Comment 1

[Simon Bünzli]

Aaron, Mats: What are your opinions on fixing this bug for correcting bug 357101 (Alt+Shift+<number> broken)?

AFAICS all modifier combinations not involving Shift are already in use by chrome. Additionally we've so far had Ctrl+Shift shortcuts to extend the number of possible accelerators. As it turns out, the same is currently not possible for Alt+Shift shortcuts - i.e. the charCode is shifted in this case (as opposed to the Ctrl+Shift case).

One further alternative would be to make sure that Ctrl+Shift and Alt+Shift always behave the same. I don't know what consequences such a change might have, though.

If both options are out of the question, we'd probably have no other option than to rewrite our accesskey code so that non-modifiers could act as modifiers as well (Opera allows for pretty much any key as content accesskey modifier, e.g. "." or the default "Shift+Esc").

IMHO Alt+Shift would still be a logical fit though (Alt being the default accesskey modifier and Shift meaning access to alternate functionality for otherwise the same combo).

Finally: Could you suggest anybody to judge whether reducing the list of possible accesskeys for chrome and content might have any i18n issues?

Comment 2

[Simon Bünzli]

So, using keyCode instead of charCode would solve the issue of numeric accesskeys not working, but would pretty much disable all non-alphanumeric keys (see attachment 243344 [details] [XPI] to bug 357101 which more or less implements this proposal).

It'd have to be the charCode of the unshifted key instead as available e.g. through MapVirtualKeyEx on Windows (which might be implemented at the same time as fixing bug 50255). As mentioned in comment #1, I don't know whether that would have further consequences though.

Comment 3

[[e-mail]]

Well maybe alt+shift key chords were just a bad idea?

Comment 4

[Simon Bünzli]

(In reply to comment #3)
> Well maybe alt+shift key chords were just a bad idea?

Yeah, that's about the alternative conclusion...

Any suggestions for different modifiers/keys instead?

Comment 5

[Simon Bünzli]

Attachment 243506 [details] partially implements the solution using unshifted char codes (works on Windows only). This fixes all accesskey issues I've so far encountered. Can't tell whether it's comprehensive enough a solution. Anyway, it looks like it would be simple enough to implement.

Open questions:
* Do functions equivalent to MapVirtualKey exist on other platforms?
* Would this break any existing use of accesskeys? (if so, we should soon get BugZilla reports anyway, since chrome accesskeys have the same limitations)
* Could such a solution be implemented on the 1.8.1 branch as well?
* What would the alternatives be?

In one scenario, we'd have to remove Shift from the list of possible accesskey modifiers. That would however mean that (1) either we've again got conflicts between content and chrome, (2) or we have to use Ctrl+Alt for content which conflicts with global shortcuts on Windows, (3) or that we have to completely disable content accesskeys (no real option, either).

An additional solution would consist in rewriting all accesskey handling code to allow any key (combo) as accesskey modifier. Alternatives for invoking would then be either one of the unused function keys or a Ctrl+<key> shortcut (haven't seen any other key which isn't already used or might not exist on all keyboard layouts).

A further alternative might consist in disabling content accesskeys in the existing code path and present a list of available accesskeys in the current page after hitting a traditional shortcut (as proposed in bug 303192). Drawback of such a solution would be that they're no longer as quick to use since the first key combo has to be released before hitting the accesskey.

Except with the solution proposed in this bug, I couldn't really help with the implementation though.

Mats, Neil: your call.

Comment 6

[Simon Bünzli]

It seems that we're already heading in the direction intended by this bug on trunk. AFAICT, due to the fix to bug 339723 numeric accesskeys work on Windows with Alt+Shift as well. Was that an intentional change resp. is it supposed to stay?

Comment 7

[Mats Palmgren (inactive)]

Attachment: wip (gtk2 only)

This fixes it on Linux/gtk2 by propagating the key code for the *unshifted*
key in event.keyCode and making ESM::HandleAcessKey use that instead of the
char code when the mask contains shift.

Can MapVirtualKeyEx do this on Windows?

I haven't checked if MacOSX have something similar, but Mac uses CTRL+key
by default so it's not as critical there.

Another solution would be to use a "prefix key" that presents a menu as
Opera does for Shift+Esc (bug 303192).
This is slower to type, but provides a nice overview of the available keys.
I think it would be nice to have both direct keys and a menu eventually.

Comment 8

[Simon Bünzli]

(In reply to comment #7)
> This fixes it on Linux/gtk2 by propagating the key code for the *unshifted*
> key in event.keyCode and making ESM::HandleAcessKey use that instead of the
> char code when the mask contains shift.

Is this the better solution than setting event.charCode to the unshifted char code and keep ESM:HandleAccessKey unchanged? This solution would have the advantage that Alt+Shift key combinations would be handled as consistently as Ctrl+Shift ones.

> Can MapVirtualKeyEx do this on Windows?

Yes.

> I think it would be nice to have both direct keys and a menu eventually.

Opera currently allows both with the same key combination; e.g. Shift+Esc gives you the overview while Shift+Esc+A activates the element with accesskey="a".

Comment 9

[Simon Bünzli]

Attachment: minimal branch patch (all platforms)

As mentioned in my mail to Aaron, this is the minimal solution I'd have suggested for fixing the specific regression of numeric accesskeys on the branch. This patch is pretty minimal in code and as local as possible in impact (doesn't change any interfaces, affects only accesskeys) and might thus qualify for the 1.8.1 branch.

Comment 10

[Aaron Leventhal]

This blocks bug 357101.

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #8)
> Opera currently allows both with the same key combination; e.g. Shift+Esc gives
> you the overview while Shift+Esc+A activates the element with accesskey="a".

I think you misunderstood what I meant by "direct keys", Opera does not
have direct keys, you need to go via a menu (Shift+Esc (release) then 
a plain 'a'). There is no way to avoid the menu as far as I know.
http://www.opera.com/support/tutorials/nomouse/#access

With a direct key I mean the accesskey + modifiers that invokes the
accesskey action directly, not a prefix key followed by the accesskey.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 244595 [details] [diff] [review]
minimal branch patch (all platforms)

I doubt this will work on many keyboards. It doesn't work on
mine for example (Swedish, se-latin1 to be precise).
My keyboard have these keys:
+--------+  +--------+ +--------+  +--------+  +--------+  
|    !   |  |  "     | |  #     |  |  ¤     |  |    %   |  
|    1   |  |  2  @  | |  3  £  |  |  4  $  |  |    5   |  
+--------+  +--------+ +--------+  +--------+  +--------+  ...
The key codes (nsKeyEvent::keyCode) for these keys are:
Alt:       '1' '2'  '3' '4' '5' '6' '7'  '8' '9'  '0'
Shift+Alt: '1' 0xDE '3' 0x0 '5' '7' 0xBF '9' 0x30 0x3D

For a given key code we need to figure out what the key code *would have
been if shift had not been pressed*. I don't think it's possible to
do that without the support of platform specific methods like
gdk_keymap_lookup_key and MapVirtualKeyEx.
And those operate on native data, not DOM key codes.

Comment 13

[Simon Bünzli]

(In reply to comment #11)
> I think you misunderstood what I meant by "direct keys", Opera does not
> have direct keys

My mistake. It just happens that in the case where you hit Shift before Esc and then hit an accesskey without releasing any of them the accesskey is directly triggered. This does however not work with numeric keys, since you'd of course have to release Shift first...

(In reply to comment #12)
> The key codes (nsKeyEvent::keyCode) for these keys are:
> Shift+Alt: '1' 0xDE '3' 0x0 '5' '7' 0xBF '9' 0x30 0x3D

Ough. This works so beautifully under Windows that I couldn't imagine it was so broken under Linux.

(In reply to comment #7)
> This fixes it on Linux/gtk2 by propagating the key code for the *unshifted*
> key in event.keyCode and making ESM::HandleAcessKey use that instead of the
> char code when the mask contains shift.

Both setting the keyCode (as you do it here) or rather setting the charCode to the unshifted value when Ctrl/Alt/Meta is hit (as I'd have proposed) do have the potential problem that we don't know what other code relies on this. Do you consider such a change realistic for the 1.8.1 branch?

It might make sense to at least check the modifiers first and see whether they match either chromeAccess or contentAccess (should any of them include the Shift key).

Comment 14

[Simon Bünzli]

Attachment: branch patch (Windows only)

This patch implements the proposed change of making the charCode of shortcuts and accesskeys more predictable (i.e. alphanumeric characters are used as is, all other keys are used unshifted). This would thus fix the accesskey issue in a more general way.

While I don't know what impact this change might have in other parts of the browser, it would among other also fix bug 50255.

One reason why I'd prefer using charCode instead of keyCode is that what we pass around really isn't a keyCode but much rather a charCode. And as mentioned in bug 246409, there seems to exist code which relies on the fact that keyCode == 0 for NS_KEY_PRESS. The charCode approach might therefore be the safer one...

Comment 15

[Mats Palmgren (inactive)]

(In reply to comment #14)
> One reason why I'd prefer using charCode instead of keyCode is that what
> we pass around really isn't a keyCode but much rather a charCode.

Yes, it's a char code. And I agree with you that using the keyCode field
to pass this value up to ESM is too risky considering how the event DOM
is used on some pages.

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 244679 [details] [diff] [review]
branch patch (Windows only)

This patch looks good. One detail needs to be fixed though, diacritics
are returned by MapVirtualKey with the MSB set:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/userinput/keyboardinput/keyboardinputreference/keyboardinputfunctions/mapvirtualkey.asp
After typing Alt+Shift+diacritic repeatedly (with the patch)
I got completely different characters, like '+' (or '=' with shift)
when typing the ´ (accent) key.
(Leaving 'uniChar' unchanged when MSB is set seemed to fix it.)

nit: write ::MapVirtualKey

Comment 17

[Simon Bünzli]

Attachment: branch patch (Windows only; nits fixed)

Comment 18

[Daniel Veditz [:dveditz]]

Looks like Simon has been patching, Congratulations, you're the new bug owner :-)

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 244798 [details] [diff] [review]
branch patch (Windows only; nits fixed)

Looks good. r=mats

Comment 20

[Mats Palmgren (inactive)]

Attachment: Fix for gtk2

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 244798 [details] [diff] [review]
branch patch (Windows only; nits fixed)

I have landed this on trunk (2006-11-23 18:10 PST).
I don't think trunk baking is that important though since
this patch makes no difference there (it will most likely
be needed later though). Since the trunk/branch differs
I think we should land it on branch right away.

Comment 22

[Simon Bünzli]

Comment on attachment 246444 [details] [diff] [review]
Fix for gtk2

Looks good. Since I can't test it and am no proper reviewer anyway, I suggest passing this for (super)review to somebody else additionally though.

Comment 23

[Reed Loden [:reed]]

If this is fixed on the trunk, is there any reason why it is not already RESO FIXED?

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 246444 [details] [diff] [review]
Fix for gtk2

approved for 1.8 branch, a=dveditz for drivers

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 244798 [details] [diff] [review]
branch patch (Windows only; nits fixed)

approved for 1.8 branch, a=dveditz for drivers

Comment 26

[Simon Bünzli]

Mats or Gavin: could you please check in both patches on the 1.8 branch before this Thursday? Thanks.

(In reply to comment #23)
> is there any reason why it is not already RESO FIXED?

This issue affects most probably more than just the two platforms for which we've got patches. I suppose that even OS X is affected (numeric accesskeys not working with a fr-FR keyboard layout).

For manageability it might be better to file new bugs for each affected platform though (all bugs depending on bug 357101). Mats, which do you prefer?

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 246444 [details] [diff] [review]
Fix for gtk2

Checked in to trunk at 2006-11-28 03:58 PST
Checked in to MOZILLA_1_8_BRANCH at 2006-11-28 04:08 PST

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 244798 [details] [diff] [review]
branch patch (Windows only; nits fixed)

Checked in to MOZILLA_1_8_BRANCH at 2006-11-28 04:17 PST

Comment 29

[Mats Palmgren (inactive)]

(In reply to comment #26)
> For manageability it might be better to file new bugs for each affected
> platform though (all bugs depending on bug 357101).

Yes, I agree.

Comment 30

[Mats Palmgren (inactive)]

-> FIXED

Comment 31

[Simon Bünzli]

This fix works nicely on the 1.8.1.1 branch (verified with a Win32 2006113005 nightly).

I've just accidentally tested this on Trunk as well and the Windows patch didn't have any effect at all there. Numeric accesskeys were already working before (don't know why exactly though) and non-alphanumeric accesskeys are still sensitive to the Shift state.

Reopening until it's clear how to proceed from here for Gecko 1.9.

Comment 32

[Simon Bünzli]

QA/testing: Steps to reproduce:
1. Go to http://www.defra.gov.uk/accesskeys.htm
2. Activate any of the numeric accesskeys (e.g. under Windows Alt+Shift+3 for the sitemap; resp. Ctrl+Shift+3 under Linux/Unix)

Additional steps to reproduce:
1. Go to http://wiki.mozilla.org/ and log in
2. Hit Alt+Shift+. (resp. Ctrl+Shift+. under Linux/Unix) to access your personal wikipage

See bug 357101 comment #37 for further verification.

Feel free to verify that Ctrl+3 resp. Ctrl+. works under OS X as well; this bug was only about Windows and Linux/GTK2 though.

Comment 33

[Simon Bünzli]

(In reply to comment #32)
Mats kindly pointed out that it's Alt+Shift+<key> under Linux just as well as under Windows.

Comment 34

[Simon Bünzli]

Marking FIXED again (cf. comment #30).

Mats: As it currently stands, shifted accesskeys are handled differently on all three major platforms. That's most probably not how we'd want to keep it... I prefer leaving the driving of the investigation as to how to resolve the remaining issues to you resp. a different bug though.