Summary: | <input> with autofocus doesn't lose focus when it has a certain onblur listener | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rakesh <rakeshchaitan> | ||||||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Major | CC: | adele, darin, dglazkov, rniwa, tkent, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
URL: | http://jsfiddle.net/JDan3/2/show/ | ||||||||||||||||||
Attachments: |
|
Description
Rakesh
2011-09-21 02:08:07 PDT
Created attachment 108125 [details]
Simplified test page.
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. > 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.
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.
(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. (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 on attachment 108129 [details]
Updated patch
The patch contains no tests.
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. (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 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. (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. (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. Created attachment 108453 [details]
Updated patch
Added layout tests.
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. Created attachment 108463 [details] Updated patch Changes as per comment#12. (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 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 (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 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(). (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? Created attachment 108637 [details] Updated patch Changes as per comment# 18 and 19. 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 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. Sorry about "review and commit +" by myself, it got wrongly selected. (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 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. (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? (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. Created attachment 108791 [details]
Updated patch.
(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 on attachment 108791 [details]
Updated patch.
ok.
Thank you for fixing this.
(In reply to comment #24) > Sorry about "review and commit +" by myself, it got wrongly selected. "webkit-patch upload" command is helpful. Comment on attachment 108791 [details] Updated patch. Clearing flags on attachment: 108791 Committed r96078: <http://trac.webkit.org/changeset/96078> All reviewed patches have been landed. Closing bug. (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. (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. |