Bug 188370 - REGRESSION (r228260): Events handled by input method invoke default event handler
Summary: REGRESSION (r228260): Events handled by input method invoke default event han...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh macOS 10.13
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-06 19:36 PDT by mzp
Modified: 2018-08-08 21:32 PDT (History)
10 users (show)

See Also:


Attachments
Problem (104.70 KB, image/png)
2018-08-06 19:36 PDT, mzp
no flags Details
WIP (2.34 KB, patch)
2018-08-08 14:54 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (8.97 KB, patch)
2018-08-08 19:54 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mzp 2018-08-06 19:36:32 PDT
Created attachment 346676 [details]
Problem

When tab key pressed on text box, focus always move to next element in spite of input method behavior. IMO, this is regression bug.

# Environments
 [affected]
 - Movaje + Safari 12.0 (14606.1.30)
 - High Sierra + Safari 12.0 (14606.1.30)
 - High Sierra + WebKit trunk(r234578)

 [not affected]
 - High Sierra + Safari 11.1.2 (13605.3.8)

# Reproduce steps

 1. Open web page containing text fields (e.g. https://twitter.com).
 2. Enable input methods (e.g. Apple Japanese Input Method.)
 3. Type A, and 'あ' is inserted.
 4. Press tab.

# Except behavior
Tab key event is consumed by IM, and focus doesn't move.

# Actual behavior
Tab key event is consumed by IM, and focus moves to next element.

# Details
Reproduce steps is common case. The real problem exists between input method consumed events and default event handler.
Safari 11 doesn't invoke default event handler by IM handled event. But, Safari12/WebKit trunk invokes default event handler by IM handled event

This regression is introduced by https://trac.webkit.org/changeset/228260/webkit. Especially by change of WebCore/dom/EventDispacher.cpp.

An event handled by input method is marked as default handled at 

    // WebCore/page/EventHandler.cpp
    // EventHandler::internalKeyEvent(const PlatformKeyboardEvent& initialKeyEvent)
    if (handledByInputMethod) {
        keyDownEvent.setWindowsVirtualKeyCode(CompositionEventKeyCode);
        keydown = KeyboardEvent::create(keyDownEvent, &m_frame.windowProxy());
        keydown->setTarget(element);
        keydown->setDefaultHandled();
    }

Before the changes, event dispatcher ignores these event.

    // WebCore/dom/EventDispatcher.cpp
    // EventDispatcher::dispatchEvent(Node& node, Event& event)
    if (!event.defaultPrevented() && !event.defaultHandled()) {
        // FIXME: Not clear why we need to reset the target for the default event handlers.
        // We should research this, and remove this code if possible.
        auto* finalTarget = event.target();
        event.setTarget(EventPath::eventTargetRespectingTargetRules(node));
        callDefaultEventHandlersInBubblingOrder(event, eventPath);
        event.setTarget(finalTarget);
    }

However, the change inserted `event.resetBeforeDispatch()`. This method resets defaultHandled field.

    // WebCore/dom/EventDispatcher.cpp
    // EventDispatcher::dispatchEvent(Node& node, Event& event)

    event.resetBeforeDispatch();
    // ....
    if (!event.defaultPrevented() && !event.defaultHandled()) {
        // FIXME: Not clear why we need to reset the target for the default event handlers.
        // We should research this, and remove this code if possible.
        auto* finalTarget = event.target();
        event.setTarget(EventPath::eventTargetRespectingTargetRules(node));
        callDefaultEventHandlersInBubblingOrder(event, eventPath);
        event.setTarget(finalTarget);
    }

This is why events handled by input method invoke default event handler.
Comment 1 Radar WebKit Bug Importer 2018-08-07 12:01:38 PDT
<rdar://problem/43013703>
Comment 2 Ryosuke Niwa 2018-08-08 14:54:09 PDT
Created attachment 346801 [details]
WIP
Comment 3 Ryosuke Niwa 2018-08-08 19:54:59 PDT
Created attachment 346813 [details]
Fixes the bug
Comment 4 Wenson Hsieh 2018-08-08 20:14:53 PDT
Comment on attachment 346813 [details]
Fixes the bug

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

> LayoutTests/ChangeLog:8
> +        Added a regression test. Due to the lack of adaquete support of textInputController in WebKitTestRunner,

Nit - adequate
Comment 5 Wenson Hsieh 2018-08-08 20:23:38 PDT
Comment on attachment 346813 [details]
Fixes the bug

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

> Source/WebCore/dom/Event.h:122
> +    bool ignoreDefaultEventHandler() const { return m_ignoreDefaultEventHandler; }

I almost feel like the getter should bed name "isIgnoringDefaultEventHandler", and the method that sets this bit should be called "ignoreDefaultEventHandler", but "ignoreDefaultEventHandler" and "setIgnoreDefaultEventHandler" seem to match "defaultHandled" and "setDefaultHandled" already, so this is probably fine.
Comment 6 Ryosuke Niwa 2018-08-08 20:58:46 PDT
(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 346813 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346813&action=review
> 
> > Source/WebCore/dom/Event.h:122
> > +    bool ignoreDefaultEventHandler() const { return m_ignoreDefaultEventHandler; }
> 
> I almost feel like the getter should bed name
> "isIgnoringDefaultEventHandler", and the method that sets this bit should be
> called "ignoreDefaultEventHandler", but "ignoreDefaultEventHandler" and
> "setIgnoreDefaultEventHandler" seem to match "defaultHandled" and
> "setDefaultHandled" already, so this is probably fine.

Hm... maybe isDefaultEventHandlerIgnored and setIsDefaultEventHandlerIgnored?
Comment 7 Wenson Hsieh 2018-08-08 21:06:05 PDT
(In reply to Ryosuke Niwa from comment #6)
> (In reply to Wenson Hsieh from comment #5)
> > Comment on attachment 346813 [details]
> > Fixes the bug
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=346813&action=review
> > 
> > > Source/WebCore/dom/Event.h:122
> > > +    bool ignoreDefaultEventHandler() const { return m_ignoreDefaultEventHandler; }
> > 
> > I almost feel like the getter should bed name
> > "isIgnoringDefaultEventHandler", and the method that sets this bit should be
> > called "ignoreDefaultEventHandler", but "ignoreDefaultEventHandler" and
> > "setIgnoreDefaultEventHandler" seem to match "defaultHandled" and
> > "setDefaultHandled" already, so this is probably fine.
> 
> Hm... maybe isDefaultEventHandlerIgnored and setIsDefaultEventHandlerIgnored?

Sounds good to me.
Comment 8 Ryosuke Niwa 2018-08-08 21:32:41 PDT
Committed r234718: <https://trac.webkit.org/changeset/234718>