Bug 185248 – [GTK][WPE] Special combination characters doesn't respect the keystroke order when high CPU load

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Andres Gomez Garcia]

I've been experiencing this for a long, loooong time. It only happens that I've finally decided to report this.

When using WKGTK+ (Ephy, as a ephy web app or embedded in, for example, Revolt (https://github.com/aperezdc/revolt) ), if the CPU gets to high load, usually due to WKGTK+ itself, writing in a text field doesn't get fluid. You may write a long text and it would only appear after some seconds.

When this happens, the keystrokes involving a combination of keystrokes, for example, accented letters (á, é, é, í, ó, ú) doesn't respect the order in which they were typed but jump to the first position.

For example, if I would be writing the following sentence:

"webkit está genial"

If the delay and high CPU happens just after the word "webkit" has been typed, I will get in the end:

"webkitá est genial"


I can reproduce this with Debian Testing's WebKitGTK+ 2.20.1 and Ephy 3.28.1

In any case, as said, I've experienced this for a long time.

[Adrian Perez]

(In reply to Andres Gomez Garcia from comment #0)
> I've been experiencing this for a long, loooong time. It only happens that
> I've finally decided to report this.
> 
> [...]

I can confirm this, and I would be surprised if others who use
WebKitGTK+ based browsers/applications have not seen this as well.
It's been going on for a long time, indeed.

[Adrian Perez]

Still happening with WebKitGTK+ 2.22.0

[Claudio Saavedra]

I found the culprit to this problem but I am not sure if the fix will be easy.

WebPageProxy, in the UI process, keeps a queue of keyboard events in order to send them to the Web process in order. It only sends one at the time, and the next in the queue is only sent when the web process responds that it has received the event (this is why under heavy load keystrokes are very slow, but that's a different issue).

For simple keys, the IM filter passes the event to the WebPageProxy, that queues the events as described above.

For composed keys, the IM filter passes the event to the WebPageProxy (which queues them), but in addition it handles the composition by using the composition methods. Once the composition is confirmed (say, á in example in comment #0), the WebPageProxy sends the ConfirmedComposition message to the web process with the composed text without regards to the key event queue, skipping it. The web process receives then the composed text and forwards it to the editor which inserts it immediately. The queued events in the ui process continue to be processed afterwards.

[Carlos Garcia Campos]

This now affects WPE as well when an input method is used.

[Carlos Garcia Campos]

Created attachment 386193 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Zan Dobersek]

Comment on attachment 386193 [details]
Patch

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

> Source/WebCore/platform/PlatformKeyboardEvent.h:103
> +        Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines() const { return m_preeditUnderlines; }
> +        Optional<uint64_t> preeditSelectionRangeStart() const { return m_preeditSelectionRangeStart; }
> +        Optional<uint64_t> preeditSelectionRangeLength() const { return m_preeditSelectionRangeLength; }

These can be const references.

> Source/WebKit/Shared/NativeWebKeyboardEvent.h:73
> +    NativeWebKeyboardEvent(GdkEvent*, const String&, HandledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>>, Optional<EditingRange>, Vector<String>&& commands);

The Optional parameters should be rvalue references.

> Source/WebKit/Shared/NativeWebKeyboardEvent.h:79
> +    NativeWebKeyboardEvent(struct wpe_input_keyboard_event*, const String&, HandledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>>, Optional<EditingRange>);

The Optional parameters should be rvalue references.

> Source/WebKit/Shared/WebEvent.h:262
> +    WebKeyboardEvent(Type, const String& text, const String& key, const String& code, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, bool handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>>, Optional<EditingRange>, Vector<String>&& commands, bool isKeypad, OptionSet<Modifier>, WallTime timestamp);

The Optional parameters should be rvalue references.

> Source/WebKit/Shared/WebEvent.h:266
> +    WebKeyboardEvent(Type, const String& text, const String& key, const String& code, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, bool handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>>, Optional<EditingRange>, bool isKeypad, OptionSet<Modifier>, WallTime timestamp);

The Optional parameters should be rvalue references.

> Source/WebKit/Shared/WebEvent.h:284
> +    Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines() const { return m_preeditUnderlines; }
> +    Optional<EditingRange> preeditSelectionRange() const { return m_preeditSelectionRange; }

These can be const references.

> Source/WebKit/Shared/WebKeyboardEvent.cpp:61
> +WebKeyboardEvent::WebKeyboardEvent(Type type, const String& text, const String& key, const String& code, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, bool handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines, Optional<EditingRange> preeditSelectionRange, Vector<String>&& commands, bool isKeypad, OptionSet<Modifier> modifiers, WallTime timestamp)

The Optional parameters should be rvalue references.

> Source/WebKit/Shared/WebKeyboardEvent.cpp:106
> +WebKeyboardEvent::WebKeyboardEvent(Type type, const String& text, const String& key, const String& code, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, bool handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines, Optional<EditingRange> preeditSelectionRange, bool isKeypad, OptionSet<Modifier> modifiers, WallTime timestamp)

The Optional parameters should be rvalue references.

> Source/WebKit/Shared/gtk/NativeWebKeyboardEventGtk.cpp:37
> +NativeWebKeyboardEvent::NativeWebKeyboardEvent(GdkEvent* event, const String& text, HandledByInputMethod handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines, Optional<EditingRange> preeditSelectionRange, Vector<String>&& commands)
> +    : WebKeyboardEvent(WebEventFactory::createWebKeyboardEvent(event, text, handledByInputMethod == HandledByInputMethod::Yes, preeditUnderlines, preeditSelectionRange, WTFMove(commands)))

The Optional parameters should be rvalue references, and they should be simply moved into the createWebKeyboardEvent() call.

> Source/WebKit/Shared/gtk/NativeWebKeyboardEventGtk.cpp:43
> +    : WebKeyboardEvent(WebEventFactory::createWebKeyboardEvent(event.nativeEvent(), event.text(), event.handledByInputMethod(), event.preeditUnderlines(), event.preeditSelectionRange(), Vector<String>(event.commands())))

Because of the rvalue references you'll have to perform explicit manual copy, which could be as simple as `{ event.preeditUnderlines() }`. Just like it's necessary for the last argument in this call.

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:255
> +WebKeyboardEvent WebEventFactory::createWebKeyboardEvent(const GdkEvent* event, const String& text, bool handledByInputMethod, Optional<Vector<CompositionUnderline>> preeditUnderlines, Optional<EditingRange> preeditSelectionRange, Vector<String>&& commands)

The Optional parameters should be rvalue references that would be just moved over into the WebKeyboardEvent constructor, just like the `commands` parameter.

> Source/WebKit/Shared/libwpe/NativeWebKeyboardEventLibWPE.cpp:33
> +NativeWebKeyboardEvent::NativeWebKeyboardEvent(struct wpe_input_keyboard_event* event, const String& text, HandledByInputMethod handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines, Optional<EditingRange> preeditSelectionRange)

The Optional parameters should be rvalue references that would be just moved over into the WebKeyboardEvent constructor.

> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:65
> +WebKeyboardEvent WebEventFactory::createWebKeyboardEvent(struct wpe_input_keyboard_event* event, const String& text, bool handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>> preeditUnderlines, Optional<EditingRange> preeditSelectionRange)

The Optional parameters should be rvalue references that would be just moved over into the WebKeyboardEvent constructor.

> Source/WebKit/Shared/libwpe/WebEventFactory.h:41
> +    static WebKeyboardEvent createWebKeyboardEvent(struct wpe_input_keyboard_event*, const String&, bool handledByInputMethod, Optional<Vector<WebCore::CompositionUnderline>>, Optional<EditingRange>);

The Optional parameters should be rvalue references.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2694
> +static void webkitWebViewSynthesizeCompositionKeyPress(WebKitWebView* webView, const String& text, Optional<Vector<CompositionUnderline>> underlines, Optional<EditingRange> selectionRange)

The Optional parameters should be rvalue references.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1912
> +void webkitWebViewBaseSynthesizeCompositionKeyPress(WebKitWebViewBase* webViewBase, const String& text, Optional<Vector<CompositionUnderline>> underlines, Optional<EditingRange> selectionRange)

The Optional parameters should be rvalue references.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:278
> +void View::synthesizeCompositionKeyPress(const String& text, Optional<Vector<WebCore::CompositionUnderline>> underlines, Optional<EditingRange> selectionRange)

The Optional parameters should be rvalue references. If not possible, then at least const references.

> Source/WebKit/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:149
> +    if (auto underlines = platformEvent->preeditUnderlines()) {

When the return type is changed to being a reference, the `auto` here should follow.

> Source/WebKit/WebProcess/WebCoreSupport/wpe/WebEditorClientWPE.cpp:251
> +    if (auto underlines = platformEvent->preeditUnderlines()) {

When the return type is changed to being a reference, the `auto` here should follow.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5359
> -void WebPage::confirmComposition(const String& compositionString)
> +void WebPage::cancelComposition(const String& compositionString)
>  {
>      if (auto* targetFrame = targetFrameForEditing(*this))
>          targetFrame->editor().confirmComposition(compositionString);
>  }

Is this fine?

[Carlos Garcia Campos]

Created attachment 386205 [details]
Patch

[Carlos Garcia Campos]

Committed r253881: <https://trac.webkit.org/changeset/253881>