Bug 54152 – typing enter in the input element should not fire textInput

• Component: Forms
• Status: RESOLVED FIXED

[Ojan Vafai]

Created attachment 81891 [details]
test case

As per spec, we should only fire this event when text is actually inserted: http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-textinput.

[Ryosuke Niwa]

I talked with Alice, who is tackling this bug.  It seems like we should avoid sending textInput event in EventHandler::handleTextInputEvent or executeInsertNewline depending on the value of InputType::shouldSubmitImplicitly.  Preventing calls earlier than that will result in not calling EditorClient::WebEditorClient::handleKeyboardEvent, and may break some WebKit applications.

Alice wanted to know where we prevent the insertion of \n in input elements because that seems to be another appropriate place to add this kind of check but I don't know the answer.  My best guess is HTMLInputElement::defaultEventHandler where we check the value of shouldSubmitImplicitly and exit early.

[Alice Boxhall]

Created attachment 83266 [details]
work in progress

[Ryosuke Niwa]

Comment on attachment 83266 [details]
work in progress

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

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

This member function as well as InputType::shouldSubmitImplicitly should be qualified as const since they don't modify anything.

> Source/WebCore/page/EventHandler.cpp:2726
> +    if (targetNode && targetNode->isHTMLElement() && targetNode->hasTagName(inputTag)) {

I'm worried that this will leave WML broken but then I'm not even sure if we fire inputText event in WML documents.

> Source/WebCore/page/EventHandler.cpp:2727
> +        HTMLInputElement* targetElement = (HTMLInputElement*) targetNode;

We don't use C-type casts.  Please do static_cast<InputElement*>(targetNode).

> Source/WebCore/page/EventHandler.cpp:2729
> +        if (targetElement->shouldSubmitImplicitly(event.get()))
> +            event->stopPropagation();

I would have combined these two if statements as in:
if (targetNode && targetNode->isHTMLElement() && targetNode->hasTagName(inputTag)
    && static_cast<HTMLInputElement*>(targetNode)->shouldSubmitImplicitly(event.get()))
    event->StopPropagation();

[Ryosuke Niwa]

Nikolas,

Do we fire textInput event in WML?  If so, how are we submitting the form on \r?  As far as I read the code in WMLInputElement::defaultEventHandler, I can't reason how it's simulating mouse click when clickDefaultFormButton is true.

[Alice Boxhall]

Created attachment 83392 [details]
work in progress

[Alice Boxhall]

LayoutTest is still to come; meanwhile...

(In reply to comment #3)
> (From update of attachment 83266 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83266&action=review
> 
> > Source/WebCore/html/HTMLInputElement.h:186
> > +    bool shouldSubmitImplicitly(Event* evt) { return m_inputType->shouldSubmitImplicitly(evt); }
> 
> This member function as well as InputType::shouldSubmitImplicitly should be qualified as const since they don't modify anything.

Done.

> > Source/WebCore/page/EventHandler.cpp:2726
> > +    if (targetNode && targetNode->isHTMLElement() && targetNode->hasTagName(inputTag)) {
> 
> I'm worried that this will leave WML broken but then I'm not even sure if we fire inputText event in WML documents.

Obviously, this is well outside the scope of my knowledge, so I'll wait to hear from someone who knows more.

> > Source/WebCore/page/EventHandler.cpp:2727
> > +        HTMLInputElement* targetElement = (HTMLInputElement*) targetNode;
> 
> We don't use C-type casts.  Please do static_cast<InputElement*>(targetNode).

Done. (Oops.)

> > Source/WebCore/page/EventHandler.cpp:2729
> > +        if (targetElement->shouldSubmitImplicitly(event.get()))
> > +            event->stopPropagation();
> 
> I would have combined these two if statements as in:
> if (targetNode && targetNode->isHTMLElement() && targetNode->hasTagName(inputTag)
>     && static_cast<HTMLInputElement*>(targetNode)->shouldSubmitImplicitly(event.get()))
>     event->StopPropagation();

Hm, ok; that looks funny to me, but if you tell me that's the WebKit way I'm happy to do it.

[Darin Adler]

(In reply to comment #3)
> > +    if (targetNode && targetNode->isHTMLElement() && targetNode->hasTagName(inputTag)) {
> 
> I'm worried that this will leave WML broken but then I'm not even sure if we fire inputText event in WML documents.

hasTagName(inputTag) is all that's needed. If true it guarantees the node is an HTMLInputElement. So no separate check for isHTMLElement is needed.

[Alice Boxhall]

Created attachment 83600 [details]
with layout test

[Ryosuke Niwa]

Comment on attachment 83600 [details]
with layout test

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

The code change looks good but you need to explain what you're doing.  Also, the layout test can be simplified further.

> LayoutTests/ChangeLog:7
> +

You should explain what kind of a test you're adding.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:1
> +<html>

Missing <!DOCTYPE html>

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:6
> +    <script>
> +    function log(msg) {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + '\n'));
> +    }

We don't normally indent tag like this.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:19
> +        } catch (e) {}  // Ignore ReferenceError if eventSender is undefined

You should exit early when eventSender is not defined.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:27
> +            var resultDiv = document.getElementById('result');
> +            resultDiv.firstChild.innerHTML = 'FAIL';

You should call log instead.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:33
> +        if (lastKeyPressed == 13)

What does 13 mean?

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:34
> +            document.getElementById('result').style.setProperty('display', 'block');

I don't think it's great that you hide/show SUCCESS like this.  You should call log on console instead.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:50
> +    function loaded() {
> +        var el = document.getElementById('el');
> +        el.addEventListener('keypress', setLastKeyPressed);
> +        el.addEventListener('keypress', showResults);
> +        el.addEventListener('input', fail);
> +        el.addEventListener('textInput', fail);
> +
> +        runTest();
> +    }

I don't think it's necessary to wait until page loads.  You should just place this entire script element below pre and just run the test there while parsing is done.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:57
> +<br><br>

Do we need these BRs?

> Source/WebCore/ChangeLog:7
> +

You should explain what caused the bug and how you fixed it.

[Ojan Vafai]

Comment on attachment 83600 [details]
with layout test

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

>> LayoutTests/ChangeLog:7
>> +
> 
> You should explain what kind of a test you're adding.

Really? What needs explanation here?

>> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:6
>> +    }
> 
> We don't normally indent tag like this.

I think you meant something like: We don't normally indent the contents of the head and body tags in tests.

> Source/WebCore/ChangeLog:14
> +        * html/HTMLInputElement.cpp:
> +        * html/HTMLInputElement.h:
> +        (WebCore::HTMLInputElement::shouldSubmitImplicitly):
> +        * page/EventHandler.cpp:
> +        (WebCore::EventHandler::handleTextInputEvent):

Ideally you'd give a .5-1 sentence explanation of the changes to each file. Also, this file list no longer matches the list of files change. It's annoying but you need to:
1. Delete the changelog entry.
2. prepare-ChangeLog
3. Add your comments back in. 

:(

[Ryosuke Niwa]

Comment on attachment 83600 [details]
with layout test

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

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:25
> +        log((unexpected ? 'Unexpected ' : '') + event.type + ' fired.');
> +        if (unexpected) {

You can rewrite this as:
if (lastKeyPressed = '\r'.charCodeAt(0)) {
    log(event.type + ' fired');
    return;
}
log('Unexpected ' + event.type + ' fired');
...

[Alice Boxhall]

Created attachment 83607 [details]
work in progress; updated layout test

[Ryosuke Niwa]

Comment on attachment 83607 [details]
work in progress; updated layout test

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

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:4
> +<head>
> +</head>

Nit: unnecessary.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:9
> +<br><br>

Again, I don't think we need these BRs here.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:10
> +<div id="result"><span style='padding: 5px'></span></div>

Do we really need a span inside?

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:17
> +var enterChar = '\r'.charCodeAt(0);

Ah, this is great!

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:27
> +    resultDiv.firstChild.style.setProperty('background-color', 'red');

I'd write it as resultDiv.firstChild.style.backgroundColor = 'red' or better yet: resultDiv.innerHTML = '<font color="red">FAIL</font>';

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:32
> +    if (resultDiv.firstChild.innerHTML != 'FAIL') {

If you had made the change above, you'll do resultDiv.innerText.indexOf('FAIL') < 0).

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:34
> +        resultDiv.firstChild.innerHTML = 'SUCCESS';
> +        resultDiv.firstChild.style.setProperty('background-color', 'green');

Ditto.  I'd write it as resultDiv.innerHTML = '<font color="red">SUCCESS</font>'.  In that case you don't even need curly brackets around the line.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:44
> +function setLastKeyPressed(event) {
> +    lastKeyPressed = event.keyCode;
> +}
> +
> +var el = document.getElementById('el');
> +el.addEventListener('keypress', setLastKeyPressed);

I'd use anonymous function as in:
el.addEventListener('keypress', function(event) { lastKeyPressed = event.keyCode; })

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:54
> +    var el = document.getElementById('el');

??

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:56
> +    succeedIfEventNotSent();

I don't think succeedIfEventNotSent needs to be a separate function.  You can just put its definition here.

[Nikolas Zimmermann]

(In reply to comment #4)
> Nikolas,
> 
> Do we fire textInput event in WML?  If so, how are we submitting the form on \r?  As far as I read the code in WMLInputElement::defaultEventHandler, I can't reason how it's simulating mouse click when clickDefaultFormButton is true.

Hm not sure at all, long ago since I last looked at the WML code. All I can say, WML doesn't support JS Scripting at all, so we don't need any events for WML in that regard. WMLInputElement, IIRC, is just a copy&paste of existing HTMLInputElement, and stripped down for WML. If it diverges, feel free to align them -- if WML is still used, (Samsung?) they'd notice if anything goes wrong. And I think you shouldn't have to care for WML (except that it still builds), as it's off-by-default.

[Alice Boxhall]

(In reply to comment #13)
> (From update of attachment 83607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83607&action=review
> 
> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:4
> > +<head>
> > +</head>
> 
> Nit: unnecessary.

Done.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:9
> > +<br><br>
> 
> Again, I don't think we need these BRs here.

They aren't necessary, but space things out a bit better when running the test manually.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:10
> > +<div id="result"><span style='padding: 5px'></span></div>
> 
> Do we really need a span inside?

Not really, but it makes it look a little better when running manually.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:17
> > +var enterChar = '\r'.charCodeAt(0);
> 
> Ah, this is great!

:-)

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:27
> > +    resultDiv.firstChild.style.setProperty('background-color', 'red');
> 
> I'd write it as resultDiv.firstChild.style.backgroundColor = 'red' or better yet: resultDiv.innerHTML = '<font color="red">FAIL</font>';

I changed my mind again on how to do this. Now adding the span in here and checking for child nodes in the succeedIfEventNotFired block.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:32
> > +    if (resultDiv.firstChild.innerHTML != 'FAIL') {
> 
> If you had made the change above, you'll do resultDiv.innerText.indexOf('FAIL') < 0).

See above.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:34
> > +        resultDiv.firstChild.innerHTML = 'SUCCESS';
> > +        resultDiv.firstChild.style.setProperty('background-color', 'green');
> 
> Ditto.  I'd write it as resultDiv.innerHTML = '<font color="red">SUCCESS</font>'.  In that case you don't even need curly brackets around the line.

Did something similar.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:44
> > +function setLastKeyPressed(event) {
> > +    lastKeyPressed = event.keyCode;
> > +}
> > +
> > +var el = document.getElementById('el');
> > +el.addEventListener('keypress', setLastKeyPressed);
> 
> I'd use anonymous function as in:
> el.addEventListener('keypress', function(event) { lastKeyPressed = event.keyCode; })

Done.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:54
> > +    var el = document.getElementById('el');
> 
> ??

Removed.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:56
> > +    succeedIfEventNotSent();
> 
> I don't think succeedIfEventNotSent needs to be a separate function.  You can just put its definition here.

I did that, and then slightly reworked the way the test worked because it was annoying me that it didn't have an obvious 'success' condition when running manually. Now I listen for a submit event and show the success message on that, or in this location if submit doesn't fire (which is also a bug, but tested in other cases).

(In reply to comment #10)
> (From update of attachment 83600 [details])
> > Source/WebCore/ChangeLog:14
> > +        * html/HTMLInputElement.cpp:
> > +        * html/HTMLInputElement.h:
> > +        (WebCore::HTMLInputElement::shouldSubmitImplicitly):
> > +        * page/EventHandler.cpp:
> > +        (WebCore::EventHandler::handleTextInputEvent):
> 
> Ideally you'd give a .5-1 sentence explanation of the changes to each file. Also, this file list no longer matches the list of files change. It's annoying but you need to:
> 1. Delete the changelog entry.
> 2. prepare-ChangeLog
> 3. Add your comments back in. 
> 
> :(

Hehe, done. The most painful part was working out how to convince prepare-ChangeLog to find my changes.

(In reply to comment #7)
> (In reply to comment #3)
> > > +    if (targetNode && targetNode->isHTMLElement() && targetNode->hasTagName(inputTag)) {
> > 
> > I'm worried that this will leave WML broken but then I'm not even sure if we fire inputText event in WML documents.
> 
> hasTagName(inputTag) is all that's needed. If true it guarantees the node is an HTMLInputElement. So no separate check for isHTMLElement is needed.

Done.

(In reply to comment #9)
> > Source/WebCore/ChangeLog:7
> > +
> 
> You should explain what caused the bug and how you fixed it.

I've added some comments, do they cover it?

[Alice Boxhall]

Created attachment 83746 [details]
Patch

[Darin Adler]

Comment on attachment 83746 [details]
Patch

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.

[Ryosuke Niwa]

Comment on attachment 83746 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2728
>> +        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.

Ah! this is a much better solution.

[Ryosuke Niwa]

Comment on attachment 83746 [details]
Patch

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

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:32
> +    if (!unexpectedEvents && !result.hasChildNodes())
> +        result.innerHTML = '<span style="padding: 5px; background-color: green">SUCCESS</span>';

If you make the change below, then you can move all of this into the anonymous function.

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:55
> +    // Just in case something goes wrong with the submit event, there are other tests for that
> +    succeedIfNoUnexpectedEvent();

I don't think we should be doing this.  We should just fail if something goes wrong.

[Alice Boxhall]

Created attachment 83764 [details]
Patch

[Alice Boxhall]

(In reply to comment #17)
> (From update of attachment 83746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83746&action=review

Reverted all other code and implemented your suggestion below.

> > 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.

Done.

(In reply to comment #19)
> (From update of attachment 83746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83746&action=review
> 
> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:32
> > +    if (!unexpectedEvents && !result.hasChildNodes())
> > +        result.innerHTML = '<span style="padding: 5px; background-color: green">SUCCESS</span>';
> 
> If you make the change below, then you can move all of this into the anonymous function.

Done.

> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:55
> > +    // Just in case something goes wrong with the submit event, there are other tests for that
> > +    succeedIfNoUnexpectedEvent();
> 
> I don't think we should be doing this.  We should just fail if something goes wrong.

Done.

[Alice Boxhall]

Created attachment 83767 [details]
Patch

[Alice Boxhall]

Sorry for the noise; latest patch simply adds a log statement in the LayoutTest in the case that window.eventSender is not found.

[Ojan Vafai]

Comment on attachment 83767 [details]
Patch

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

> LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:58
> +} else {
> +    log('This test requires eventSender to run in LayoutTests.');
> +}

Given that you have instructions to run it manually, I don't see that this log comment adds anything. Any test runner that is running WebKit's layout tests will have eventSender. The only reason we check for eventSender is for running the test manually.

If you really want to keep this in though, WebKit style for one-line else statements is to leave off the brackets.

[Alice Boxhall]

(In reply to comment #24)
> (From update of attachment 83767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83767&action=review
> 
> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:58
> > +} else {
> > +    log('This test requires eventSender to run in LayoutTests.');
> > +}
> 
> Given that you have instructions to run it manually, I don't see that this log comment adds anything. Any test runner that is running WebKit's layout tests will have eventSender. The only reason we check for eventSender is for running the test manually.

rniwa informed me that some platforms actually don't have eventSender, which made this change advisable.
 
> 
> If you really want to keep this in though, WebKit style for one-line else statements is to leave off the brackets.

Ok. I'm sure I'll get used to that soon.

[Ojan Vafai]

(In reply to comment #25)
> rniwa informed me that some platforms actually don't have eventSender, which made this change advisable.

Ryosuke, which platforms don't have eventSender?

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #25)
> > rniwa informed me that some platforms actually don't have eventSender, which made this change advisable.
> 
> Ryosuke, which platforms don't have eventSender?

WebKit2 and others.

(In reply to comment #24)
> (From update of attachment 83767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83767&action=review
> 
> > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:58
> > +} else {
> > +    log('This test requires eventSender to run in LayoutTests.');
> > +}
> 
> Given that you have instructions to run it manually, I don't see that this log comment adds anything. Any test runner that is running WebKit's layout tests will have eventSender. The only reason we check for eventSender is for running the test manually.

Should do:
else if (window.layoutTestController)
    log('This test requires eventSender to run in LayoutTests.');

[Alice Boxhall]

Created attachment 83770 [details]
Patch

[Alice Boxhall]

> (In reply to comment #24)
> > (From update of attachment 83767 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=83767&action=review
> > 
> > > LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html:58
> > > +} else {
> > > +    log('This test requires eventSender to run in LayoutTests.');
> > > +}
> > 
> > Given that you have instructions to run it manually, I don't see that this log comment adds anything. Any test runner that is running WebKit's layout tests will have eventSender. The only reason we check for eventSender is for running the test manually.
> 
> Should do:
> else if (window.layoutTestController)
>     log('This test requires eventSender to run in LayoutTests.');

Done.

(Also, I noticed while doing this that my last patch actually reverted to my previous code change. This one is better!)

[WebKit Commit Bot]

Comment on attachment 83770 [details]
Patch

Clearing flags on attachment: 83770

Committed r79777: <http://trac.webkit.org/changeset/79777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[sideshowbarker]

For the record here, the “typing enter in the input element should not fire textInput” requirement is now also specified in the UI Events Algorithms spec at https://w3c.github.io/uievents/event-algo.html#fire%20key%20input%20events, and https://github.com/web-platform-tests/wpt/pull/44472/files#diff-c6d874752d696fc97ee934bbd2e13f6887425b5d67b6566906597e8f1e23aaa4 has a test.

(I’m mentioning it for any future readers here who may be doing troubleshooting and testing for regressions of this bug — because in working on a patch for bug #268988, I noticed this bug while trying to figure out myself how to update my patch after regressing the existing WebKit test at https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/forms/textinput-not-fired-on-enter-in-input.html and looking at its revision history.)