From 06ef42c72c9fe0db6541c134682dc6e2887c7263 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Wed, 15 May 2024 05:42:33 +0000 Subject: [PATCH] Bug 1879765 - part 2: Make `BrowserChild` store the last `code` value of consumed `eKeyDown` event r=smaug The builtin legacy IME of Windows to type a Unicode with typing a code point causes consumed `eKeyDown` events while typing the code point, i.e., without `eKeyPress` (FYI: The consumed state is not exposed to the web, it's used only in chrome UI for the compatibility with Chrome). Then, `BrowserChild` store whether the last `eKeyDown` was consumed or not to prevent the following `eKeyPress`. Finally, a `eKeyPress` event is fired to input the Unicode character after `eKeyUp` for `Alt`. The stored value is set to new value only when another `eKeyDown` event is sent from the parent process. Therefore, the last digit inputting `eKeyDown` causes `BrowserChild` thinking the last `eKeyDown` is consumed so that the last `eKeyPress` is not dispatched. This patch makes `BrowserChild` to store the `code` value of the last consumed `eKeyDown` and check the `code` value to consider whether coming `eKeyPress` should be or not be dispatched to `PresShell` and the DOM. Differential Revision: https://phabricator.services.mozilla.com/D207957 --- dom/base/TextInputProcessor.cpp | 65 +++++++++ .../chrome/window_nsITextInputProcessor.xhtml | 120 ++++++++++++++++ dom/events/test/mochitest.toml | 2 + ...icode_input_on_windows_with_emulation.html | 131 ++++++++++++++++++ dom/interfaces/base/nsITextInputProcessor.idl | 19 +++ dom/ipc/BrowserChild.cpp | 46 +++++- dom/ipc/BrowserChild.h | 2 +- 7 files changed, 380 insertions(+), 5 deletions(-) create mode 100644 dom/events/test/test_unicode_input_on_windows_with_emulation.html diff --git a/dom/base/TextInputProcessor.cpp b/dom/base/TextInputProcessor.cpp index c263bdcf9b3a..28091136008a 100644 --- a/dom/base/TextInputProcessor.cpp +++ b/dom/base/TextInputProcessor.cpp @@ -1250,6 +1250,71 @@ nsresult TextInputProcessor::KeyupInternal( return NS_OK; } +NS_IMETHODIMP +TextInputProcessor::InsertTextWithKeyPress(const nsAString& aString, + Event* aDOMKeyEvent, + uint32_t aKeyFlags, + uint8_t aOptionalArgc, + bool* aDoDefault) { + MOZ_RELEASE_ASSERT(aDoDefault, "aDoDefault must not be nullptr"); + MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); + + nsresult rv = IsValidStateForComposition(); + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + + if (aOptionalArgc < 1) { + aDOMKeyEvent = nullptr; + } + if (aOptionalArgc < 2) { + aKeyFlags = 0; + } + *aDoDefault = !(aKeyFlags & KEY_DEFAULT_PREVENTED); + + WidgetKeyboardEvent* const originalKeyEvent = + aDOMKeyEvent ? aDOMKeyEvent->WidgetEventPtr()->AsKeyboardEvent() + : nullptr; + if (NS_WARN_IF(aDOMKeyEvent && !originalKeyEvent)) { + return NS_ERROR_INVALID_ARG; + } + + WidgetKeyboardEvent keyEvent(true, eKeyPress, nullptr); + if (originalKeyEvent) { + keyEvent = WidgetKeyboardEvent(*originalKeyEvent); + keyEvent.mFlags.mIsTrusted = true; + keyEvent.mMessage = eKeyPress; + } + keyEvent.mKeyNameIndex = KEY_NAME_INDEX_USE_STRING; + keyEvent.mKeyValue = aString; + rv = PrepareKeyboardEventToDispatch(keyEvent, aKeyFlags); + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + // Do not dispatch modifier key events even if the source event is a modifier + // key event because modifier state should be changed before this. + // TODO: In some test scenarios, we may need a new flag to use the given + // modifier state as-is. + keyEvent.mModifiers = GetActiveModifiers(); + + // See KeyDownInternal() for the detail of this. + if (XRE_IsContentProcess()) { + nsresult rv = InitEditCommands(keyEvent); + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + } + + nsEventStatus status = + *aDoDefault ? nsEventStatus_eIgnore : nsEventStatus_eConsumeNoDefault; + RefPtr dispatcher(mDispatcher); + if (dispatcher->MaybeDispatchKeypressEvents(keyEvent, status)) { + *aDoDefault = (status != nsEventStatus_eConsumeNoDefault); + } + + return NS_OK; +} + NS_IMETHODIMP TextInputProcessor::GetModifierState(const nsAString& aModifierKeyName, bool* aActive) { diff --git a/dom/base/test/chrome/window_nsITextInputProcessor.xhtml b/dom/base/test/chrome/window_nsITextInputProcessor.xhtml index c62ba2ce472d..ea569d8ea551 100644 --- a/dom/base/test/chrome/window_nsITextInputProcessor.xhtml +++ b/dom/base/test/chrome/window_nsITextInputProcessor.xhtml @@ -3648,6 +3648,125 @@ async function runKeyTests() window.removeEventListener("keyup", handler); } +function runInsertTextWithKeyPressTests() { + const description = "runInsertTextWithKeyPressTests(): "; + + var TIP = createTIP(); + ok(TIP.beginInputTransactionForTests(window), + description + "TIP.beginInputTransactionForTests() should succeed"); + + input.value = ""; + input.focus(); + + let events = []; + function handler(aEvent) { + events.push({ + type: aEvent.type, + key: aEvent.key, + code: aEvent.code, + shiftKey: aEvent.shiftKey, + altKey: aEvent.altKey, + ctrlKey: aEvent.ctrlKey, + metaKey: aEvent.metaKey, + }); + } + function stringifyEvents(aEvents) { + if (!aEvents.length) { + return "[]"; + } + function stringifyEvent(aEvent) { + return `{ type: "${aEvent.type}", key: "${aEvent.key}", code: ${aEvent.code}, shiftKey: ${ + aEvent.shiftKey + }, altKey: ${aEvent.altKey}, ctrlKey: ${aEvent.ctrlKey}, metaKey: ${aEvent.metaKey}}`; + } + let result = ""; + for (const event of aEvents) { + if (result == "") { + result = "[ "; + } else { + result += ", "; + } + result += stringifyEvent(event); + } + return result + " ]"; + } + window.addEventListener("keydown", handler); + window.addEventListener("keypress", handler); + window.addEventListener("keyup", handler); + + events = []; + input.value = ""; + TIP.insertTextWithKeyPress("X"); + is( + input.value, + "X", + `${description}insertTextWithKeyPress without optional args should cause inserting the string` + ); + is( + stringifyEvents(events), + stringifyEvents([ + { type: "keypress", key: "X", code: "", shiftKey: false, altKey: false, ctrlKey: false, metaKey: false }, + ]), + `${description}insertTextWithPress without optional args should cause only a "keypress" event` + ); + + events = []; + input.value = ""; + TIP.insertTextWithKeyPress("Y", new KeyboardEvent("keydown", {key: "Alt", code: "AltLeft"})); + is( + input.value, + "Y", + `${description}insertTextWithKeyPress with Alt keydown event should cause inserting the string` + ); + // The key value should be the inserted string and the code value should be same as the source event's. + is( + stringifyEvents(events), + stringifyEvents([ + { type: "keypress", key: "Y", code: "AltLeft", shiftKey: false, altKey: false, ctrlKey: false, metaKey: false }, + ]), + `${description}insertTextWithPress with Alt keydown event should cause only a "keypress" event whose code is "AltLeft"` + ); + + events = []; + input.value = ""; + TIP.insertTextWithKeyPress("Z", new KeyboardEvent("keydown", {key: "Alt", code: "AltLeft", altKey: true})); + is( + input.value, + "Z", + `${description}insertTextWithKeyPress with Alt keydown whose altKey is true should cause inserting the string` + ); + // TIP should use its internal modifier state instead of specified modifier state. + is( + stringifyEvents(events), + stringifyEvents([ + { type: "keypress", key: "Z", code: "AltLeft", shiftKey: false, altKey: false, ctrlKey: false, metaKey: false }, + ]), + `${description}insertTextWithPress with Alt keydown whose altKey is true should cause only a "keypress" event whose altKey is false"` + ); + + TIP.keydown(new KeyboardEvent("keydown", { key: "Alt" })); + events = []; + input.value = ""; + TIP.insertTextWithKeyPress("X", new KeyboardEvent("keydown", {key: "Shift", code: "ShiftLeft", shiftKey: true})); + is( + input.value, + "", + `${description}insertTextWithKeyPress after Alt keydown should not cause inserting the string` + ); + is( + stringifyEvents(events), + stringifyEvents([ + { type: "keypress", key: "X", code: "ShiftLeft", shiftKey: false, altKey: true, ctrlKey: false, metaKey: false }, + ]), + `${description}insertTextWithPress after Alt keydown should cause only a "keypress" event whose altKey is true"` + ); + TIP.keyup(new KeyboardEvent("keyup ", { key: "Alt" })); + + window.removeEventListener("keydown", handler); + window.removeEventListener("keypress", handler); + window.removeEventListener("keyup", handler); +} + function runErrorTests() { var description = "runErrorTests(): "; @@ -4854,6 +4973,7 @@ async function runTests() runCompositionWithKeyEventTests(); runConsumingKeydownBeforeCompositionTests(); await runKeyTests(); + runInsertTextWithKeyPressTests(); runErrorTests(); runCommitCompositionTests(); await runCallbackTests(false); diff --git a/dom/events/test/mochitest.toml b/dom/events/test/mochitest.toml index 19a082b1e326..2c65cd7604d3 100644 --- a/dom/events/test/mochitest.toml +++ b/dom/events/test/mochitest.toml @@ -503,6 +503,8 @@ skip-if = ["os == 'android'"] # timeout ["test_unbound_before_in_active_chain.html"] +["test_unicode_input_on_windows_with_emulation.html"] + ["test_use_conflated_keypress_event_model_on_newer_Office_Online_Server.html"] ["test_use_split_keypress_event_model_on_old_Confluence.html"] diff --git a/dom/events/test/test_unicode_input_on_windows_with_emulation.html b/dom/events/test/test_unicode_input_on_windows_with_emulation.html new file mode 100644 index 000000000000..b15d06c613a4 --- /dev/null +++ b/dom/events/test/test_unicode_input_on_windows_with_emulation.html @@ -0,0 +1,131 @@ + + + +Test event sequence of Unicode character input of Windows builtin IME + + + + + + + + + diff --git a/dom/interfaces/base/nsITextInputProcessor.idl b/dom/interfaces/base/nsITextInputProcessor.idl index 1dea6640bce8..f8d0158a30e6 100644 --- a/dom/interfaces/base/nsITextInputProcessor.idl +++ b/dom/interfaces/base/nsITextInputProcessor.idl @@ -584,6 +584,25 @@ interface nsITextInputProcessor : nsISupports boolean keyup(in Event aKeyboardEvent, [optional] in unsigned long aKeyFlags); + /** + * insertTextWithKeyPress() is designed for automated tests. This dispatches + * isolated eKeyPress events to insert aString if selection is in an editable + * element. + * + * @aString The string which will be inserted. + * @aKeyboardEvent [optional] This is nullable. If specified as + * non-nullptr, the insertion is emulated as caused by the + * key event. + * @param aKeyFlags Special flags. The values can be some of KEY_* + * constants. + * @return false if one of the dispatched eKeyPress event is + * consumed. + */ + [can_run_script, optional_argc] + boolean insertTextWithKeyPress(in AString aString, + [optional] in Event aKeyboardEvent, + [optional] in unsigned long aKeyFlags); + /** * getModifierState() returns modifier state managed by this instance. * diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp index ef348e0831f5..a10ba929bba0 100644 --- a/dom/ipc/BrowserChild.cpp +++ b/dom/ipc/BrowserChild.cpp @@ -278,7 +278,6 @@ BrowserChild::BrowserChild(ContentChild* aManager, const TabId& aTabId, mUniqueId(aTabId), mDidFakeShow(false), mTriedBrowserInit(false), - mIgnoreKeyPressEvent(false), mHasValidInnerSize(false), mDestroyed(false), mIsTopLevel(aIsTopLevel), @@ -1983,9 +1982,14 @@ mozilla::ipc::IPCResult BrowserChild::RecvRealKeyEvent( aEvent.AreAllEditCommandsInitialized()); // If content code called preventDefault() on a keydown event, then we don't - // want to process any following keypress events. + // want to process any following keypress events which is caused by the + // preceding keydown (i.e., default action of the preceding keydown). + // In other words, if the keypress is not a default action of the preceding + // keydown, we should not stop dispatching keypress event even if the + // immediate preceding keydown was consumed. const bool isPrecedingKeyDownEventConsumed = - aEvent.mMessage == eKeyPress && mIgnoreKeyPressEvent; + aEvent.mMessage == eKeyPress && mPreviousConsumedKeyDownCode.isSome() && + mPreviousConsumedKeyDownCode.value() == aEvent.mCodeNameIndex; WidgetKeyboardEvent localEvent(aEvent); localEvent.mWidget = mPuppetWidget; @@ -1999,7 +2003,41 @@ mozilla::ipc::IPCResult BrowserChild::RecvRealKeyEvent( UpdateRepeatedKeyEventEndTime(localEvent); if (aEvent.mMessage == eKeyDown) { - mIgnoreKeyPressEvent = status == nsEventStatus_eConsumeNoDefault; + // If eKeyDown is consumed, we should stop dispatching the following + // eKeyPress events since the events are default action of eKeyDown. + // FIXME: We should synthesize eKeyPress in this process (bug 1181501). + if (status == nsEventStatus_eConsumeNoDefault) { + MOZ_ASSERT_IF(!aEvent.mFlags.mIsSynthesizedForTests, + aEvent.mCodeNameIndex != CODE_NAME_INDEX_USE_STRING); + // If mPreviousConsumedKeyDownCode is not Nothing, 2 or more keys may be + // pressed at same time and their eKeyDown are consumed. However, we + // forget the previous eKeyDown event result here and that might cause + // dispatching eKeyPress events caused by the previous eKeyDown in + // theory. However, this should not occur because eKeyPress should be + // fired before another eKeyDown, although it's depend on how the native + // keyboard event handler is implemented. + mPreviousConsumedKeyDownCode = Some(aEvent.mCodeNameIndex); + } + // If eKeyDown is not consumed but we know preceding eKeyDown is consumed, + // we need to forget it since we should not stop dispatching following + // eKeyPress events which are default action of current eKeyDown. + else if (mPreviousConsumedKeyDownCode.isSome() && + aEvent.mCodeNameIndex == mPreviousConsumedKeyDownCode.value()) { + mPreviousConsumedKeyDownCode.reset(); + } + } + // eKeyPress is a default action of eKeyDown. Therefore, eKeyPress is fired + // between eKeyDown and eKeyUp. So, received an eKeyUp for eKeyDown which + // was consumed means that following eKeyPress events should be dispatched. + // Therefore, we need to forget the fact that the preceding eKeyDown was + // consumed right now. + // NOTE: On Windows, eKeyPress may be fired without preceding eKeyDown if + // IME or utility app sends WM_CHAR message. So, if we don't forget it, + // we'd consume unrelated eKeyPress events. + else if (aEvent.mMessage == eKeyUp && + mPreviousConsumedKeyDownCode.isSome() && + aEvent.mCodeNameIndex == mPreviousConsumedKeyDownCode.value()) { + mPreviousConsumedKeyDownCode.reset(); } if (localEvent.mFlags.mIsSuppressedOrDelayed) { diff --git a/dom/ipc/BrowserChild.h b/dom/ipc/BrowserChild.h index 7e84b80be16b..b11e7c5c24d6 100644 --- a/dom/ipc/BrowserChild.h +++ b/dom/ipc/BrowserChild.h @@ -750,6 +750,7 @@ class BrowserChild final : public nsMessageManagerScriptExecutor, RefPtr mManager; RefPtr mBrowsingContext; RefPtr mStatusFilter; + Maybe mPreviousConsumedKeyDownCode; uint32_t mChromeFlags; uint32_t mMaxTouchPoints; layers::LayersId mLayersId; @@ -770,7 +771,6 @@ class BrowserChild final : public nsMessageManagerScriptExecutor, bool mDidFakeShow : 1; bool mTriedBrowserInit : 1; - bool mIgnoreKeyPressEvent : 1; bool mHasValidInnerSize : 1; bool mDestroyed : 1;