Bug 59071 – <input type="number"> dispatches two blurs when tabbing from an invalid number

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Joseph Pecoraro]

Created attachment 90485 [details]
[TEST] Reduced Test Case

With the following test case: (attached)

    <p>Type "0123" or "a123" into the input and then blur with TAB.</p>
    <input id="x" type="number" onblur="alert('blurred');">

* STEPS TO REPRODUCE
1. Focus the input
2. Input an invalid number ("a123")
3. Tab to blur the input

* EXPECTED RESULTS
1 alert saying "blurred"

* ACTUAL RESULTS
2 alerts saying "blurred"

[Kent Tamura]

I found an extra focus event was also fired during NumberInputType::handleBlurEvent().

In the following stack trace, the deeper updateFromElement() is not needed.  We need to avoid it....

1   WebCore::HTMLTextFormControlElement::dispatchFocusEvent()
2   WebCore::Document::setFocusedNode(WTF::PassRefPtr<WebCore::Node>)
3   WebCore::FocusController::setFocusedNode(WebCore::Node*, WTF::PassRefPtr<WebCore::Frame>)
4   WebCore::SelectionController::setFocusedNodeIfNeeded()
5   WebCore::SelectionController::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::SelectionController::CursorAlignOnScroll, WebCore::TextGranularity, WebCore::DirectionalityPolicy)
6   WebCore::SelectionController::textWillBeReplaced(WebCore::CharacterData*, unsigned int, unsigned int, unsigned int)
7   WebCore::CharacterData::setDataAndUpdate(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned int, unsigned int)
8   WebCore::CharacterData::setData(WTF::String const&, int&)
9   WebCore::replaceChildrenWithText(WebCore::HTMLElement*, WTF::String const&, int&)
10  WebCore::HTMLElement::setInnerText(WTF::String const&, int&)
11  WebCore::RenderTextControl::setInnerTextValue(WTF::String const&)
12  WebCore::RenderTextControlSingleLine::updateFromElement()
13  WebCore::updateFromElementCallback(WebCore::Node*)
14  WebCore::ContainerNode::dispatchPostAttachCallbacks()
15  WebCore::ContainerNode::resumePostAttachCallbacks()
16  WebCore::Document::recalcStyle(WebCore::Node::StyleChange)
17  WebCore::Document::updateStyleIfNeeded()
18  WebCore::Document::updateLayout()
19  WebCore::SelectionController::textWillBeReplaced(WebCore::CharacterData*, unsigned int, unsigned int, unsigned int)
20  WebCore::CharacterData::setDataAndUpdate(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned int, unsigned int)
21  WebCore::CharacterData::setData(WTF::String const&, int&)
22  WebCore::replaceChildrenWithText(WebCore::HTMLElement*, WTF::String const&, int&)
23  WebCore::HTMLElement::setInnerText(WTF::String const&, int&)
24  WebCore::RenderTextControl::setInnerTextValue(WTF::String const&)
25  WebCore::RenderTextControlSingleLine::updateFromElement()
26  WebCore::NumberInputType::handleBlurEvent()
27  WebCore::HTMLInputElement::handleBlurEvent()
28  WebCore::HTMLTextFormControlElement::dispatchBlurEvent()
29  WebCore::Document::setFocusedNode(WTF::PassRefPtr<WebCore::Node>)
30  WebCore::FocusController::advanceFocusInDocumentOrder(WebCore::FocusDirection, WebCore::KeyboardEvent*, bool)
31  WebCore::FocusController::advanceFocus(WebCore::FocusDirection, WebCore::KeyboardEvent*, bool)

[Kent Tamura]

(In reply to comment #1)
> I found an extra focus event was also fired during NumberInputType::handleBlurEvent().
> 
> In the following stack trace, the deeper updateFromElement() is not needed.  We need to avoid it....

It's not a problem.  The root problem is SelectionController::textWillBeReplaced() sets focus on the input element.  We'd like to change the inner text without focusing.

It seems difficult to avoid focus in textWillBeReplaced().  I'm going to introduce Node::beforeBlurEvent(), which is called before setting 0 to Document::m_focusedNode.

[Kent Tamura]

Created attachment 91927 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 91927 [details]
Patch

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

We really should take a deeper look at the whole blur/focus event firing lifecycle. I noticed that dispatchBlurEvent is actually performing some of the "do stuff before blur event is dispatched" things.

> Source/WebCore/dom/Node.h:553
> +    virtual void beforeBlurEvent();

I think a more common convention is to use the "will" prefix, as in "willBlur".

[Kent Tamura]

Created attachment 92011 [details]
Patch 2

Renamed to willBlur().

[Kent Tamura]

(In reply to comment #4)
> We really should take a deeper look at the whole blur/focus event firing lifecycle. I noticed that dispatchBlurEvent is actually performing some of the "do stuff before blur event is dispatched" things.

dispatchBlurEvent() doesn't do anything other than calling ChromeClient::elementDidFocus().  So it is not helpful in this case.
Document::setFocusedNode() do a lot of things.  It calls setActive() and setFocus() and we can override them.  However we need a hook point before clearing m_focusedNode.  So I think introducing willBlur() is reasonable.

[Joseph Pecoraro]

Comment on attachment 92011 [details]
Patch 2

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

> Source/WebCore/html/InputType.h:177
> +    virtual void willBlur();
>      virtual void handleBlurEvent();

It doesn't look like InputType::handleBlurEvent is needed anymore. NumberInputType had the
only virtual implementation of it, and InputType's impl is blank. Should that be removed or
do you envision it being needed in the future?

[Kent Tamura]

Comment on attachment 92011 [details]
Patch 2

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

>> Source/WebCore/html/InputType.h:177
>>      virtual void handleBlurEvent();
> 
> It doesn't look like InputType::handleBlurEvent is needed anymore. NumberInputType had the
> only virtual implementation of it, and InputType's impl is blank. Should that be removed or
> do you envision it being needed in the future?

Good catch!

I'll remove InputType::handleBlurEvent().

[Kent Tamura]

Created attachment 94889 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 94889 [details]
Patch for landing

Clearing flags on attachment: 94889

Committed r87371: <http://trac.webkit.org/changeset/87371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.