295228 - Make sure keypress handlers do not expect lowercase letters when Alt/Ctrl is down and are not affected by Caps Lock state

Status: RESOLVED FIXED

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

Description

[Dainis Jonitis]

This is required to fix bug 287179.
This is cross platform fix for bugs like bug 178110, bug 138475, bug 6135, bug
158679 etc.

The idea is to change all consumers of "keypress" to not expect that lowercase
"charCode" will be returned by platform specific Widget code, if Alt/Ctrl
modifier keys are pressed (as it was done in bug 285161).
The platform specific code should return the correct letter case, taking into
account the Shift and Caps Lock key states.
The consumers should perform case insensitive comparison for "charCode". To
distinguish between lower and upper case characters they should explicitly check
the "isShift".

Comment 1

[Dainis Jonitis]

Attachment: diff -d -u -15

The only consumer that was not upper/lower case ready was:
/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp

The other files are changed only to make minor cleanup by removing redundant
code, fixing broken debug code etc.

Please note that I've added the Tab as a valid separator for modifier keys in
nsXBLPrototypeHandler::ConstructPrototype (). I hope it is logical to not
distinguish between Tab and Space characters.

Comment 2

[Dainis Jonitis]

Comment on attachment 184318 [details] [diff] [review]
diff -d -u -15

I am not sure whether I selected correct component and even more confused about
who should review/superreview the code. Some changes are in content, other in
layout and even in extensions.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm not sure that calling ToLowerCase on a PRUint32 is a meaningful operation...
In fact, I'm pretty sure that breaks for non-plane-0 characters.

That said, I'm not sure I can do a thorough review of this any time soon (within
a week or more).

bryner, neil, jag might be able to review changes to XBL.

Comment 4

[Dainis Jonitis]

Do you think that I should throw away most significant word by converting to
PRUnichar? Or should I interpret it as a nsAString and perform comparison on it
to allow non-plane-0 characters?

Comment 5

[[e-mail]]

Comment on attachment 184318 [details] [diff] [review]
diff -d -u -15

>-    if (!(mKeyMask & cShift))
>-      mKeyMask &= ~cShiftMask;
This looks as if it might break bug 104371.

Comment 6

[Dainis Jonitis]

Neil,
At least with patch for bug 287179 I do not see regression with bug 104371. And
even without it everything should work, because both nsHTMLEditor.cpp and
nsPlaintextEditor ignore only Alt, Ctrl and Meta modifiers, but accept the Shift
modifier.

As Boris noticed the case comparison code is not ready for higher plane Unicode
characters. The same problem is within these files:
1. nsXBLPrototypeHandler.cpp
2. nsListControlFrame.cpp
3. nsMenuBarFrame.cpp
4. nsMenuPopupFrame.cpp
I will prepare new patch that fixes all those consumers.

BTW nsImageDocument.cpp and nsIsIndexFrame.cpp do not check the modifier keys
when comparing the GetCharCode. Should not they be restricted to case when no
any modifier key is pressed?

Comment 7

[Dainis Jonitis]

Attachment: diff -d -u -15 Fix for non-0 plane character comparison

Comment 8

[Boris Zbarsky [:bzbarsky]]

Erm.  Why are we just casting that PRUint32* to PRUnichar*?  How is that
PRUint32 produced, exactly?  That is, higher-plane characters may require more
than two PRUnichars to be encoded, but they may be encoded in a single PRUint32
using UTF-32 or some such...

Comment 9

[Dainis Jonitis]

I am casting &PRUint32 (and not *PRUint32) to *PRUnichar in assumption that that
PRUint32 is in the same form as PRUnichar strings (but not 0 terminated) and
contains one or two UTF16 code points. If it is plane-0 character then higher
two bytes will be 0. If it is non plane-0 character then all four bytes are
significant.
I believe the DOM events specification should say how this PRUint32 is encoded.
But actually there are no other alternatives to the one described above. In
worst case it can enforce the higher two bytes to be 0 and thus limit to the
plane-0 characters, but it will still be compatible with my code, right?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> in assumption that that PRUint32 is in the same form as PRUnichar strings

I don't believe that's a correct assumption here....

> If it is plane-0 character then higher two bytes will be 0.

Independent of endianness?

Comment 11

[Dainis Jonitis]

> > If it is plane-0 character then higher two bytes will be 0.
> 
> Independent of endianness?

At least old code relied on it - it always casted to PRUnichar to throw away the
higher two bytes. If everything worked in old version on all platforms then it
should work with this patch, too.

Example from old nsXBLPrototypeHandler.cpp:

  mDetail = key[0];  // PRUint32 is initialized with first character of string

and later used:

  if (code != PRUint32(mDetail))  // PRUint32 code compared with PRUnichar
  

If you think that these assumptions are wrong can you suggest some sample code
to look at. It was my first attempt to use Mozilla string data types and it was
best that I can think of.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> it always casted to PRUnichar to throw away the higher two bytes.

Given a PRUint32 n, (PRUnichar)n will throw away the two high bytes.  On the
other hand, ((PRunichar*)&n)[0] (which is what you are saying is equivalent)
will throw away the second two bytes in the memory layout.  Those may be the
high or low bytes, depending on endianness.

In particular, say we are looking at the English character 'A'.  The PRUnichar
for this would be 0x41.  Depending on endianness, the two bytes in it would be
[0x0 0x41] or [0x41 0x0].  Now a PRUint32 representing that same 'A' would also
be 0x41 _as a number_.  The bytes in it would be, depending on endianness,
either [0x0 0x00 0x00 0x41] or [0x41 0x0 0x0 0x0].

In the latter case (little-endian), casting that byte array to PRUnichar* gives
us 0x41 as the fist PRUnichar and 0x0 as the second one.  In the former case
(big-endian), you get 0x0 as the FIRST PRUnichar, and 0x41 as the second one. 
In other words, it looks like the empty string on a big-endian system.

It sound like we have no clear idea (as in, no one ever stopped to think about
it) of how this PRUint32 encodes a character, so it may make the most sense to
just cast it to PRUnichar as the old code did and file a followup bug to sort
out what happens with non-plane-0 stuff.  :(

Comment 13

[Dainis Jonitis]

Then first version of patch should be ok for this temporary solution. Should I
change the review '?' flags back to it again?

About this followup bug. In which component should it be? DOM events?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, if the first patch version does the right thing there, just switch the
flags to it...  And yes, DOM events.

Comment 15

[Dainis Jonitis]

Then I think the best solution is to add the new
  GetCharCode (nsAString& aCharCodeString);

method to the nsDOMKeyboardEvent class in nsDOMKeyboardEvent.cpp.
It will be the same helper method as GetWhich ().
It will allow to hide all endianness details within this method. 

What do you think? If you agree with solution should I still do it in a separate
bug?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Exposed on what interface?

Comment 17

[Dainis Jonitis]

Initially I thought not to expose it to any of interfaces, so that callers
should explicitly cast nsIDOMKeyEvent* to nsDOMKeyboardEvent* before accessing it.
But it could be much more convenient to expose it to the nsIDOMKeyEvent
interface to avoid significant changes in callers. 

nsIDOMKeyEvent.idl does not contain FROZEN keyword. 
1. Does it mean that I can add extra 
  readonly attribute string    charCode
to this file?
2. Should it be the last line to preserve binary compatibility? 
3. Should I change the GUID of interface anyway?

Sorry for too much questions - it's all new for me...

Comment 18

[Boris Zbarsky [:bzbarsky]]

> so that callers should explicitly cast nsIDOMKeyEvent* to nsDOMKeyboardEvent*
> before accessing it.

That's really not a good solution, imo.

> But it could be much more convenient to expose it to the nsIDOMKeyEvent

Unfortunately, that's not a great solution either.  Changes to that interface
need to be pretty carefully thought about (and shouldn't be happening before
1.9, imo, whereas I got the impression that we want this patch for 1.8).

So I'd just file that separate DOM Events bug...

Comment 19

[Dainis Jonitis]

After more investigation seems that GetWhich () is exposed on nsIDOMNSUIEvent
interface. I can expose this new function on this interface then, too. It will
not have sense for Mouse events, but I can add assert for this case. 

Comment 20

[Boris Zbarsky [:bzbarsky]]

The problem is that all the interfaces you're talking about are exposed to
content script; adding new things on them should be done with some care.

Comment 21

[Dainis Jonitis]

Comment on attachment 184318 [details] [diff] [review]
diff -d -u -15

Back to revision 1 of patch. 
I will open a followup bug to handle the non-plane-0 characters.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184318 [details] [diff] [review]
diff -d -u -15

>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>   if (mMisc)
>+  {

Bracing style for this file is:

  if () {
  }
  else {
  }

Please follow that?

>+    match = (ToLowerCase(code) == mDetail);

How about ToLowerCase(PRUnichar(code)) to make it clear what's going on?

sr=bzbarsky with those nits picked.

Comment 23

[Dainis Jonitis]

Attachment: Same as version 1, but with Boris review notes fixed

Comment 24

[Boris Zbarsky [:bzbarsky]]

Dainis, has this landed?  Does this need to land for 1.8?  If so, this should
probably get into 1.8b4 asap....

Comment 25

[Dainis Jonitis]

No, it still waits for review from Neil and is very far in his review queue. 

Comment 26

[[e-mail]]

Comment on attachment 185448 [details] [diff] [review]
Same as version 1, but with Boris review notes fixed

>+  if (mMisc) {
>     aKeyEvent->GetCharCode(&code);
>+    match = (ToLowerCase(PRUnichar(code)) == mDetail);
>+  } else {
>     aKeyEvent->GetKeyCode(&code);
>+    match = (code == PRUint32(mDetail));
>+  }
> 
>+  if (!match)
>     return PR_FALSE;
It seems to me that this can be simplified as follows:
if (!match) =>
if (mMisc ? (ToLowerCase(PRUnichar(code)) != mDetail) : (code !=
PRUint32(mDetail))) =>
if (mMisc ? (PRUint32(ToLowerCase(PRUnichar(code))) != PRUint32(mDetail)) :
(code != PRUint32(mDetail))) =>
if ((mMisc ? PRUint32(ToLowerCase(PRUnichar(code))) : code) !=
PRUint32(mDetail)) =>
if (mMisc)
  code = ToLowerCase(PRUnichar(code));
if (code != PRUint32(mDetail))
which can then be merged back in to the existing block. r=me with this fixed. I
tried this on Windows and was glad to see Caps Lock didn't affect j/J keys for
junk mail :-)

Comment 27

[Dainis Jonitis]

Neil,

You mean something like this:

PRBool
nsXBLPrototypeHandler::KeyEventMatched(nsIDOMKeyEvent* aKeyEvent)
{
  if (mDetail == -1)
    return PR_TRUE; // No filters set up. It's generic.
  
  // Get the keycode or charcode of the key event.
  PRUint32 code;
  
  if (mMisc) {
    aKeyEvent->GetCharCode(&code);
    code = ToLowerCase(PRUnichar(code));
  }
  else
    aKeyEvent->GetKeyCode(&code);
  
  if (code != PRUint32(mDetail))
    return PR_FALSE;
  
  return ModifiersMatchMask(aKeyEvent);
}


Should be ok. Probably even more readable.
Can you then please check in these changes? I do not have CVS access rights. If
you want new patch then let me know.
Thanks in advance!

Comment 28

[Dainis Jonitis]

Attachment: Same as version 1, but with Neil's review notes fixed

Comment 29

[Dainis Jonitis]

Neil was not in CC list, thus I decided to prepare new patch.
Boris, probably you can check this in?
Once this will be checked in, I will open the followup bug that fixes the non
plane-0 characters in endian independent way.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I can check this in on trunk tonight or tomorrow; do you want to try to get this
into 1.8?  Is it safe enough?

Comment 31

[Dainis Jonitis]

It will be great if you check it on trunk. I think it is safe enough for 1.8,
but you better ask Neil.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Neil, does that last patch address your comments?

And do you think this is 1.8 material?

Comment 33

[[e-mail]]

(In reply to comment #32)
>Neil, does that last patch address your comments?
Seems to.

>And do you think this is 1.8 material?
Safe, useful, but not essential.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  If people want this on branch, please request approval.

Comment 35

[Dainis Jonitis]

Opened follow-up bug 306627 to fix non 0-plane character problem.

Comment 36

[Boris Zbarsky [:bzbarsky]]

This caused regression bug 306686 (fixed) and regression bug 307423 (needs fixing).

Comment 37

[Dainis Jonitis]

Probably it is worth to add "any" keyword support for "modifiers" for XUL "key",
like XBL.

Comment 38

[[e-mail]]

Note that that also involves making the shortcuts display correctly in menus
(I guess that means changes to nsMenuX.cpp and nsMenuFrame.cpp).

Comment 39

[Ginn Chen]

Can we turn off the warning?
NS_WARNING("GetCharCode used for wrong key event; should use onkeypress.");

This case is common, a lot of code call it and compare output with 0.
Annoying while debugging.

Comment 40

[Dainis Jonitis]

Sure. Go ahead and comment it out.

Comment 41

[Ginn Chen]

Done.

Comment 42

[timeless]

[e-mail]: you just made a change to /content/ w/o any sign off from a content peer. that's not how things are supposed to be done.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Ginn, please undo the change you made, and do so ASAP.  Checking the charcode of a key event other than keypress is a bug, since the number you get is meaningless.  So if you're hitting this a lot during debugging, file bugs on the code that does it.  That's why we have the warning!  Removing the warning because there's a lot of buggy code that triggers it is very very backwards.

In the future, do NOT check in changes to content without content peer review.  Ever.  Dainis saying "go ahead" does not constitute content peer review.

Comment 44

[Ginn Chen]

Undo-ed. Sorry, all.

Comment 45

[Dainis Jonitis]

Most of these warnings are not from incorrectly written external callers, but from internall callers that need to get all information about key event (both keycode and charcode), like in:
1. content/xbl/src/nsXBLWindowKeyHandler.cpp, WalkHandlers()
2. extension/typeaheadfind/src/nsTypeAheadFind.cpp, KeyPress()
3.layout/forms/nsTextControlFrame.cpp, DOMEventToNativeKeyEvent()

Comment 46

[Boris Zbarsky [:bzbarsky]]

Then we should fix said internal callers -- they're asking for the charcode in cases when the answer is meaningless.

Comment 47

[Boris Zbarsky [:bzbarsky]]

The warnings in GTK builds are due to bug 309316, for what it's worth.