Bug 163021 – Support onbeforeinput event handling for the new InputEvent spec

• Component: UI Events
• Status: RESOLVED FIXED

[Wenson Hsieh]

Support onbeforeinput event handling for the new InputEvent spec

[Wenson Hsieh]

Created attachment 290826 [details]
Prototype (tests in progress)

[Build Bot]

Comment on attachment 290826 [details]
Prototype (tests in progress)

Attachment 290826 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2232455

New failing tests:
editing/mac/spelling/accept-misspelled-candidate.html
editing/undo/undo-after-event-edited.html

[Build Bot]

Created attachment 290833 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290826 [details]
Prototype (tests in progress)

Attachment 290826 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2232462

New failing tests:
editing/mac/spelling/accept-misspelled-candidate.html
editing/undo/undo-after-event-edited.html

[Build Bot]

Created attachment 290835 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290826 [details]
Prototype (tests in progress)

Attachment 290826 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2232454

New failing tests:
editing/mac/spelling/accept-misspelled-candidate.html
editing/undo/undo-after-event-edited.html

[Build Bot]

Created attachment 290836 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290826 [details]
Prototype (tests in progress)

Attachment 290826 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2232505

New failing tests:
editing/undo/undo-after-event-edited.html

[Build Bot]

Created attachment 290838 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

Created attachment 290858 [details]
First pass

[Wenson Hsieh]

<rdar://problem/28658073>

[Wenson Hsieh]

Comment on attachment 290858 [details]
First pass

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

> Source/WebCore/ChangeLog:14
> +        Tweaks existing layout tests and adds new unit tests.

self nit - s/unit/layout/

[Darin Adler]

Comment on attachment 290858 [details]
First pass

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

There are many different special cases in the code, but the tests don’t seem to cover them.

> Source/WebCore/ChangeLog:9
> +        `onbeforeinput` InputEvents to the page. To do this, we introduce two new virtual methods:

Terminology here is wrong. There is no such thing as an "onbeforeinput" event. The event name would be "beforeinput". The string "onbeforeinput" would be the event handler attribute name for that event.

> Source/WebCore/dom/Node.cpp:2207
> +    if (!document().settings()->inputEventsEnabled())

What guarantees settings won’t be nullptr?

> Source/WebCore/dom/Node.cpp:2213
> +    auto beforeInputEvent = InputEvent::create(eventNames().beforeinputEvent, inputType, true, true, document().defaultView(), 0);
> +    dispatchScopedEvent(beforeInputEvent);
> +
> +    return !beforeInputEvent->defaultPrevented();

I suggest just calling this "event", instead of calling it beforeInputEvent. Despite that fact that the other functions above use that different terminology.

> Source/WebCore/dom/Node.cpp:-2276
> -    } else if (event.type() == eventNames().webkitEditableContentChangedEvent) {
> -        dispatchInputEvent(emptyString());

What’s the rationale for removing this? Change log does not say. In fact, there are no per-function comments in the change log at all!

> Source/WebCore/dom/Node.h:528
> +    virtual bool dispatchBeforeInputEvent(const AtomicString& inputType);

We should not be adding this function to the Node class. This is an event we only dispatch to an Element, so the argument should be an Element&.

This should not be a virtual function. Unless there is some reason it is going to need to be overridden in the future. But at this time there is no abstraction here.

This function does not need to be a member function of Element class. I suggest having it be a non-member function, perhaps in the Editor.cpp file or somewhere else where we collect related code about editing events.

> Source/WebCore/editing/CompositeEditCommand.h:120
> +    virtual bool willApplyCommand();

Should comment what the bool return value means. Unclear.

> Source/WebCore/editing/Editor.cpp:1031
> +    bool continueWithDefaultBehavior = true;

Paragraphing in this function is peculiar. I think it would read more clearly without any blank lines.

> Source/WebCore/editing/Editor.cpp:1036
> +    if (endRoot && endRoot != startRoot)
> +        continueWithDefaultBehavior &= endRoot->dispatchBeforeInputEvent(inputTypeName);

Need a test covering the behavior that even if the start root beforeinput event prevents the default the end root gets the event too.

> Source/WebCore/editing/Editor.cpp:1041
> +static void dispatchInputEvents(PassRefPtr<Element> prpStartRoot, PassRefPtr<Element> prpEndRoot, const AtomicString& inputTypeName)

If we are renaming this function, then we should also get rid of the archaic use of PassRefPtr here. Can just change these to RefPtr or RefPtr&&.

> Source/WebCore/editing/Editor.cpp:1057
> +    return dispatchBeforeInputEvents(composition->startingRootEditableElement(), composition->endingRootEditableElement(), emptyString());

Why is empty string OK here? Do we have tests that cover this?

> Source/WebCore/editing/Editor.cpp:1076
> +    dispatchInputEvents(composition->startingRootEditableElement(), composition->endingRootEditableElement(), emptyString());

Why is empty string OK here? Do we have tests that cover this?

> Source/WebCore/editing/Editor.cpp:1101
> +    return dispatchBeforeInputEvents(composition.startingRootEditableElement(), composition.endingRootEditableElement(), emptyString());

Why is empty string OK here? Do we have tests that cover this?

> Source/WebCore/editing/Editor.cpp:3088
> +    if (element && !element->dispatchBeforeInputEvent(emptyString()))

Why is empty string OK here? Do we have tests that cover this?

> Source/WebCore/editing/Editor.cpp:3105
> +        element->dispatchInputEvent(emptyString());

Why is empty string OK here? Do we have tests that cover this?

> Source/WebCore/editing/TypingCommand.cpp:272
> +    if (m_isHandlingInitialTypingCommand)
> +        return CompositeEditCommand::willApplyCommand();
> +
> +    // If this is not the initial typing command, then the TypingCommand will handle the willApplyCommand logic separately
> +    // in TypingCommand::willAddTypingToOpenCommand
> +    return true;

I think we should write this the other way around. First, the unusual case, and then the normal case.

    if (!m_isHandlingInitialTypingCommand) {
        // The TypingCommand will handle the willApplyCommand logic separately in TypingCommand::willAddTypingToOpenCommand.
        return true;
    }

    return CompositeEditCommand::willApplyCommand();

> Source/WebCore/editing/TypingCommand.cpp:377
> +    // TODO: Use the newly added typing command and granularity to ensure that an InputEvent with the
> +    // correct inputType is dispatched.

In our project we use FIXME, and not TODO.

> Source/WebCore/editing/TypingCommand.h:89
> +protected:
> +    bool willApplyCommand() override;
> +    void didApplyCommand() override;

These should be private and final, rather than protected and override, unless I am missing some case where a derived class wants to call or override these.

> Source/WebCore/editing/TypingCommand.h:114
> +    void doApply() override;
> +    bool isTypingCommand() const override;
> +    bool preservesTypingStyle() const override { return m_preservesTypingStyle; }
> +    bool shouldRetainAutocorrectionIndicator() const override

These should be final rather than override unless they need to be overridden further.

> Source/WebCore/editing/TypingCommand.h:120
> +    void setShouldRetainAutocorrectionIndicator(bool retain) override { m_shouldRetainAutocorrectionIndicator = retain; }
> +    bool shouldStopCaretBlinking() const override { return true; }

Ditto.

[Ryosuke Niwa]

Comment on attachment 290858 [details]
First pass

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

>> Source/WebCore/dom/Node.cpp:-2276
>> -        dispatchInputEvent(emptyString());
> 
> What’s the rationale for removing this? Change log does not say. In fact, there are no per-function comments in the change log at all!

Yeah, we should definitely explain this behavioral change in the change log.

>> Source/WebCore/editing/Editor.cpp:3088
>> +    if (element && !element->dispatchBeforeInputEvent(emptyString()))
> 
> Why is empty string OK here? Do we have tests that cover this?

I think the idea is to specify input event type in a future patch.

>> Source/WebCore/editing/TypingCommand.h:114
>> +    bool shouldRetainAutocorrectionIndicator() const override
> 
> These should be final rather than override unless they need to be overridden further.

Or maybe we can make TypingCommand final if nothing inherits from it?

[Wenson Hsieh]

Created attachment 290983 [details]
Patch for landing

[Wenson Hsieh]

Comment on attachment 290858 [details]
First pass

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

>> Source/WebCore/ChangeLog:9
>> +        `onbeforeinput` InputEvents to the page. To do this, we introduce two new virtual methods:
> 
> Terminology here is wrong. There is no such thing as an "onbeforeinput" event. The event name would be "beforeinput". The string "onbeforeinput" would be the event handler attribute name for that event.

Got it -- fixed!

>> Source/WebCore/dom/Node.cpp:2207
>> +    if (!document().settings()->inputEventsEnabled())
> 
> What guarantees settings won’t be nullptr?

Good catch! Added a check, since this does not appear to be guaranteed.

>> Source/WebCore/dom/Node.cpp:2213
>> +    return !beforeInputEvent->defaultPrevented();
> 
> I suggest just calling this "event", instead of calling it beforeInputEvent. Despite that fact that the other functions above use that different terminology.

Sounds good -- done.

>>> Source/WebCore/dom/Node.cpp:-2276
>>> -        dispatchInputEvent(emptyString());
>> 
>> What’s the rationale for removing this? Change log does not say. In fact, there are no per-function comments in the change log at all!
> 
> Yeah, we should definitely explain this behavioral change in the change log.

Good point. Ryosuke and I discussed this and agreed that we can remove webkitEditableContentChangedEvent and just dispatch the input event directly. Also added more comments in the per-function section describing other changes.

>> Source/WebCore/dom/Node.h:528
>> +    virtual bool dispatchBeforeInputEvent(const AtomicString& inputType);
> 
> We should not be adding this function to the Node class. This is an event we only dispatch to an Element, so the argument should be an Element&.
> 
> This should not be a virtual function. Unless there is some reason it is going to need to be overridden in the future. But at this time there is no abstraction here.
> 
> This function does not need to be a member function of Element class. I suggest having it be a non-member function, perhaps in the Editor.cpp file or somewhere else where we collect related code about editing events.

I see -- this makes sense. Moved this to be a static helper in Editor.cpp.

>> Source/WebCore/editing/CompositeEditCommand.h:120
>> +    virtual bool willApplyCommand();
> 
> Should comment what the bool return value means. Unclear.

Sounds good -- added a comment: 'If willApplyCommand returns false, we won't proceed with applying the command.'

>> Source/WebCore/editing/Editor.cpp:1031
>> +    bool continueWithDefaultBehavior = true;
> 
> Paragraphing in this function is peculiar. I think it would read more clearly without any blank lines.

Done.

>> Source/WebCore/editing/Editor.cpp:1036
>> +        continueWithDefaultBehavior &= endRoot->dispatchBeforeInputEvent(inputTypeName);
> 
> Need a test covering the behavior that even if the start root beforeinput event prevents the default the end root gets the event too.

Ok -- I'll add one in this patch.

>> Source/WebCore/editing/Editor.cpp:1041
>> +static void dispatchInputEvents(PassRefPtr<Element> prpStartRoot, PassRefPtr<Element> prpEndRoot, const AtomicString& inputTypeName)
> 
> If we are renaming this function, then we should also get rid of the archaic use of PassRefPtr here. Can just change these to RefPtr or RefPtr&&.

Ok! Changed to use RefPtr.

>> Source/WebCore/editing/Editor.cpp:1057
>> +    return dispatchBeforeInputEvents(composition->startingRootEditableElement(), composition->endingRootEditableElement(), emptyString());
> 
> Why is empty string OK here? Do we have tests that cover this?

This is a stub until we implement InputEvent.inputType. I will add tests to verify that we have the proper inputType when I land support for this attribute in the next patch.

>> Source/WebCore/editing/Editor.cpp:1076
>> +    dispatchInputEvents(composition->startingRootEditableElement(), composition->endingRootEditableElement(), emptyString());
> 
> Why is empty string OK here? Do we have tests that cover this?

Please see above.

>> Source/WebCore/editing/Editor.cpp:1101
>> +    return dispatchBeforeInputEvents(composition.startingRootEditableElement(), composition.endingRootEditableElement(), emptyString());
> 
> Why is empty string OK here? Do we have tests that cover this?

Please see above.

>> Source/WebCore/editing/Editor.cpp:3105
>> +        element->dispatchInputEvent(emptyString());
> 
> Why is empty string OK here? Do we have tests that cover this?

Please see above.

>> Source/WebCore/editing/TypingCommand.cpp:272
>> +    return true;
> 
> I think we should write this the other way around. First, the unusual case, and then the normal case.
> 
>     if (!m_isHandlingInitialTypingCommand) {
>         // The TypingCommand will handle the willApplyCommand logic separately in TypingCommand::willAddTypingToOpenCommand.
>         return true;
>     }
> 
>     return CompositeEditCommand::willApplyCommand();

Sounds good -- changed.

>> Source/WebCore/editing/TypingCommand.cpp:377
>> +    // correct inputType is dispatched.
> 
> In our project we use FIXME, and not TODO.

Good catch -- s/TODO/FIXME/.

>> Source/WebCore/editing/TypingCommand.h:89
>> +    void didApplyCommand() override;
> 
> These should be private and final, rather than protected and override, unless I am missing some case where a derived class wants to call or override these.

Done.

>>> Source/WebCore/editing/TypingCommand.h:114
>>> +    bool shouldRetainAutocorrectionIndicator() const override
>> 
>> These should be final rather than override unless they need to be overridden further.
> 
> Or maybe we can make TypingCommand final if nothing inherits from it?

Done. (made TypingCommand final).

>> Source/WebCore/editing/TypingCommand.h:120
>> +    bool shouldStopCaretBlinking() const override { return true; }
> 
> Ditto.

Made TypingCommand final.

[WebKit Commit Bot]

Comment on attachment 290983 [details]
Patch for landing

Clearing flags on attachment: 290983

Committed r206944: <http://trac.webkit.org/changeset/206944>

[Darin Adler]

Comment on attachment 290858 [details]
First pass

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

>>>> Source/WebCore/editing/TypingCommand.h:114
>>>> +    bool shouldRetainAutocorrectionIndicator() const override
>>> 
>>> These should be final rather than override unless they need to be overridden further.
>> 
>> Or maybe we can make TypingCommand final if nothing inherits from it?
> 
> Done. (made TypingCommand final).

Making TypingCommand final was a good idea. But now these functions are marked neither override nor final, and thus there is nothing asserting that these override a function from the base class. It would be good to mark each of these functions final, that way we’ll get an error if they no longer override a function from the base class.

[Wenson Hsieh]

(In reply to comment #18)
> Comment on attachment 290858 [details]
> First pass
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290858&action=review
> 
> >>>> Source/WebCore/editing/TypingCommand.h:114
> >>>> +    bool shouldRetainAutocorrectionIndicator() const override
> >>> 
> >>> These should be final rather than override unless they need to be overridden further.
> >> 
> >> Or maybe we can make TypingCommand final if nothing inherits from it?
> > 
> > Done. (made TypingCommand final).
> 
> Making TypingCommand final was a good idea. But now these functions are
> marked neither override nor final, and thus there is nothing asserting that
> these override a function from the base class. It would be good to mark each
> of these functions final, that way we’ll get an error if they no longer
> override a function from the base class.

This is true. I'll put together a follow-up for this.