Bug 68513 - <input> with autofocus doesn't lose focus when it has a certain onblur listener
Summary: <input> with autofocus doesn't lose focus when it has a certain onblur listener
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL: http://jsfiddle.net/JDan3/2/show/
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-21 02:08 PDT by Rakesh
Modified: 2011-09-27 00:51 PDT (History)
6 users (show)

See Also:


Attachments
Simplified test page. (538 bytes, text/html)
2011-09-21 02:09 PDT, Rakesh
no flags Details
Proposed Patch (2.88 KB, patch)
2011-09-21 03:04 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (1.12 KB, patch)
2011-09-21 04:02 PDT, Rakesh
tkent: review-
Details | Formatted Diff | Diff
Updated patch (3.79 KB, patch)
2011-09-23 02:55 PDT, Rakesh
tkent: review-
Details | Formatted Diff | Diff
Updated patch (3.95 KB, patch)
2011-09-23 05:24 PDT, Rakesh
tkent: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (5.31 KB, patch)
2011-09-26 00:48 PDT, Rakesh
tkent: review-
Details | Formatted Diff | Diff
Updated patch. (5.26 KB, patch)
2011-09-26 23:05 PDT, Rakesh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rakesh 2011-09-21 02:08:07 PDT
Steps to reproduce:
1. Select the text field labeled as "Input 1 (autofocus)".
2. Type some characters.
3. Try to move focus to other controls

Expected result:
"Input 1" loses focus.

Chromium issue: http://code.google.com/p/chromium/issues/detail?id=97015.
Comment 1 Rakesh 2011-09-21 02:09:01 PDT
Created attachment 108125 [details]
Simplified test page.
Comment 2 Rakesh 2011-09-21 03:04:20 PDT
Created attachment 108127 [details]
Proposed Patch 

The script in this page tries to change the "type" of an input element which has an "autofocus" attribute set on onblur and onFocus events.
With change in "type" webcore detaches the node and adds the node again with modified type.
Issue:
The problem is we try to focus the Node if an "autofocus" attribute is set while attaching the node which causes the node to focus again.

Approach used in the patch:
We should autofocus an element only once after the page load. 
"The autofocus content attribute allows the author to indicate that a control is to be focused as soon as the page is loaded" from http://dev.w3.org/html5/spec/association-of-controls-and-forms.html#autofocusing-a-form-control

I am using an autoFocused flag in Document which will set once the node has been focused for very first time.

Please let me know your comments or better approach to handle this.
Comment 3 Kent Tamura 2011-09-21 03:17:26 PDT
> I am using an autoFocused flag in Document which will set once the node has been focused for very first time.

We already have m_ignoreAutofocus flag.  I think we can use it for this purpose too.
Comment 4 Rakesh 2011-09-21 04:02:41 PDT
Created attachment 108129 [details]
Updated patch


Yes, m_ignoreAutofocus can be used. This flag is not being set from any place presently.

Attached an updated patch.
Comment 5 Kent Tamura 2011-09-21 05:09:26 PDT
(In reply to comment #4)
> Created an attachment (id=108129) [details]
> Updated patch

Do you want it to be reviewed? It doesn't have the "review?" flag.
Comment 6 Rakesh 2011-09-21 05:24:48 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=108129) [details] [details]
> > Updated patch
> 
> Do you want it to be reviewed? It doesn't have the "review?" flag.
sorry, missed setting that.
Comment 7 Kent Tamura 2011-09-21 06:30:52 PDT
Comment on attachment 108129 [details]
Updated patch

The patch contains no tests.
Comment 8 Ryosuke Niwa 2011-09-21 08:38:49 PDT
Comment on attachment 108129 [details]
Updated patch

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

> Source/WebCore/ChangeLog:9
> +        * html/HTMLFormControlElement.cpp:
> +        (WebCore::HTMLFormControlElement::attach):

Please explain why we should be modifying this function. Also, my gut tells me calling setIgnoreAutofocus inside attach is wrong.
Comment 9 Rakesh 2011-09-22 02:11:20 PDT
(In reply to comment #8)
> (From update of attachment 108129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108129&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        * html/HTMLFormControlElement.cpp:
> > +        (WebCore::HTMLFormControlElement::attach):
> 
> Please explain why we should be modifying this function. Also, my gut tells me calling setIgnoreAutofocus inside attach is wrong.

As Autofocus is happening from attach and we have to make sure only this element needs to be focused.

We can as well call setIgnoreAutofocus from focusPostAttach callback after setting the focus but I am not sure if we need to check for ignoreAutoFocus before setting the focus. 

Do you see any side effects of doing that from attach()?
Comment 10 Ryosuke Niwa 2011-09-22 08:53:46 PDT
Comment on attachment 108129 [details]
Updated patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        (WebCore::HTMLFormControlElement::attach):
>> 
>> Please explain why we should be modifying this function. Also, my gut tells me calling setIgnoreAutofocus inside attach is wrong.
> 
> As Autofocus is happening from attach and we have to make sure only this element needs to be focused.
> 
> We can as well call setIgnoreAutofocus from focusPostAttach callback after setting the focus but I am not sure if we need to check for ignoreAutoFocus before setting the focus. 
> 
> Do you see any side effects of doing that from attach()?

If anything, I'd expect the fix for this bug to be in HTMLInputElement::updateType.
Comment 11 Rakesh 2011-09-23 00:34:39 PDT
(In reply to comment #10)
> (From update of attachment 108129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108129&action=review
> 
> >>> Source/WebCore/ChangeLog:9
> >>> +        (WebCore::HTMLFormControlElement::attach):
> >> 
> >> Please explain why we should be modifying this function. Also, my gut tells me calling setIgnoreAutofocus inside attach is wrong.
> > 
> > As Autofocus is happening from attach and we have to make sure only this element needs to be focused.
> > 
> > We can as well call setIgnoreAutofocus from focusPostAttach callback after setting the focus but I am not sure if we need to check for ignoreAutoFocus before setting the focus. 
> > 
> > Do you see any side effects of doing that from attach()?
> 
> If anything, I'd expect the fix for this bug to be in HTMLInputElement::updateType.

As per my understanding of the specification "The auto-focus content attribute allows the author to indicate that a control is to be focused as soon as the page is loaded" implies "an element can be auto-focused only once". So it seems correct to handle it when we are auto-focusing for first time.
Comment 12 Kent Tamura 2011-09-23 02:28:01 PDT
(In reply to comment #11)
I think the code change in the last patch is reasonable.  This problem can happen with <select> with/without multiple, and setting setIgnoreAutofocus() would resolve all of the cases.
Comment 13 Rakesh 2011-09-23 02:55:57 PDT
Created attachment 108453 [details]
Updated patch

Added layout tests.
Comment 14 Kent Tamura 2011-09-23 03:10:59 PDT
Comment on attachment 108453 [details]
Updated patch

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

r- because of test style.

> LayoutTests/fast/forms/autofocus-focus-only-once.html:6
> + input { background:red }
> + input:focus { background:lime }

One-space indentation looks strange.
Please remove the indentation.

> LayoutTests/fast/forms/autofocus-focus-only-once.html:12
> +<script language="JavaScript" type="text/javascript">
> +    function log(message) {
> +        document.getElementById("console").innerHTML += "<li>"+message+"</li>";
> +    }
> +

Indenting the all of the content by four spaces is meaningless.  Please remove the indentation.

> LayoutTests/fast/forms/autofocus-focus-only-once.html:30
> +        if (document.activeElement == document.getElementById("input2"))
> +            log("SUCCESS");
> +        else
> +            log("FAILURE");

You had better load LayoutTests/fast/js/resource/js-test-pre.js, and write shouldBe('document.activeElement', 'document.getElementById("input2")').
See LayoutTests/fast/forms/autofocus-keygen.html, etc.
Comment 15 Rakesh 2011-09-23 05:24:45 PDT
Created attachment 108463 [details]
Updated patch

Changes as per comment#12.
Comment 16 Rakesh 2011-09-23 05:26:36 PDT
(In reply to comment #15)
> Created an attachment (id=108463) [details]
> Updated patch
> 
> Changes as per comment#12.
Its "Changes as per comment#14".
Comment 17 WebKit Review Bot 2011-09-23 07:50:09 PDT
Comment on attachment 108463 [details]
Updated patch

Attachment 108463 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9813254

New failing tests:
fast/forms/autofocus-opera-006.html
Comment 18 Kent Tamura 2011-09-25 19:01:33 PDT
(In reply to comment #17)
> New failing tests:
> fast/forms/autofocus-opera-006.html

Oh, we expect the last element with autofucus gets focused.

So, we can't use Document::m_ignoreAutofocus flag.
We had better introduce another flag to HTMLFormControlElement.
Comment 19 Kent Tamura 2011-09-25 19:05:05 PDT
Comment on attachment 108463 [details]
Updated patch

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

r- because of fast/forms/autofocus-opera-006.html failed.

> LayoutTests/fast/forms/autofocus-focus-only-once-expected.txt:11
> +TEST COMPLETE
> +PASS document.activeElement is document.getElementById("input2")
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

Showing "TEST COMPLETE" twice is wrong.
If you'd like to use finishJSTest(), you need to call "jsTestIsAsync = true" before loading js-test-post.js.
Otherwise, I think you can move the content of test() to a place between </pre> and <script> for js-test-post.js, and remove finishJSTest().
Comment 20 Rakesh 2011-09-26 00:45:36 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > New failing tests:
> > fast/forms/autofocus-opera-006.html
> 
> Oh, we expect the last element with autofucus gets focused.
> 
> So, we can't use Document::m_ignoreAutofocus flag.
> We had better introduce another flag to HTMLFormControlElement.

Will upload new patch with these changes.

I am still thinking about the issue where the element with autofocus
attribute which is added may be on some event after the page load should
not be focused. Do you think this can be a issue?
Comment 21 Rakesh 2011-09-26 00:48:02 PDT
Created attachment 108637 [details]
Updated patch

Changes as per comment# 18 and 19.
Comment 22 WebKit Review Bot 2011-09-26 00:48:37 PDT
Comment on attachment 108637 [details]
Updated patch

Rejecting attachment 108637 [details] from review queue.

rakesh.kn@motorola.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 23 WebKit Review Bot 2011-09-26 00:49:20 PDT
Comment on attachment 108637 [details]
Updated patch

Rejecting attachment 108637 [details] from commit-queue.

rakesh.kn@motorola.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 24 Rakesh 2011-09-26 00:53:06 PDT
Sorry about "review  and commit +" by myself, it got wrongly selected.
Comment 25 Kent Tamura 2011-09-26 18:58:00 PDT
(In reply to comment #20)
> I am still thinking about the issue where the element with autofocus
> attribute which is added may be on some event after the page load should
> not be focused. Do you think this can be a issue?

What type of events?
Comment 26 Kent Tamura 2011-09-26 19:07:52 PDT
Comment on attachment 108637 [details]
Updated patch

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

> LayoutTests/fast/forms/autofocus-focus-only-once.html:22
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();
> +

These lines are not needed. js-test-pre.js does it.

> Source/WebCore/html/HTMLFormControlElement.h:104
> +    bool hasAutofocused() { return m_hasAutoFocused; }
> +    void setAutofocused(bool autofocused = true) { m_hasAutoFocused = autofocused; }
> +

Inconsistent capitalization: hasAuto*f*ocused vs. m_hasAuto*F*ocused
  Let's follow the capitalization of Document::m_ignoreAutofocus.  So it should be m_hasAuto*f*ocused.

Also, we don't need to use the default argument for setAutofocused().
  void setAutofocused() { m_hasAutofocused = true; }
is enough.
Comment 27 Rakesh 2011-09-26 22:50:40 PDT
(In reply to comment #25)
> (In reply to comment #20)
> > I am still thinking about the issue where the element with autofocus
> > attribute which is added may be on some event after the page load should
> > not be focused. Do you think this can be a issue?
> 
> What type of events?

Say on button press, if an element with autofocus attribute is created then, should the new element be focused after creation?
Comment 28 Rakesh 2011-09-26 22:53:26 PDT
(In reply to comment #26)
> (From update of attachment 108637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108637&action=review
> 
> > LayoutTests/fast/forms/autofocus-focus-only-once.html:22
> > +    if (window.layoutTestController)
> > +        layoutTestController.dumpAsText();
> > +
> 
> These lines are not needed. js-test-pre.js does it.
Done.

> 
> > Source/WebCore/html/HTMLFormControlElement.h:104
> > +    bool hasAutofocused() { return m_hasAutoFocused; }
> > +    void setAutofocused(bool autofocused = true) { m_hasAutoFocused = autofocused; }
> > +
> 
> Inconsistent capitalization: hasAuto*f*ocused vs. m_hasAuto*F*ocused
>   Let's follow the capitalization of Document::m_ignoreAutofocus.  So it should be m_hasAuto*f*ocused.
> 
Done, thanks for pointing that out.

> Also, we don't need to use the default argument for setAutofocused().
>   void setAutofocused() { m_hasAutofocused = true; }
> is enough.

Yes, that looks much better. Will upload new patch with changes shortly.
Comment 29 Rakesh 2011-09-26 23:05:52 PDT
Created attachment 108791 [details]
Updated patch.
Comment 30 Kent Tamura 2011-09-26 23:07:39 PDT
(In reply to comment #27)
> Say on button press, if an element with autofocus attribute is created then, should the new element be focused after creation?

I see. It might be reasonable to cancel autofocus by button press.
It would be a part of the step 8 of the specification.

http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#attr-fe-autofocus
> 8. If the user has indicated (for example, by starting to type in a form control) that he does not wish focus to be changed, then optionally abort these steps.

We had better discuss it in a separated bug.
Comment 31 Kent Tamura 2011-09-26 23:53:19 PDT
Comment on attachment 108791 [details]
Updated patch.

ok.
Thank you for fixing this.
Comment 32 Kent Tamura 2011-09-26 23:54:06 PDT
(In reply to comment #24)
> Sorry about "review  and commit +" by myself, it got wrongly selected.

"webkit-patch upload" command is helpful.
Comment 33 WebKit Review Bot 2011-09-27 00:33:22 PDT
Comment on attachment 108791 [details]
Updated patch.

Clearing flags on attachment: 108791

Committed r96078: <http://trac.webkit.org/changeset/96078>
Comment 34 WebKit Review Bot 2011-09-27 00:33:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Rakesh 2011-09-27 00:43:49 PDT
(In reply to comment #30)
> (In reply to comment #27)
> > Say on button press, if an element with autofocus attribute is created then, should the new element be focused after creation?
> 
> I see. It might be reasonable to cancel autofocus by button press.
> It would be a part of the step 8 of the specification.
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#attr-fe-autofocus
> > 8. If the user has indicated (for example, by starting to type in a form control) that he does not wish focus to be changed, then optionally abort these steps.
> 
> We had better discuss it in a separated bug.

Added https://bugs.webkit.org/show_bug.cgi?id=68876.
Comment 36 Rakesh 2011-09-27 00:51:15 PDT
(In reply to comment #32)
> (In reply to comment #24)
> > Sorry about "review  and commit +" by myself, it got wrongly selected.
> 
> "webkit-patch upload" command is helpful.

Thanks for reviewing and for the inputs.