Bug 16735 - keyboard events created with DOM have keyCode and charCode of 0; thus they aren't handled correctly internally
: keyboard events created with DOM have keyCode and charCode of 0; thus they ar...
Status: NEW
: WebKit
HTML DOM
: 528+ (Nightly build)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
: InRadar
:
: 31234
  Show dependency treegraph
 
Reported: 2008-01-04 14:27 PST by
Modified: 2012-07-18 03:34 PST (History)


Attachments
brief and non-exhaustive test case (806 bytes, text/html)
2008-01-04 14:28 PST, Alice Liu
no flags Details
Patch (Work in progress) (50.07 KB, patch)
2009-11-14 21:08 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Patch (Work in progress) (49.62 KB, patch)
2009-11-15 14:35 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Patch (Work in progress) (49.59 KB, patch)
2009-11-25 18:19 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Layout test (3.77 KB, patch)
2010-01-07 23:50 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Layout test (3.76 KB, patch)
2010-01-07 23:52 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-01-04 14:27:38 PST
<rdar://problem/5671005>

calling initKeyboardEvent with a keyIdentifier other than tab doesn't seem to work. 

see test case

* STEPS TO REPRODUCE
1. open attached test case
2. press the various buttons in the test case.  the buttons should make the indicated key event happen in the text area. 

* RESULTS
currently none of these work on Mac.  Strangely the "tab" one does work on Windows.
------- Comment #1 From 2008-01-04 14:28:30 PST -------
Created an attachment (id=18278) [details]
brief and non-exhaustive test case
------- Comment #2 From 2008-01-04 14:44:57 PST -------
(In reply to comment #0)
> currently none of these work on Mac.

I think this could be because focus is on a button when clicking - perhaps setTimeout() could result in a different behavior.
------- Comment #3 From 2008-01-04 14:52:32 PST -------
This is a flawed test case.  Focus shifts away from the text field when you click buttons.
------- Comment #4 From 2008-01-04 14:53:04 PST -------
Oh, never mind. I misunderstood the test case.  This should work.
------- Comment #5 From 2008-01-04 16:37:07 PST -------
See also related bug 13368.
------- Comment #6 From 2008-01-04 16:43:08 PST -------
Alexey already understands this but I thought I'd be specific here.

Events created with initKeyboardEvent are not "realistic enough" because they always have a keyCode of 0 and charCode of 0. So any code that looks at keyCode or charCode, including code inside the engine that responds to keyboard events and older legacy JavaScript code, won't work with these events.

To simulate a key press we need to send a keydown event, a keypress event, and a keyup event. It's not clear what the type should be for the keypress event.
------- Comment #7 From 2009-11-14 21:08:58 PST -------
Created an attachment (id=43240) [details]
Patch (Work in progress)

Work in progress.

Possible fix for this issue.

Tested only on the Mac build (at this time). Also, neither modified PlatformKeyboardEventChromium.cpp nor included a test case.

My thought was to create a reverse mapping from keyIdentifier to charCode, see KeyboardEvent::keyIdentifierList(). Modified KeyboardEvent to fall back on method KeyboardEvent::charCodeForKeyIdentifier if KeyboardEvent::m_keyEvent == null (i.e. this event wasn't generated by a physical key press - hence no platform-specific event).

Since some these charCodes are platform-specific, method KeyboardEvent::charCodeForKeyIdentifier first calls PlatformKeyboardEvent::keyIdentifierList() to get any platform-specific mappings. Otherwise, we fall back on KeyboardEvent::keyIdentifierList(), parse printable ASCII characters or parse Unicode code points (of the form U+000D).

I am open to suggestions on this patch and approaches to resolve this issue, if we choose to do so.
------- Comment #8 From 2009-11-15 14:09:46 PST -------
Uses of hexDigitValue can be replaced by toASCIIHexValue from <wtf/ASCIICType.h>.
------- Comment #9 From 2009-11-15 14:24:43 PST -------
Will update the patch. Do you have any suggestions on this patch? In particular, is this a reasonable approach?

(In reply to comment #8)
> Uses of hexDigitValue can be replaced by toASCIIHexValue from
> <wtf/ASCIICType.h>.
------- Comment #10 From 2009-11-15 14:35:47 PST -------
Created an attachment (id=43253) [details]
Patch (Work in progress)

Updated patch based on Darin's comment.

Also, some minor corrections and formatting changes.
------- Comment #11 From 2009-11-15 14:41:20 PST -------
It makes sense to have mappings from key identifiers to key code and character code. Ideally the data could be shared by the PlatformKeyEvent code for the various platforms that has to map keys to key identifiers, key codes, and character codes. There may need to be some exceptions to get every last key to work correcty, but for the most part the same data table should be able to be used both to help in the mapping of a platform's keys to key identifiers, key codes, and character codes, and to map from a key identifier to a key code and character code.

I also think the direction we should go in is to compute and store the key code and the character code in the KeyboardEvent object rather than dynamically fetching them from the PlayformKeyboardEvent when asked.

After construction/init time, the PlatformKeyboardEvent would be used only in cases where it must. Specifically that means only in the keyEvent function, which should be renamed underlyingPlatformEvent to make it seem less "normal" to dig down to that level. Any code that does must have a good reason.

(Besides renaming m_keyEvent to m_underlyingPlatformEvent, we should use an OwnPtr.)

To investigate how this is used I took at look at all the call sites. There were three categories:

    1) When sending events to plug-ins. This is probably something we should probably keep longer term. It's asking a lot for plug-ins to be able to handle events created by the DOM. Would probably require one of the newer plug-in architectures to do it cleanly.

    2) When mapping events to editing commands on Mac OS X in -[WebHTMLView _interceptEditingKeyEvent:shouldSaveCommands:], because the AppKit API in question needs an NSEvent. This means that any commands that work through the Mac OS X editing key bindings won't work with DOM events. That includes a lot of basic stuff, and would be good to try to address this by creating an appropriate NSEvent if there is no underlying event already. Perhaps there there would be problems where the NSEvent would cause input methods to malfunction if we had a key down without a key up or malformed in other ways. We also would have to consider whether to cache the NSEvent we created.

    3) When handling events in analogous functions to (2) on other platforms. These various functions mostly started as copies of the Win code or have a similar function:

        Chromium: WebKeyboardEventBuilder::WebKeyboardEventBuilder,
            EditorClientImpl::interpretKeyEvent and
            EditorClientImpl::handleEditingKeyboardEvent
        GTK: EditorClient::handleKeyboardEvent and
            EditorClient::handleInputMethodKeydown
        Haiku: EditorClientHaiku::handleKeyboardEvent
        Qt: EditorClientQt::handleKeyboardEvent
        Win: WebView::handleEditingKeyboardEvent
        Wx: EditorClientWx::handleEditingKeyboardEvent and
            EditorClientWx::interpretKeyEvent

    I can't tell if any of these non-Mac-OS-X functions really need any data that comes only from the native event, the way the Mac OS X code path does. It seems likely they do things this way largely because they were inspired by the Mac OS X code. I suspect we could probably do all of this in a way that would work with synthetic DOM events instead of requiring actual platform events. Probably an easier project than (2) above where we'd have to work with the Mac OS X editing key mapping machinery.

I'd love to fix first (3) and then (2) so we could do any non-plug-in action with a DOM keyboard event. I'm not saying all that should or could be covered by this one bug report.

To muddy the waters further, I'm not sure the version of keyboard event init function and keyboard event class we have here matches the DOM Level 3 standard any longer. We may have to do some work to keep existing callers working and make more modern code work as well. It seems to me that if keyCode and charCode are part of the object, then the constructor should simply take in values for those.
------- Comment #12 From 2009-11-18 14:26:26 PST -------
(In reply to comment #11)
> It makes sense to have mappings from key identifiers to key code and character
> code. Ideally the data could be shared by the PlatformKeyEvent code for the
> various platforms that has to map keys to key identifiers, key codes, and
> character codes. There may need to be some exceptions to get every last key to
> work correcty, but for the most part the same data table should be able to be
> used both to help in the mapping of a platform's keys to key identifiers, key
> codes, and character codes, and to map from a key identifier to a key code and
> character code.

I agree, it would be ideal if we could share the same lookup table. I made an attempt to do so by categorizing the platform-independent keyIdentifier in KeyboardEvents::keyIdentifiers() and the platform-dependent ones in PlatformKeyboardEvents::keyIdentifiers(). And the way I structured the lookup, by looking up an identifier first in the platform-specific table, allows for there to be platform-specific exceptions to the otherwise platform-independent table.

> I also think the direction we should go in is to compute and store the key code
> and the character code in the KeyboardEvent object rather than dynamically
> fetching them from the PlayformKeyboardEvent when asked.

(A) These seems like a separate issue from this bug or would be better addressed in a separate bug. Some of the "platform-specific" aspects may in actuality be platform-independent (i.e. can be moved into KeyboardEvents). I'll look into this some more.

> After construction/init time, the PlatformKeyboardEvent would be used only in
> cases where it must. Specifically that means only in the keyEvent function,
> which should be renamed underlyingPlatformEvent to make it seem less "normal"
> to dig down to that level. Any code that does must have a good reason.

Right. I think this will naturally follow from addressing (A), and is probably best addressed in a separate bug.

> (Besides renaming m_keyEvent to m_underlyingPlatformEvent, we should use an
> OwnPtr.)

I have filed bug #31647 to change KeyboardEvent::m_keyEvent to use an OwnPtr.

> To investigate how this is used I took at look at all the call sites. There
> were three categories:
> 
>     1) When sending events to plug-ins. This is probably something we should
> probably keep longer term. It's asking a lot for plug-ins to be able to handle
> events created by the DOM. Would probably require one of the newer plug-in
> architectures to do it cleanly.
> 
>     2) When mapping events to editing commands on Mac OS X in -[WebHTMLView
> _interceptEditingKeyEvent:shouldSaveCommands:], because the AppKit API in
> question needs an NSEvent. This means that any commands that work through the
> Mac OS X editing key bindings won't work with DOM events. That includes a lot
> of basic stuff, and would be good to try to address this by creating an
> appropriate NSEvent if there is no underlying event already. Perhaps there
> there would be problems where the NSEvent would cause input methods to
> malfunction if we had a key down without a key up or malformed in other ways.
> We also would have to consider whether to cache the NSEvent we created.
> 
>     3) When handling events in analogous functions to (2) on other platforms.
> These various functions mostly started as copies of the Win code or have a
> similar function:

For the following, I have listed only the methods that explicitly call functionality defined in an instance of PlatformKeyboardEvent.

> 
>         Chromium: WebKeyboardEventBuilder::WebKeyboardEventBuilder,

This uses methods PlatformKeyboardEvent::text, PlatformKeyboardEvent::unmodifiedText, and PlatformKeyboardEvent::nativeVirtualKeyCode.

>             EditorClientImpl::interpretKeyEvent and

This seems that it can be re-written without the use of PlatformKeyboardEvent. See (*).

>             EditorClientImpl::handleEditingKeyboardEvent

This uses methods PlatformKeyboardEvent::text, and PlatformKeyboardEvent::isSystemKey. See (*).

>         GTK: EditorClient::handleKeyboardEvent and

This uses method PlatformKeyboardEvent::text, and PlatformKeyboardEvent::type (**).

>             EditorClient::handleInputMethodKeydown

This method is empty. It only has the comment "We handle IME within chrome".

>         Haiku: EditorClientHaiku::handleKeyboardEvent

This uses method PlatformKeyboardEvent::windowsVirtualKeyCode, PlatformKeyboardEvent::type (**), and PlatformKeyboardEvent::text. See (*).

>         Qt: EditorClientQt::handleKeyboardEvent
>         Win: WebView::handleEditingKeyboardEvent

This uses methods PlatformKeyboardEvent::text, PlatformKeyboardEvent::isSystemKey, and PlatformKeyboardEvent::type (**).

>         Wx: EditorClientWx::handleEditingKeyboardEvent and

This uses methods PlatformKeyboardEvent::text and PlatformKeyboardEvent::type (**).

>             EditorClientWx::interpretKeyEvent

This uses method PlatformKeyboardEvent::type (**).

(*) Calls PlatformKeyboardEvent::altKey(), PlatformKeyboardEvent::ctrlKey(), PlatformKeyboardEvent::metaKey(), or PlatformKeyboardEvent::shiftKey(). But these seem unnecessary on the surface, since we may able to substitute them for the respective KeyboardEvent calls, which are inherited from UIEventWithKeyState.

(**) We may able to substitute PlatformKeyboardEvent::type for KeyboardEvent::type, which is inherited from Event (note: KeyboardEvents extends UIEventWithKeyState extends UIEvent extends Event).

> 
>     I can't tell if any of these non-Mac-OS-X functions really need any data
> that comes only from the native event, the way the Mac OS X code path does. It
> seems likely they do things this way largely because they were inspired by the
> Mac OS X code. I suspect we could probably do all of this in a way that would
> work with synthetic DOM events instead of requiring actual platform events.
> Probably an easier project than (2) above where we'd have to work with the Mac
> OS X editing key mapping machinery.
> 
> I'd love to fix first (3) and then (2) so we could do any non-plug-in action
> with a DOM keyboard event. I'm not saying all that should or could be covered
> by this one bug report.

Right. I think it would be best if we address these issues in separate bugs.

> To muddy the waters further, I'm not sure the version of keyboard event init
> function and keyboard event class we have here matches the DOM Level 3 standard
> any longer. We may have to do some work to keep existing callers working and
> make more modern code work as well. It seems to me that if keyCode and charCode
> are part of the object, then the constructor should simply take in values for
> those.

Yes, I am aware that our API differs from the DOM Level 3 standard. My thought was to first fix this bug then update our API to match the spec (which is bug #13368).
------- Comment #13 From 2009-11-18 14:33:45 PST -------
(In reply to comment #11)
>         Qt: EditorClientQt::handleKeyboardEvent

This uses method PlatformKeyboardEvent::qtEvent, PlatformKeyboardEvent::type (**), PlatformKeyboardEvent::windowsVirtualKeyCode, and PlatformKeyboardEvent::text. See (*).
------- Comment #14 From 2009-11-18 16:45:20 PST -------
(From update of attachment 43253 [details])
> +    int firstChar = keyIdentifier.characterStartingAt(0);

The type is UChar32, not int.

> +    if (keyIdentifier.length() == 1 && isASCIIPrintable(firstChar))
> +        return static_cast<unsigned>(firstChar);
> +    if (keyIdentifier.length() == 1)
> +        return 0; // Non-printable ASCII character.

Nicer to combine these two with a nested if. Also best to put the length into a local variable since we use it multiple times.

> +    if (keyIdentifier.length() == (2 /* "U+" */ + numUnicodeCodePointHexDigits) && keyIdentifier.substring(0, 2) == "U+") {

It's much more efficient to index twice or call startsWith than to actually compute a substring.

    keyIdentifier[0] == 'U' || keyIdentifier[1] == '+'

> +        const UChar* p = keyIdentifier.characters();
> +        p += 2; // Skip over "U+", so that we point to the first hex digit.

Seems these should go into one line.

> +        unsigned unicodeValue = 0;

This should have the type UChar.

> +        unsigned numHexDigits = numUnicodeCodePointHexDigits;
> +        while (numHexDigits--)
> +            unicodeValue = unicodeValue << 4 | toASCIIHexValue(*(p++));      

Strange trailing spaces here.

Seems tom me that for four digits it would be cleaner to not write this as a loop.

> +        return isValidCharCode(unicodeValue)? unicodeValue : 0;

We use a space before the "?" in these kinds of expression.

> +        return m_keyEvent? m_keyEvent->windowsVirtualKeyCode() : keyCodeForKeyIdentifier(m_keyIdentifier);

Here too.

> +    return m_keyEvent? static_cast<int>(m_keyEvent->text().characterStartingAt(0)) : charCodeForKeyIdentifier(m_keyIdentifier);

And here.
------- Comment #15 From 2009-11-25 18:19:38 PST -------
Created an attachment (id=43887) [details]
Patch (Work in progress)

This patch is a work in progress.

Updated patch based on Darin's remarks in <https://bugs.webkit.org/show_bug.cgi?id=16735#c14>.

I just wanted to post what I have so far. I'm still working on this.
------- Comment #16 From 2009-12-08 18:42:06 PST -------
I just want to say that I have not forgotten about this bug. I am going to try to work on this some more later this week/weekend.
------- Comment #17 From 2010-01-07 23:50:39 PST -------
Created an attachment (id=46115) [details]
Layout test

DRT and manual test case.
------- Comment #18 From 2010-01-07 23:52:19 PST -------
Created an attachment (id=46116) [details]
Layout test

Minor formatting correction.
------- Comment #19 From 2010-01-08 08:21:09 PST -------
I suggest using arrays instead of runs of function calls to initialize the maps. This helps reduce the need for a comment on each line, and usually ends up with smaller code size as well.
------- Comment #20 From 2010-01-08 08:21:20 PST -------
Arrays, with loops.
------- Comment #21 From 2010-03-18 12:24:25 PST -------
I did not quite follow the whole discussion or dig into the code, but is it foreseen that dispatching a "keydown" also fires the corresponding "keypress" if the keyIdentifier warrants it (if a visible character is typed as a result of the keydown). The standard does not foresee to be able to dispatch "keypress" as a separate event, but one should result in the other (if preventDefault is not called from a keyDown handler).
------- Comment #22 From 2010-03-18 13:26:02 PST -------
(In reply to comment #21)
> I did not quite follow the whole discussion or dig into the code, but is it
> foreseen that dispatching a "keydown" also fires the corresponding "keypress"
> if the keyIdentifier warrants it (if a visible character is typed as a result
> of the keydown). The standard does not foresee to be able to dispatch
> "keypress" as a separate event, but one should result in the other (if
> preventDefault is not called from a keyDown handler).

Sounds logical but:

    1) Not really the same thing as this bug, so not clear it should be discussed here.

    2) Does not match other browsers’ behavior so could lead to incompatibilities.

We tried a while back to have keydown events dispatch keypress events in their default event handlers but found this is not how any other browser behaves. Lets discuss this elsewhere, though. There’s already a lot of discussion here.
------- Comment #23 From 2010-03-19 06:55:39 PST -------
I based my comment on reading an older version of the standard, where seemingly "keypress" was not (supposed to be) availabe as a separate creatable event. 

The latest DOM-L3 draft does describe (but deprecates) keypress in interface KeyboardEvent (http://www.w3.org/TR/DOM-Level-3-Events/#events-keyboardevents).

I created Bug 36364 for further discussion
------- Comment #24 From 2010-10-22 10:15:36 PST -------
*** Bug 48134 has been marked as a duplicate of this bug. ***
------- Comment #25 From 2010-12-06 01:52:32 PST -------
I took this patch and it didn't work out directly for me (am using qt port).I made some additional changes and it works.

1.Inside "initKeyBoardEvent" created a new PlatformKeyBoardEvent(set it to m_keyEvent).
2. Use the "charCodeForKeyIdentifier" to get the char code of the keyIdentifier.Used this charCode to get the windowsKeyCode and set the value of m_windowsVirtualKeyCode with this key code.
3. In "EditorClientQt::handleKeyboardEvent" added 2 extra cases to handle home and end key as follows:-
        case VK_HOME:
            frame->editor()->command("MoveToBeginningOfLine").execute();
        case VK_END:
            frame->editor()->command("MoveToEndOfLine").execute();
4. For "A" key, used the key code to set the m_text. (for "!" can we use the char code??)

Let me know your feedback on the changes.
------- Comment #26 From 2010-12-16 05:24:43 PST -------
Found an another way (think better then previous one) :)

1.In "EditorClientQt::handleKeyboardEvent" all the operation is based on PlatformKeyboardEvent. But for the case of events with "initKeyboardEvent" we normally don't have the required one.So used the keycode corresponding to the "KeyboardEvent" :-
    //Don't have a platformkeyboardevent(case of initKeyboardEvent.)
    //For such cases check for any assoaciated keycode
    if(!kevent) {
        switch(event->keyCode()){
            case VK_END:
                frame->editor()->command("MoveToEndOfLine").execute();
                return;
            case VK_HOME:
                frame->editor()->command("MoveToBeginningOfLine").execute();
                return;
            default:
                if(equalIgnoringCase(event->type(), "KeyDown")) {
                    String str = event->stringForKeyEvent();
                    if(!str.isEmpty())
                        frame->editor()->insertText(str, event);
                }
                return;
        }
    }
2.Implemented the "keyCodeForKeyIdentifier" (earlier returning 0)
static unsigned keyCodeForKeyIdentifier(const String& keyIdentifier)
{
    return windowsKeyCodeForKeyEvent(charCodeForKeyIdentifier(keyIdentifier));
}
3.Modified the keyIdentifierList as follows
    original:- keyIdentifierList.set("Exclamation", '!');
    Modified:- keyIdentifierList.set("U+0021", '!');
4.Added a new method in "KeyboardEvent" to return the corresponding string:-
String KeyboardEvent::stringForKeyEvent()
{
    String eventString;

    //Find the string in the keyidentifier list
    HashMap<String, unsigned>::iterator itt = keyIdentifierList()->find(m_keyIdentifier);
    if (itt != keyIdentifierList()->end())
        eventString.append(UChar(itt->second));

    //Now check if the keycode represents any printable character
    if(eventString.isEmpty() && PlatformKeyboardEvent::isVirtualKeyCodeRepresentingCharacter(keyCode()))
        eventString.append(UChar(keyCode()));

    return eventString;
}
------- Comment #27 From 2011-06-15 19:19:17 PST -------
Daniel, any updates on the patch?
------- Comment #28 From 2011-06-16 08:52:10 PST -------
(In reply to comment #27)
> Daniel, any updates on the patch?

As it turns out I haven't had the chance to come back to this bug. It's on my todo list. That being said, feel free to work on this.
------- Comment #29 From 2012-03-09 15:59:01 PST -------
ping and thanks for your work on this as it would be very helpful for a current use case in Chrom{e,ium}