Bug 59071 - <input type="number"> dispatches two blurs when tabbing from an invalid number
Summary: <input type="number"> dispatches two blurs when tabbing from an invalid number
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 69395
  Show dependency treegraph
 
Reported: 2011-04-20 21:11 PDT by Joseph Pecoraro
Modified: 2011-10-04 20:34 PDT (History)
3 users (show)

See Also:


Attachments
[TEST] Reduced Test Case (124 bytes, text/html)
2011-04-20 21:11 PDT, Joseph Pecoraro
no flags Details
Patch (10.89 KB, patch)
2011-05-02 09:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (10.72 KB, patch)
2011-05-02 17:11 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (10.96 KB, patch)
2011-05-25 17:41 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-04-20 21:11:38 PDT
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"
Comment 1 Kent Tamura 2011-05-01 22:09:41 PDT
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)
Comment 2 Kent Tamura 2011-05-01 23:58:45 PDT
(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.
Comment 3 Kent Tamura 2011-05-02 09:26:50 PDT
Created attachment 91927 [details]
Patch
Comment 4 Dimitri Glazkov (Google) 2011-05-02 09:41:44 PDT
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".
Comment 5 Kent Tamura 2011-05-02 17:11:30 PDT
Created attachment 92011 [details]
Patch 2

Renamed to willBlur().
Comment 6 Kent Tamura 2011-05-24 07:17:21 PDT
(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.
Comment 7 Joseph Pecoraro 2011-05-25 15:40:38 PDT
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?
Comment 8 Kent Tamura 2011-05-25 17:40:14 PDT
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().
Comment 9 Kent Tamura 2011-05-25 17:41:04 PDT
Created attachment 94889 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2011-05-26 02:11:05 PDT
Comment on attachment 94889 [details]
Patch for landing

Clearing flags on attachment: 94889

Committed r87371: <http://trac.webkit.org/changeset/87371>
Comment 11 WebKit Commit Bot 2011-05-26 02:11:10 PDT
All reviewed patches have been landed.  Closing bug.