Bug 68513 – <input> with autofocus doesn't lose focus when it has a certain onblur listener

• Component: Forms
• Status: RESOLVED FIXED

[Rakesh]

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.

[Rakesh]

Created attachment 108125 [details]
Simplified test page.

[Rakesh]

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.

[Kent Tamura]

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

[Rakesh]

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.

[Kent Tamura]

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

[Rakesh]

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

[Kent Tamura]

Comment on attachment 108129 [details]
Updated patch

The patch contains no tests.

[Ryosuke Niwa]

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.

[Rakesh]

(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()?

[Ryosuke Niwa]

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.

[Rakesh]

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

[Kent Tamura]

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

[Rakesh]

Created attachment 108453 [details]
Updated patch

Added layout tests.

[Kent Tamura]

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.

[Rakesh]

Created attachment 108463 [details]
Updated patch

Changes as per comment#12.

[Rakesh]

(In reply to comment #15)
> Created an attachment (id=108463) [details]
> Updated patch
> 
> Changes as per comment#12.
Its "Changes as per comment#14".

[WebKit Review Bot]

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

[Kent Tamura]

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

[Kent Tamura]

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

[Rakesh]

(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?

[Rakesh]

Created attachment 108637 [details]
Updated patch

Changes as per comment# 18 and 19.

[WebKit Review Bot]

Comment on attachment 108637 [details]
Updated patch

Rejecting attachment 108637 [details] from review queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 108637 [details]
Updated patch

Rejecting attachment 108637 [details] from commit-queue.

[e-mail] 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.

[Rakesh]

Sorry about "review  and commit +" by myself, it got wrongly selected.

[Kent Tamura]

(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?

[Kent Tamura]

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.

[Rakesh]

(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?

[Rakesh]

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

[Rakesh]

Created attachment 108791 [details]
Updated patch.

[Kent Tamura]

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

[Kent Tamura]

Comment on attachment 108791 [details]
Updated patch.

ok.
Thank you for fixing this.

[Kent Tamura]

(In reply to comment #24)
> Sorry about "review  and commit +" by myself, it got wrongly selected.

"webkit-patch upload" command is helpful.

[WebKit Review Bot]

Comment on attachment 108791 [details]
Updated patch.

Clearing flags on attachment: 108791

Committed r96078: <http://trac.webkit.org/changeset/96078>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rakesh]

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

[Rakesh]

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