Bug 202183 – [Win] Update KeyboardEvent as per the latest specification

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win] Update KeyboardEvent as per the latest specification

MDN: KeyboardEvent
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent

W3C: UI Events
https://www.w3.org/TR/uievents/#events-keyboardevents

Bug 162852 – [Meta] Update KeyboardEvent as per the latest specification
Bug 166759 – [GTK] Should support key and code properties on keyboard events

[Fujii Hironori]

UI Events KeyboardEvent code Values
https://www.w3.org/TR/uievents-code/

UI Events KeyboardEvent key Values
https://www.w3.org/TR/uievents-key/

KeyboardEvent: code values - Web APIs | MDN
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code/code_values

Key Values - Web APIs | MDN
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values

[Fujii Hironori]

Windows WM_KEYDONW has only key code, but charactor information which is needed for KeyboardEvent.key.
In Chromium, PlatformKeyMap::DomKeyFromKeyboardCodeImpl converts a key code to a charactor code by using the current keyboard layout information.
PlatformKeyMap::UpdateLayout builds mapping table by using ToUnicodeEx.

Issue 1585193002: Build key map DomCodeToKey() for Windows - Code Review
https://codereview.chromium.org/1585193002

[Fujii Hironori]

Firefox implementations looks quite complicated. I can't understand them.

Firefox also has the similer code to build a mapping table by using ToUnicodeEx in KeyboardLayout::LoadLayout.
But, I can't understand how this table is used.

It seems that Firefox is mapping a key code by using the following WM_CHAR events for printable keys,
and a hard-coded table for non-printable keys.

NativeKey::GetFollowingCharMessage retrieves queued WM_CHAR messages for printable keys.

KeyboardLayout::ConvertNativeKeyCodeToKeyNameIndex is converting non-printable keys.
https://dxr.mozilla.org/mozilla-central/source/widget/NativeKeyToDOMKeyName.h is the hard-coded table.

842927 - Implement DOM3 KeyboardEvent.key only for non-printable key, first
https://bugzilla.mozilla.org/show_bug.cgi?id=842927

[Fujii Hironori]

I think this problem can be solved more simply.
The problem is:
- TranslateMessage is using ToUnicodeEx
- ToUnicodeEx stores a previous dead key
- extra calling ToUnicodeEx consumes the stored dead key

For example, typing '`' and 'a':

- Types '`'
- The first calling ToUnicodeEx from TranslateMessage stores '`' as a dead key
- Types 'a'
- The second calling ToUnicodeEx from TranslateMessage returns 'à' as expected

The problem happens if ToUnicodeEx is used after calling TranslateMessage

- Types '`'
- The first calling ToUnicodeEx from TranslateMessage stores '`' as a dead key
- Extra calling ToUnicodeEx with '`' consumes the stored dead key, and return '``'
- Types 'a'
- The second calling ToUnicodeEx from TranslateMessage unexpectedly returns 'a'

This problem can be solved by one more extra calling ToUnicodeEx with VK_SPACE.

- Types '`'
- The first calling ToUnicodeEx from TranslateMessage stores '`' as a dead key
- ToUnicodeEx(VK_SPACE) consums the dead key
- ToUnicodeEx('`') stores a dead key again
- Types 'a'
- The second calling ToUnicodeEx from TranslateMessage returns 'à' as expected

https://en.wikipedia.org/wiki/Dead_key

[Fujii Hironori]

Created attachment 380407 [details]
manual test to show key and code properties of keyboard events

[Fujii Hironori]

I tested Firefox and Chrome on Windows by typing ^ and a with German keyboard.
And, marked the differences with ★.

Firefox 69 for Windows

type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:â code:KeyA
type:keypress key:â code:KeyA
type:keyup key:a code:KeyA
type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:^^ code:Backquote ★
type:keypress key:^ code:Backquote
type:keypress key:^ code:Backquote
type:keyup key:^ code:Backquote ★

Chrome 77 for Windows

type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:â code:KeyA
type:keypress key:â code:KeyA
type:keyup key:a code:KeyA
type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:Dead code:Backquote ★
type:keypress key:^ code:Backquote
type:keypress key:^ code:Backquote
type:keyup key:Dead code:Backquote ★

[Fujii Hironori]

(In reply to Fujii Hironori from comment #6)
> I tested Firefox and Chrome on Windows by typing ^ and a with German

Oops. I typed [^] [a] [^] [^].

[Fujii Hironori]

comment 4 is wrong.
If I type [^][^][a].
It should be "^^a", but it becomes "^^â" because the second [^] also stores a dead key.

[Fujii Hironori]

If I set bit 2 of wFlags for ToUnicodeEx, it seems that ToUnicodeEx doesn't change the stored dead key.

https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-tounicodeex
> If bit 2 is set, keyboard state is not changed (Windows 10, version 1607 and newer)

Unfortunately, this behavior is supported only by Windows 10, version 1607 and newer.

And, it's not obvious to me how should I use this flag.
For example, If I type [^][a], TranslateMessage of [a] consumes the stored dead key.
So, calling ToUnicodeEx with [a] and the flag returns "a" rather than "â" because no dead key is stored.
Should I use ToUnicodeEx before TranslateMessage? Or, shouldn't I use TranslateMessage?

[Fujii Hironori]

In Chromium, InputMethodWinBase::DispatchKeyEvent retrieves queued WM_CHAR events, and sets a char information from WM_CHAR to the keydown event.

Callstack:

> content.dll!ui::MakeWebKeyboardEventFromUiEvent(const ui::KeyEvent & event) Line 115	C++
> content.dll!ui::MakeWebKeyboardEvent(const ui::KeyEvent & event) Line 332	C++
> content.dll!content::NativeWebKeyboardEvent::NativeWebKeyboardEvent(const ui::KeyEvent & key_event, wchar_t character) Line 132	C++
> content.dll!content::RenderWidgetHostViewAura::InsertChar(const ui::KeyEvent & event) Line 1270	C++
> ui_base_ime_win.dll!ui::InputMethodWinBase::OnChar(HWND__ * window_handle, unsigned int message, unsigned __int64 wparam, __int64 lparam, const tagMSG & event, int * handled) Line 294	C++
> ui_base_ime_win.dll!ui::InputMethodWinBase::ProcessUnhandledKeyEvent(ui::KeyEvent * event, const std::__1::vector<tagMSG,std::__1::allocator<tagMSG>> * char_msgs) Line 503	C++
> ui_base_ime_win.dll!ui::InputMethodWinBase::DispatchKeyEvent(ui::KeyEvent * event) Line 255	C++
> aura.dll!aura::WindowEventDispatcher::PreDispatchKeyEvent(ui::KeyEvent * event) Line 1036	C++
> aura.dll!aura::WindowEventDispatcher::PreDispatchEvent(ui::EventTarget * target, ui::Event * event) Line 535	C++
> events.dll!ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget * target, ui::Event * event) Line 55	C++
> events.dll!ui::EventProcessor::OnEventFromSource(ui::Event * event) Line 49	C++
> events.dll!ui::EventSource::DeliverEventToSink(ui::Event * event) Line 112	C++
> events.dll!ui::EventSource::SendEventToSinkFromRewriter(const ui::Event * event, const ui::EventRewriter * rewriter) Line 137	C++
> events.dll!ui::EventSource::SendEventToSink(const ui::Event * event) Line 106	C++
> views.dll!views::DesktopWindowTreeHostWin::HandleKeyEvent(ui::KeyEvent * event) Line 934	C++
> views.dll!views::HWNDMessageHandler::OnKeyEvent(unsigned int message, unsigned __int64 w_param, __int64 l_param) Line 1912	C++
> views.dll!views::HWNDMessageHandler::_ProcessWindowMessage(HWND__ * hWnd, unsigned int uMsg, unsigned __int64 wParam, __int64 lParam, __int64 & lResult, unsigned long dwMsgMapID) Line 380	C++
> views.dll!views::HWNDMessageHandler::OnWndProc(unsigned int message, unsigned __int64 w_param, __int64 l_param) Line 1024	C++
> gfx.dll!gfx::WindowImpl::WndProc(HWND__ * hwnd, unsigned int message, unsigned __int64 w_param, __int64 l_param) Line 298	C++
> gfx.dll!base::win::WrappedWindowProc<&gfx::WindowImpl::WndProc>(HWND__ * hwnd, unsigned int message, unsigned __int64 wparam, __int64 lparam) Line 75	C++
> [External Code]	
> base.dll!base::MessagePumpForUI::ProcessMessageHelper(const tagMSG & msg) Line 476	C++
> base.dll!base::MessagePumpForUI::ProcessNextWindowsMessage() Line 445	C++
> base.dll!base::MessagePumpForUI::DoRunLoop() Line 212	C++
> base.dll!base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate) Line 76	C++
> base.dll!base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(bool application_tasks_allowed, base::TimeDelta timeout) Line 467	C++
> base.dll!base::RunLoop::Run() Line 156	C++
> chrome.dll!ChromeBrowserMainParts::MainMessageLoopRun(int * result_code) Line 1854	C++
> content.dll!content::BrowserMainLoop::RunMainMessageLoopParts() Line 1026	C++
> content.dll!content::BrowserMainRunnerImpl::Run() Line 149	C++
> content.dll!content::BrowserMain(const content::MainFunctionParams & parameters) Line 47	C++
> content.dll!content::RunBrowserProcessMain(const content::MainFunctionParams & main_function_params, content::ContentMainDelegate * delegate) Line 556	C++
> content.dll!content::ContentMainRunnerImpl::RunServiceManager(content::MainFunctionParams & main_params, bool start_service_manager_only) Line 963	C++
> content.dll!content::ContentMainRunnerImpl::Run(bool start_service_manager_only) Line 871	C++
> content.dll!content::ContentServiceManagerMainDelegate::RunEmbedderProcess() Line 52	C++
> embedder.dll!service_manager::Main(const service_manager::MainParams & params) Line 423	C++
> content.dll!content::ContentMain(const content::ContentMainParams & params) Line 20	C++
> chrome.dll!ChromeMain(HINSTANCE__ * instance, sandbox::SandboxInterfaceInfo * sandbox_info, __int64 exe_entry_point_ticks) Line 110	C++
> chrome.exe!MainDllLoader::Launch(HINSTANCE__ * instance, base::TimeTicks exe_entry_point_ticks) Line 202	C++
> chrome.exe!wWinMain(HINSTANCE__ * instance, HINSTANCE__ * prev, wchar_t *, int) Line 234	C++
> [External Code]	

Issue 1267483003: Combine the WM_CHAR with WM_KEY* for key event flow. - Code Review
https://codereview.chromium.org/1267483003

[Fujii Hironori]

I typed [^][a][^z][^][^] in German keyboard.

Firefox 69 for Windows

type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:â code:KeyA
type:keypress key:â code:KeyA
type:keyup key:a code:KeyA
type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:^y code:KeyZ
type:keypress key:^ code:KeyZ
type:keypress key:y code:KeyZ
type:keyup key:y code:KeyZ
type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:^^ code:Backquote
type:keypress key:^ code:Backquote
type:keypress key:^ code:Backquote
type:keyup key:^ code:Backquote


Chrome 77 for Windows

type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:â code:KeyA
type:keypress key:â code:KeyA
type:keyup key:a code:KeyA
type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:y code:KeyZ
type:keypress key:^ code:KeyZ
type:keypress key:y code:KeyZ
type:keyup key:y code:KeyZ
type:keydown key:Dead code:Backquote
type:keyup key:Dead code:Backquote
type:keydown key:Dead code:Backquote
type:keypress key:^ code:Backquote
type:keypress key:^ code:Backquote
type:keyup key:Dead code:Backquote


Microsoft Edge 44.18362.387.0 (Microsoft EdgeHTML 18.18362)

type:keydown key:Unidentified code:undefined
type:keyup key:Unidentified code:undefined
type:keydown key:â code:undefined
type:keypress key:â code:undefined
type:keyup key:â code:undefined
type:keydown key:Unidentified code:undefined
type:keyup key:Unidentified code:undefined
type:keydown key:^y code:undefined
type:keypress key:^y code:undefined
type:keyup key:^y code:undefined
type:keydown key:Unidentified code:undefined
type:keyup key:Unidentified code:undefined
type:keydown key:^^ code:undefined
type:keypress key:^^ code:undefined
type:keyup key:^^ code:undefined

[Fujii Hironori]

(In reply to Fujii Hironori from comment #11)
(...)
> type:keydown key:^y code:KeyZ
(...)
> type:keydown key:^y code:undefined
> type:keypress key:^y code:undefined
> type:keyup key:^y code:undefined

Firefox and Edge set two base characters for 'key' property.
This seems incompatible with the spec.

https://www.w3.org/TR/uievents-key/#keys-unicode

> A key string is a string containing a 0 or 1 non-control
> characters ("base" characters) followed by 0 or more combining
> characters. The string MUST be in Normalized Form C (NFC) as
> described in [UnicodeNormalizationForms].

[Fujii Hironori]

I tested on macOS with
https://javascript.info/keyboard-events

Chrome 77 on macOS 10.14

keydown key=Dead code=Equal
keyup key=Dead code=Equal
--------------------------------------------------------------------------------
keydown key=á code=KeyA
keyup key=a code=KeyA
--------------------------------------------------------------------------------
keydown key=Dead code=Equal
keyup key=Dead code=Equal
--------------------------------------------------------------------------------
keydown key=y code=KeyZ
keyup key=y code=KeyZ
--------------------------------------------------------------------------------
keydown key=Dead code=Equal
keyup key=Dead code=Equal
--------------------------------------------------------------------------------
keydown key=Dead code=Equal
keyup key=Dead code=Equal


Safari Technology Preview 93 on macOS 10.14

keydown key=Dead code=Equal
keyup key=´ code=Equal
--------------------------------------------------------------------------------
keydown key=á code=KeyA
keyup key=a code=KeyA
--------------------------------------------------------------------------------
keydown key=Dead code=Equal
keyup key=´ code=Equal
--------------------------------------------------------------------------------
keydown key=´y code=KeyZ
keyup key=y code=KeyZ
--------------------------------------------------------------------------------
keydown key=Dead code=Equal
keyup key=´ code=Equal
--------------------------------------------------------------------------------
keydown key=´ code=Equal
keyup key=´ code=Equal

[Fujii Hironori]

Created attachment 382570 [details]
WIP patch

[Fujii Hironori]

Created attachment 382816 [details]
WIP patch

[Fujii Hironori]

Created attachment 383018 [details]
WIP patch

[Fujii Hironori]

Created attachment 383338 [details]
WIP patch

[Fujii Hironori]

Created attachment 383438 [details]
WIP patch

[Fujii Hironori]

Created attachment 383542 [details]
Patch

[Fujii Hironori]

review?

[Fujii Hironori]

review?

[Ross Kirsling]

Comment on attachment 383542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383542&action=review

> Source/WebCore/platform/win/WindowsKeyNames.cpp:39
> +typedef enum : uint8_t {
> +    EF_NONE = 0,
> +    EF_SHIFT_DOWN = 1 << 0,
> +    EF_CONTROL_DOWN = 1 << 1,
> +    EF_ALT_DOWN = 1 << 2,
> +    EF_CAPS_LOCK_ON = 1 << 3,
> +} EventFlags;

Might be neat to use OptionSet here...unless the goal is to keep this file as similar as possible to the Chromium copy, in which case it's fine as-is.

> Source/WebCore/platform/win/WindowsKeyNames.cpp:209
> +            return nullString();

Interesting, I didn't realize this existed.
There are only 5 instances of `return nullString();` in Source/ compared to 731 of `return String();`...
I guess it was introduced early this year for JSStrings: https://github.com/WebKit/webkit/commit/cb663b3dd21a5b9fbdd180625406226f851a1c7e

According to Yusuke on IRC: "nullString is used almost only when you want `const String&` allocated in somewhere."

> Source/WebCore/platform/win/WindowsKeyNames.cpp:311
> +
> +

Nit: extra newline

> Source/WebCore/platform/win/WindowsKeyNames.cpp:341
> +    } domKeyMap[] = {

Seems odd that domKeyMap is defined *inside* domCodeFromLParam while cNonPrintableKeyMap is defined *outside* of nonPrintableVirtualKeyToDomKey.

> Source/WebCore/platform/win/WindowsKeyNames.h:31
> +#pragma once
> +#include <windows.h>

Nit: missing newline

> Source/WebCore/platform/win/WindowsKeyNames.h:49
> +    typedef std::pair<unsigned /*VirtualKey*/, unsigned /*EventFlags*/> VirtualKeyEventFlagsPair;

Would be nice to update typedefs to usings, again assuming we're not trying to minimize edits to this file.

[Fujii Hironori]

Created attachment 384037 [details]
Patch

* Addressed the review feedbacks.

[Fujii Hironori]

Comment on attachment 383542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383542&action=review

>> Source/WebCore/platform/win/WindowsKeyNames.cpp:39
>> +} EventFlags;
> 
> Might be neat to use OptionSet here...unless the goal is to keep this file as similar as possible to the Chromium copy, in which case it's fine as-is.

Good idea.

>> Source/WebCore/platform/win/WindowsKeyNames.cpp:209
>> +            return nullString();
> 
> Interesting, I didn't realize this existed.
> There are only 5 instances of `return nullString();` in Source/ compared to 731 of `return String();`...
> I guess it was introduced early this year for JSStrings: https://github.com/WebKit/webkit/commit/cb663b3dd21a5b9fbdd180625406226f851a1c7e
> 
> According to Yusuke on IRC: "nullString is used almost only when you want `const String&` allocated in somewhere."

Interesting. I think nullString() is just a nullptr of WTF::StringImpl. Replaced.

[Ross Kirsling]

Comment on attachment 384037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384037&action=review

> Source/WebCore/platform/win/WindowsKeyNames.h:39
> +template<typename T> struct DefaultHash<OptionSet<T>> {

Hmm, wouldn't this need to go in wtf/OptionSet.h?

[Fujii Hironori]

Comment on attachment 384037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384037&action=review

>> Source/WebCore/platform/win/WindowsKeyNames.h:39
>> +template<typename T> struct DefaultHash<OptionSet<T>> {
> 
> Hmm, wouldn't this need to go in wtf/OptionSet.h?

Fair enough. I should do it.
Filed: Bug 204562 – Add DefaultHash<OptionSet<T>> and HashTrait<OptionSet<T>> specializations

[Fujii Hironori]

Created attachment 384320 [details]
Patch

[Ross Kirsling]

Comment on attachment 384320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384320&action=review

r=me

> Source/WebCore/platform/win/WindowsKeyNames.cpp:194
> +            // VK_KANA isn't generated by any modern layouts but is a listed value
> +            // that third-party apps might synthesize, so we handle it anyway.

Style nit: indentation

> Source/WebCore/platform/win/WindowsKeyNames.cpp:274
> +        KeyModifierSet({KeyModifier::Shift, KeyModifier::CapsLock}) & modifiers,

Style nit: spaces inside braces

> Source/WebCore/platform/win/WindowsKeyNames.cpp:522
> +                    m_printableKeyCodeToKey.set(std::make_pair(virtualKey, modifiers), makeString(UChar(translatedChars[0])));

Seems like we should use WTF::ucharFrom...but then it's only one character.
(Then again, I guess if it weren't just one character then you wouldn't need makeString either, since there's String(const wchar_t*). Oh well.)

[Fujii Hironori]

Comment on attachment 384320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384320&action=review

Thank you very much for the review.

>> Source/WebCore/platform/win/WindowsKeyNames.cpp:522
>> +                    m_printableKeyCodeToKey.set(std::make_pair(virtualKey, modifiers), makeString(UChar(translatedChars[0])));
> 
> Seems like we should use WTF::ucharFrom...but then it's only one character.
> (Then again, I guess if it weren't just one character then you wouldn't need makeString either, since there's String(const wchar_t*). Oh well.)

Yes, it's a single character. Will replace it with String(translatedChars, 1).

[Fujii Hironori]

Created attachment 384321 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 384321 [details]
Patch for landing

Clearing flags on attachment: 384321

Committed r252873: <https://trac.webkit.org/changeset/252873>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57482382>