Bug 16735

Created attachment 18278 [details] brief and non-exhaustive test case

(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.

This is a flawed test case. Focus shifts away from the text field when you click buttons.

Oh, never mind. I misunderstood the test case. This should work.

See also related bug 13368.

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.

Created attachment 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.

Uses of hexDigitValue can be replaced by toASCIIHexValue from <wtf/ASCIICType.h>.

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>.

Created attachment 43253 [details] Patch (Work in progress) Updated patch based on Darin's comment. Also, some minor corrections and formatting changes.

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.

(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).

(In reply to comment #11) > Qt: EditorClientQt::handleKeyboardEvent This uses method PlatformKeyboardEvent::qtEvent, PlatformKeyboardEvent::type (**), PlatformKeyboardEvent::windowsVirtualKeyCode, and PlatformKeyboardEvent::text. See (*).

Comment on attachment 43253 [details] Patch (Work in progress) > + 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.

Created attachment 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.

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.

Created attachment 46115 [details] Layout test DRT and manual test case.

Created attachment 46116 [details] Layout test Minor formatting correction.

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.

Arrays, with loops.

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).

(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.

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

*** Bug 48134 has been marked as a duplicate of this bug. ***

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.

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; }

Daniel, any updates on the patch?

(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.

ping and thanks for your work on this as it would be very helpful for a current use case in Chrom{e,ium}

Comment on attachment 46116 [details] Layout test Attachment 46116 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6327915365007360 New failing tests: fast/events/programmatic-keypress-key-events.html

Created attachment 249933 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 46116 [details] Layout test Attachment 46116 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5422889520070656 New failing tests: fast/events/programmatic-keypress-key-events.html

Created attachment 249934 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Is there any update on this? It would be a wonderful fix to have.

(In reply to comment #34) > Is there any update on this? It would be a wonderful fix to have. I plan to work on this bug this weekend.

(In reply to comment #35) > > I plan to work on this bug this weekend. That would be great, Daniel. I'm no expert at the WebKit source code but I'll be happy to help with testing. Please let me know if there is anything I can help with.

Has there been any progress on this issue?

(In reply to comment #37) > Has there been any progress on this issue? We should fix bug #76121 or implement similar functionality to differentiate between a programmatic keyboard event and a non-programmatic keyboard event before fixing this bug.

It's really great to see #76121 has been fixed. Now that the isTrusted property exists, can this issue get revisited? It's an issue near and dear to my heart :) Thanks!