Escape should clear search field

• Component: Forms
• Status: RESOLVED FIXED

[Evan Stade]

pressing escape in a <input type="search"> field should clear the contents (same as if the user presses the x)

[Kentaro Hara]

Created attachment 100667 [details]
Patch

[Evan Martin]

Comment on attachment 100667 [details]
Patch

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

> Source/WebCore/html/SearchInputType.cpp:113
> +    const String& key = event->keyIdentifier();
> +    if (key == "U+001B") {
> +        HTMLInputElement* input = element();
> +        input->setValueForUser("");
> +        input->onSearch();
> +        event->setDefaultHandled();
> +    } else
> +        TextFieldInputType::handleKeydownEvent(event);

Does this mean it's impossible for content to handle 'esc' specially?  I wonder if this maybe belongs on keypress rather than keydown.

[Kent Tamura]

Comment on attachment 100667 [details]
Patch

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

r- because of possible use-after-free.


> LayoutTests/fast/forms/input-search-press-escape-key.html:28
> +    eventSender.keyDown("escape");

Do you need to use eventSender?
Doesn't the following event dispatching work?
 var event = document.createEvent('KeyboardEvents');
 event.initKeyboardEvent(...);
 input.dispatchEvent(event);

> Source/WebCore/html/SearchInputType.cpp:110
> +        input->setValueForUser("");
> +        input->onSearch();

setValueForUser() dispatches 'change' event and a JavaScript handler can delete 'input'.
You need to use RefPtr<HTMLInputElement>.

>> Source/WebCore/html/SearchInputType.cpp:113
>> +        TextFieldInputType::handleKeydownEvent(event);
> 
> Does this mean it's impossible for content to handle 'esc' specially?  I wonder if this maybe belongs on keypress rather than keydown.

Agree with Evan.

[Kentaro Hara]

Comment on attachment 100667 [details]
Patch

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

>> LayoutTests/fast/forms/input-search-press-escape-key.html:28
>> +    eventSender.keyDown("escape");
> 
> Do you need to use eventSender?
> Doesn't the following event dispatching work?
>  var event = document.createEvent('KeyboardEvents');
>  event.initKeyboardEvent(...);
>  input.dispatchEvent(event);

Keyboard event dispatching does fire the event but does nothing because of the bug: http://code.google.com/p/chromium/issues/detail?id=52408
This bug happens on not only Linux Chromium 14.0.822.0 but also Mac Safari 5.0.5 and even Linux Firefox 5.0. 

You can test it here:
http://haraken.info/null/keyevent.html

Provided that the above test code is not wrong, I am not sure but I guess that this is not a bug and there must be some strong reason (security or something?) that browsers are forbidding dispatching keyboard events. But if this is really a bug to be fixed and you think that we should first fix the keyboard event dispatching bug, then I will fix it and then come back to this search form bug later.

>> Source/WebCore/html/SearchInputType.cpp:110
>> +        input->onSearch();
> 
> setValueForUser() dispatches 'change' event and a JavaScript handler can delete 'input'.
> You need to use RefPtr<HTMLInputElement>.

OK. I will fix it in the next patch.

>>> Source/WebCore/html/SearchInputType.cpp:113
>>> +        TextFieldInputType::handleKeydownEvent(event);
>> 
>> Does this mean it's impossible for content to handle 'esc' specially?  I wonder if this maybe belongs on keypress rather than keydown.
> 
> Agree with Evan.

I am not sure for this. I agree that it becomes impossible for content to handle 'Escape', if we handle it in handleKeydownEvent(). However, it seems that most key events for input elements are being handled not by handleKeypressEvent() but by handleKeydownEvent(). For example, TextFieldInputType.cpp, NumberInputType.cpp and RangeInputType.cpp have handleKeydownEvent() but do not have handleKeypressEvent(). Also, if we handle 'Escape' in handleKeypressEvent(), then it becomes impossible to test it by eventSender.keyDown("escape") (, although whether we should test it by eventSender.keyDown() or not is debatable as I described above).

[Kent Tamura]

Comment on attachment 100667 [details]
Patch

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

>>> LayoutTests/fast/forms/input-search-press-escape-key.html:28
>>> +    eventSender.keyDown("escape");
>> 
>> Do you need to use eventSender?
>> Doesn't the following event dispatching work?
>>  var event = document.createEvent('KeyboardEvents');
>>  event.initKeyboardEvent(...);
>>  input.dispatchEvent(event);
> 
> Keyboard event dispatching does fire the event but does nothing because of the bug: http://code.google.com/p/chromium/issues/detail?id=52408
> This bug happens on not only Linux Chromium 14.0.822.0 but also Mac Safari 5.0.5 and even Linux Firefox 5.0. 
> 
> You can test it here:
> http://haraken.info/null/keyevent.html
> 
> Provided that the above test code is not wrong, I am not sure but I guess that this is not a bug and there must be some strong reason (security or something?) that browsers are forbidding dispatching keyboard events. But if this is really a bug to be fixed and you think that we should first fix the keyboard event dispatching bug, then I will fix it and then come back to this search form bug later.

ok, I understand createEvent&initKeyboardEvet doesn't work.

>>>> Source/WebCore/html/SearchInputType.cpp:113
>>>> +        TextFieldInputType::handleKeydownEvent(event);
>>> 
>>> Does this mean it's impossible for content to handle 'esc' specially?  I wonder if this maybe belongs on keypress rather than keydown.
>> 
>> Agree with Evan.
> 
> I am not sure for this. I agree that it becomes impossible for content to handle 'Escape', if we handle it in handleKeydownEvent(). However, it seems that most key events for input elements are being handled not by handleKeypressEvent() but by handleKeydownEvent(). For example, TextFieldInputType.cpp, NumberInputType.cpp and RangeInputType.cpp have handleKeydownEvent() but do not have handleKeypressEvent(). Also, if we handle 'Escape' in handleKeypressEvent(), then it becomes impossible to test it by eventSender.keyDown("escape") (, although whether we should test it by eventSender.keyDown() or not is debatable as I described above).

I think many existing key-binding code should be switched to keypress.
eventSender.keyDown() dispatched keydown, keyup, and keypress.

[Ojan Vafai]

Comment on attachment 100667 [details]
Patch

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

>>>>> Source/WebCore/html/SearchInputType.cpp:113
>>>>> +        TextFieldInputType::handleKeydownEvent(event);
>>>> 
>>>> Does this mean it's impossible for content to handle 'esc' specially?  I wonder if this maybe belongs on keypress rather than keydown.
>>> 
>>> Agree with Evan.
>> 
>> I am not sure for this. I agree that it becomes impossible for content to handle 'Escape', if we handle it in handleKeydownEvent(). However, it seems that most key events for input elements are being handled not by handleKeypressEvent() but by handleKeydownEvent(). For example, TextFieldInputType.cpp, NumberInputType.cpp and RangeInputType.cpp have handleKeydownEvent() but do not have handleKeypressEvent(). Also, if we handle 'Escape' in handleKeypressEvent(), then it becomes impossible to test it by eventSender.keyDown("escape") (, although whether we should test it by eventSender.keyDown() or not is debatable as I described above).
> 
> I think many existing key-binding code should be switched to keypress.
> eventSender.keyDown() dispatched keydown, keyup, and keypress.

Looking at EventDispatcher::dispatchEvent, we first fire the event to JS and then call defaultEventHandler. haraken, can you confirm that if you call preventDefault from the esc key's keydown event that we don't clear the search input?

That said, I think tkent is probably right that most, if not all, of the existing key-binding code should be moved to handleKeyPressEvent. I'm not 100% sure of that though. Would be nice to get input from someone involved in putting it on handleKeydownEvent in the first place. haraken, you could look at the blame list for some other instances of it and CC the appropriate developers on this bug to get them to comment.

[Kentaro Hara]

Thank you for comments!

ojan:
> Looking at EventDispatcher::dispatchEvent, we first fire the event to JS and then call defaultEventHandler. haraken, can you confirm that if you call preventDefault from the esc key's keydown event that we don't clear the search input?

You are right. I confirmed that we can prevent WebCore from calling defaultEventHandler() by calling preventDefault() in JS. 


tkent:
> > I think many existing key-binding code should be switched to keypress.
> > eventSender.keyDown() dispatched keydown, keyup, and keypress.

Indeed we may switch those key-binding codes to keypress, but at the present stage, there are several problems to try to handle those key-bindings in the keypress event. 

[1] xxxxxxInputType::handleKeypressEvent() is not called. 

HTMLInputElement.cpp:
void HTMLInputElement::defaultEventHandler(Event* evt) {
    ...;

    /*** part1 ***/
    // Call the base event handler before any of our own event handling for almost all events in text fields.
    // Makes editing keyboard handling take precedence over the keydown and keypress handling in this function.
    bool callBaseClassEarly = isTextField() && (evt->type() == eventNames().keydownEvent || evt->type() == eventNames().keypressEvent);
    if (callBaseClassEarly) {
        HTMLFormControlElementWithState::defaultEventHandler(evt);
        if (evt->defaultHandled())
            return;
    }

    /*** part2 ***/
    // DOMActivate events cause the input to be "activated" - in the case of image and submit inputs, this means
    // actually submitting the form. For reset inputs, the form is reset. These events are sent when the user clicks
    // on the element, or presses enter while it is the active element. JavaScript code wishing to activate the element
    // must dispatch a DOMActivate event - a click event will not do the job.
    if (evt->type() == eventNames().DOMActivateEvent) {
        m_inputType->handleDOMActivateEvent(evt);
        if (evt->defaultHandled())
            return;
    }

    /*** part3 ***/
    // Use key press event here since sending simulated mouse events
    // on key down blocks the proper sending of the key press event.
    if (evt->isKeyboardEvent() && evt->type() == eventNames().keypressEvent) {
        m_inputType->handleKeypressEvent(static_cast<KeyboardEvent*>(evt));
        if (evt->defaultHandled())
            return;
    }
    ...;
}

What is happening is that since HTMLFormControlElementWithState::defaultEventHandler(evt) calls setDefaultHandled() deep inside the call, the method returns at the end of part1 and thus m_inputType->handleKeypressEvent(static_cast<KeyboardEvent*>(evt)) is never called. 

I guess that this problem can be safely resolved just by reordering the above codes to part3 -> part2 -> part1. 


[2] EventSender::keyDown() does not dispatch a keypress event for control characters, including 'escape'. For example, in chromium...

chromium/EventSender.cpp:
void EventSender::keyDown(const CppArgumentList& arguments, CppVariant* result) {
    ...;

    webview()->handleInputEvent(eventDown);  // keydown event

    m_shell->webViewHost()->clearEditCommand();

    if (generateChar) { // only if it is not a control character...
        eventChar.type = WebInputEvent::Char;
        eventChar.keyIdentifier[0] = '\0';
        webview()->handleInputEvent(eventChar);  // keypress event
    }

    webview()->handleInputEvent(eventUp);  // keyup event
}

Also, I confirmed that mac/EventSendingController.mm dispatches the keypress event only for non-control characters. I am not sure for efl, gtk, qt and win, but anyway, we definitely need per-platform changes. 

I think that it is possible to make the fix so that it always dispatches the keypress event, but I am not sure whether this fix is acceptable.

[Kent Tamura]

(In reply to comment #7)
> [2] EventSender::keyDown() does not dispatch a keypress event for control characters, including 'escape'. For example, in chromium...

I checked the specification:

http://www.w3.org/TR/DOM-Level-3-Events/#event-type-keypress
> A user agent must dispatch this event when a key is pressed down, if and only if that key normally produces a character value. 

I think it's the reason we don't produce keypress for control characters.
So, using keydown event is very reasonable in this case.

[Kentaro Hara]

Created attachment 100922 [details]
Patch

[Kentaro Hara]

Comment on attachment 100667 [details]
Patch

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

>>> Source/WebCore/html/SearchInputType.cpp:110
>>> +        input->onSearch();
>> 
>> setValueForUser() dispatches 'change' event and a JavaScript handler can delete 'input'.
>> You need to use RefPtr<HTMLInputElement>.
> 
> OK. I will fix it in the next patch.

Done.

[Darin Adler]

Comment on attachment 100922 [details]
Patch

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

> LayoutTests/fast/forms/input-search-press-escape-key.html:28
> +    eventSender.keyDown("escape");

Why can’t we just use "\x1B" here?

> Source/WebCore/html/SearchInputType.cpp:104
> +        return;

Why are we not calling TextFieldInputType::handleKeydownEvent in this case?

> Source/WebCore/html/SearchInputType.cpp:113
> +        event->setDefaultHandled();
> +    } else
> +        TextFieldInputType::handleKeydownEvent(event);

We'd normally use a return here instead of an else.

[Darin Adler]

On Mac escape should only clear the search field if we already have dismissed the menu.

[Kentaro Hara]

Created attachment 100927 [details]
Patch

[Kentaro Hara]

Comment on attachment 100922 [details]
Patch

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

>> LayoutTests/fast/forms/input-search-press-escape-key.html:28
>> +    eventSender.keyDown("escape");
> 
> Why can’t we just use "\x1B" here?

Great idea! I fixed it and removed all per-platform changes in EventSender.

>> Source/WebCore/html/SearchInputType.cpp:104
>> +        return;
> 
> Why are we not calling TextFieldInputType::handleKeydownEvent in this case?

Fixed.

>> Source/WebCore/html/SearchInputType.cpp:113
>> +        TextFieldInputType::handleKeydownEvent(event);
> 
> We'd normally use a return here instead of an else.

Fixed.

[Kentaro Hara]

> On Mac escape should only clear the search field if we already have dismissed the menu.

Do you mean the menu of the search field that suggests a list of matched candidates with the input value? On my Mac 10.6 environment, when I press 'escape', the search field is cleared even if the menu is being appeared.

[Kent Tamura]

(In reply to comment #15)
> > On Mac escape should only clear the search field if we already have dismissed the menu.
> 
> Do you mean the menu of the search field that suggests a list of matched candidates with the input value? On my Mac 10.6 environment, when I press 'escape', the search field is cleared even if the menu is being appeared.

With search fields on the upper-right corner of Xcode and Safari 5, escape key dismisses a menu and doesn't clear the text.

[Kentaro Hara]

darin:
> On Mac escape should only clear the search field if we already have dismissed the menu.

I manually confirmed on Mac Safari 5.0.5 that the search field is cleared only if we have dismissed the menu, using the following html:

<input type="search" value="Test" results="20" onsearch="alert('onsearch')" />


tkent:
> With search fields on the upper-right corner of Xcode and Safari 5, escape key dismisses a menu and doesn't clear the text.

Thanks, I confirmed it. 

FYI: However, with search fields on the upper-right corner of "System Preferences", escape key clears the text even if a menu is being displayed.

[Kent Tamura]

Comment on attachment 100927 [details]
Patch

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

r- because of patch conflicts.

> LayoutTests/fast/forms/input-search-press-escape-key.html:65
> +}
> +

Please show a manual test instruction if there is no layoutTestController or eventSender.

> Source/WebCore/html/SearchInputType.cpp:107
> +        if (input->disabled() || input->readOnly())
> +            return;

TextFieldInputType::handleKeydownEvent() should be called even if the input is not editable.
It doesn't affect any behavior.  It's for consistency.

[Kentaro Hara]

Created attachment 100934 [details]
Patch

[Kent Tamura]

Comment on attachment 100934 [details]
Patch

r- because of patch conflicts

[Kentaro Hara]

Created attachment 100940 [details]
Patch

[Kent Tamura]

Comment on attachment 100940 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 100940 [details]
Patch

Clearing flags on attachment: 100940

Committed r91058: <http://trac.webkit.org/changeset/91058>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.