Bug 163113 – Support InputEvent.data for the new InputEvent spec

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Wenson Hsieh]

See https://www.w3.org/TR/input-events/ and https://www.w3.org/TR/uievents/ for more details.

[Wenson Hsieh]

<rdar://problem/28681935>

[Wenson Hsieh]

Created attachment 291032 [details]
Test patch (not for review)

[Wenson Hsieh]

Created attachment 291060 [details]
First pass

[Darin Adler]

Comment on attachment 291060 [details]
First pass

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

> Source/WebCore/dom/InputEvent.h:35
>  struct InputEventInit : public UIEventInit {

No need for "public" here.

> Source/WebCore/dom/InputEvent.h:42
> +    static Ref<InputEvent> create(const AtomicString& eventType, const String& inputType, bool canBubble, bool cancelable, DOMWindow* view, String data = String(), int detail = 0)

Argument type should be const String& or perhaps String&&. Default can be { } rather than String(). But also, I am not sure we want default argument values here.

> Source/WebCore/dom/InputEvent.h:52
> +    InputEvent(const AtomicString& eventType, const String& inputType, bool canBubble, bool cancelable, DOMWindow*, String data, int detail);

Argument type should be const String& or perhaps String&&.

> Source/WebCore/dom/InputEvent.h:59
>      String inputType() const { return m_inputType; }

Return type should be const String&.

> Source/WebCore/dom/InputEvent.h:60
> +    String data() const { return m_data; }

Return type should be const String&.

> Source/WebCore/dom/Node.h:528
> +    virtual void dispatchInputEvent(const String& inputType, String data = String());

This function should not be virtual. There are no overrides of it.

As with most functions that dispatch an event to an event target, this function should not be a member function. Instead it should go where it is being used, perhaps in Editor.cpp, and likely the argument type can be something like Element& rather than Node&.

The new argument to this function should be const String& rather than String. Or possibly String&& depending on how it is used.

The default can be written { } rather than String(). That’s what we use in most new call sites like this one.

> Source/WebCore/editing/CompositeEditCommand.h:118
> +    virtual String inputEventData() const { return String(); }

Can use { } rather than String() to return a null string.

> Source/WebCore/editing/EditCommand.cpp:148
> +    if (auto* frame = m_document->frame())
> +        return frame->selection().selection().isInTextAreaOrTextInput();
> +
> +    return false;

WebKit code calls for early return style, which means the error case is the return, not the main case. Or you could use &&:

    auto* frame = m_document->frame();
    return frame && frame->selection().selection().isInTextAreaOrTextInput();

> Source/WebCore/editing/Editor.cpp:114
> +static bool dispatchBeforeInputEvent(Element& element, const AtomicString& inputType, String data = String())

The new argument to this function should be const String& rather than String. Or possibly String&& depending on how it is used.

The default can be written { } rather than String(). That’s what we use in most new call sites like this one.

> Source/WebCore/editing/Editor.cpp:1046
> +static bool dispatchBeforeInputEvents(RefPtr<Element> startRoot, RefPtr<Element> endRoot, const AtomicString& inputTypeName, String data = String())

The new argument to this function should be const String& rather than String. Or possibly String&& depending on how it is used.

The default can be written { } rather than String(). That’s what we use in most new call sites like this one.

> Source/WebCore/editing/Editor.cpp:1056
> +static void dispatchInputEvents(RefPtr<Element> startRoot, RefPtr<Element> endRoot, const AtomicString& inputTypeName, String data = String())

The new argument to this function should be const String& rather than String. Or possibly String&& depending on how it is used.

The default can be written { } rather than String(). That’s what we use in most new call sites like this one.

> Source/WebCore/editing/ReplaceRangeWithTextCommand.h:40
> +    String inputEventData() const final;

I suggest making this override private rather than putting it in the public. I don’t think anyone needs to call this non-polymorphically.

> Source/WebCore/editing/SpellingCorrectionCommand.h:41
> +    String inputEventData() const final;

I suggest making this override private rather than putting it in the public. I don’t think anyone needs to call this non-polymorphically.

> Source/WebCore/editing/VisibleSelection.cpp:671
> +    Node* container = start().containerNode();

I suggest using auto* here. The type will be ContainerNode*, I think, not Node*, and that might even lead to slightly more efficient code.

> Source/WebCore/editing/VisibleSelection.h:108
> +    bool isInTextAreaOrTextInput() const;

Can we put all of this logic into TypingCommand::isEditingTextAreaOrTextInput() and local helper functions if needed? We can always move the function here if someone else needs it, but I’d suggest keeping this concept all in one place for now.

[Wenson Hsieh]

Created attachment 291123 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291123 [details]
Patch for landing

Clearing flags on attachment: 291123

Committed r207010: <http://trac.webkit.org/changeset/207010>

[Darin Adler]

Comment on attachment 291123 [details]
Patch for landing

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

> Source/WebCore/html/HTMLFormControlElement.cpp:329
> +    HTMLElement::dispatchInputEvent();

This is a peculiar line of code. It should just call dispatchInputEvent, not HTMLElement::dispatchInputEvent, which is a peculiar way to write the same thing.

[Wenson Hsieh]

Comment on attachment 291123 [details]
Patch for landing

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

>> Source/WebCore/html/HTMLFormControlElement.cpp:329
>> +    HTMLElement::dispatchInputEvent();
> 
> This is a peculiar line of code. It should just call dispatchInputEvent, not HTMLElement::dispatchInputEvent, which is a peculiar way to write the same thing.

Good catch. I will address this in a followup: https://bugs.webkit.org/show_bug.cgi?id=163236