[webkit-reviews] review denied: [Bug 54152] typing enter in the input element should not fire textInput : [Attachment 83746] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alice Boxhall
<aboxhall at chromium.org>'s request for review:
Bug 54152: typing enter in the input element should not fire textInput
https://bugs.webkit.org/show_bug.cgi?id=54152

Attachment 83746: Patch
https://bugs.webkit.org/attachment.cgi?id=83746&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83746&action=review

> Source/WebCore/html/HTMLInputElement.h:186
> +    bool shouldSubmitImplicitly(Event* evt) const { return
m_inputType->shouldSubmitImplicitly(evt); }

This function definition shouldn’t be inline in the header. We don’t want to
expose the InputType.h header to everyone who includes the file.

We can just declare this here in the header and put the implementation into
HTMLInputElement.cpp.

Also, please name this event, not evt.

> Source/WebCore/html/InputType.cpp:341
> +bool InputType::shouldSubmitImplicitly(Event* event) const

There’s really no point in having “const” functions in the input type class.
Please leave these functions as normal member functions rather than const
member functions.

> Source/WebCore/page/EventHandler.cpp:2728
> +    Node* targetNode = target->toNode();
> +    if (targetNode && targetNode->hasTagName(inputTag)
> +	   &&
static_cast<HTMLInputElement*>(targetNode)->shouldSubmitImplicitly(event.get())
)
> +	   event->stopPropagation();

It doesn’t make a lot of sense to actively call dispatchEvent after calling
stopPropagation. And having input-element-specific code here in the event
handler class is not good, even though we’ve done that before.

I think the cleanest way to do this is to put the code into HTMLInputElement’s
preDispatchEventHandler function.

    if (m_inputType->shouldSubmitImplicitly(event.get())) {
	event->stopPropagation();
	return 0;
    }

You won’t have to touch anything else. Only that one source file,
HTMLInputElement.cpp, and only that one function.