Bug 24092

Created attachment 107836 [details] Patch Removed the check for readonly in shouldAutoFocus(). As per specs autofocus should be ignored only for hidden & output controls. http://www.whatwg.org/specs/web-forms/current-work/#the-autofocus

Ryoduke can you review patch once? Thanks.

Comment on attachment 107836 [details] Patch What do other browsers do? What does spec say?

Comment on attachment 107836 [details] Patch r-, since this change doesn't have a rationale. Please feel free to request review on the same patch once you add a rationale (do both IE and Firefox agree? what does HTML5 say?) This code looks deliberate - did you check when and why it was added to WebKit?

The check was introduced http://trac.webkit.org/changeset/34626, the original implementation of autofocus attribute.

"what does HTML5 say?" Apparently, many reviewers have a difficulty reading the whole bug (one of my patches was rejected for no rationale last week even though it was right there in bug description). Slightly more of it should be in bug title and/or in ChangeLog to make the process more smooth. We strive for spec compliance, which can be achieved by changing WebKit, or by changing specs. It's important to investigate the reasons for existing behavior, and compatibility story.

I completely agree with both of you. We should be adding rationale in the bug description and make it somehow mandatory. This will ease for the committers and reviewers both to analyze the behavior and may be the cause of existence of the bug easily. For the current bug here is the rationale - MSIE 9 - Fail Firefox 6 - Fail Opera 11 - Fail Google Chrome - Fail But as per specs, it does not mention to ignore autofocus attribute for readonly element. One use case may be user want to always focus on the text in the readonly input to allow user to copy it. (In reply to comment #6) > "what does HTML5 say?" > > Apparently, many reviewers have a difficulty reading the whole bug (one of my patches was rejected for no rationale last week even though it was right there in bug description). Slightly more of it should be in bug title and/or in ChangeLog to make the process more smooth. > > We strive for spec compliance, which can be achieved by changing WebKit, or by changing specs. It's important to investigate the reasons for existing behavior, and compatibility story.

(In reply to comment #7) > I completely agree with both of you. We should be adding rationale in the bug description and make it somehow mandatory. This will ease for the committers and reviewers both to analyze the behavior and may be the cause of existence of the bug easily. > > For the current bug here is the rationale - > > MSIE 9 - Fail > Firefox 6 - Fail > Opera 11 - Fail > Google Chrome - Fail So all major browsers do not auto-focus the element? Mn... that isn't what I found. The test case attached on this bug doesn't work property (has a bug) so I created my own and Firefox appeared to have set focus.

> So all major browsers do not auto-focus the element? Mn... that isn't what I found. The test case attached on this bug doesn't work property (has a bug) so I created my own and Firefox appeared to have set focus. Ohh.., I modified the same test case to create Layout test case. I check that, and it fails. Can you upload your own test case?

Created attachment 108191 [details] test

With my test, IE9 doesn't autofocus while FF6 does.

(In reply to comment #11) > With my test, IE9 doesn't autofocus while FF6 does. Ryosuke, your test case does autofocus on Opera 11.51 as well. So readonly inputs does receive autofocus in FF6, Opera. As per specs we should set focus to readonly. What you suggest? Should we fix?

It seems reasonable to fix this bug. Please update your patch.

One more thing. Are there websites affected by this bug?

What will it mean in practice to have a disabled input focused (I see that the test uses :focused, but that doesn't seem very practical)? Will the page be scrolled to make it visible? I'm also curious about disabled input autofocus behavior. But it probably doesn't matter much either way.

Created attachment 108285 [details] Updated Patch I have updated patch and the Layout test case as well to use only one input readonly box as per your test case. That should be enough I guess. Currently I m not aware on any websites which will get affected. But still will keep on looking.

(In reply to comment #15) > What will it mean in practice to have a disabled input focused (I see that the test uses :focused, but that doesn't seem very practical)? Will the page be scrolled to make it visible? > I agree with you that there are not much practical use case for this issue. But, yes the page gets scrolled and shows the auto focused element. This might be to some extent useful to web devs. > I'm also curious about disabled input autofocus behavior. > > But it probably doesn't matter much either way.

Created attachment 108287 [details] Updated Patch Sorry for the wrong patch. Something messed up in svn up.

(In reply to comment #15) > What will it mean in practice to have a disabled input focused (I see that the test uses :focused, but that doesn't seem very practical)? Will the page be scrolled to make it visible? > > I'm also curious about disabled input autofocus behavior. > > But it probably doesn't matter much either way. I agree that focusing a readonly element is a bit weird, but they are currently focusable, we just don't autofocus them. The HTML spec seems to imply that if the element is focusable is should be autofocused, which makes sense to me: http://www.whatwg.org/specs/web-apps/current-work/#attr-fe-autofocus "Queue a task that checks to see if the element is focusable, and if so, runs the focusing steps for that element. User agents may also change the scrolling position of the document, or perform some other action that brings the element to the user's attention." kaustubh, since browsers currently disagree, it's probably worth getting the spec to explicitly say one way or the other. Would you mind sending an email to whatwg@whatwg.org asking for clarification on whether readonly/disabled/hidden inputs should be focusable? I'm inclined to say that they should only be focusable if you explicitly set a tabIndex, but I don't really feel strongly about it. Also, I don't see why form controls are the only things that are autofocusable (at least, that's how shouldAutofocus is currently written). It seems that anything that is focusable should accept autofocus. As the spec is currently written, I'd expect the code to look like this: static bool shouldAutofocus(HTMLFormControlElement* element) { if (!element->autofocus()) return false; if (!element->renderer()) return false; if (element->document()->ignoreAutofocus()) return false; return element->isFocusable(); }

Ojan's interpretation of the spec in comment 19 is correct — there's nothing in the spec that special cases readonlyness as far as focusing goes, whether for regular focusing or for autofocusing.

Ojan, I will surely mail them today itself about the clarification. And as of now Opera and Firefox supports the autofocusing of readonly elements. My initial test case was little buggy and Ryosuke has notified me with his proper test case. According to specs your view about how the shouldHaveFocus functions would be is exactly right. Current patch is to achieve the same. But still we should check with whatwg for clarity.

whatwg discussion: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033279.html

Should we land this under regression may be?

Created attachment 108629 [details] Updated patch Adding updated patch.

Given Ian's response, it seems reasonable to fix this bug. Also, I highly doubt this can cause serious compat. issues since users can always move focus around.

Comment on attachment 108629 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=108629&action=review > LayoutTests/fast/forms/autofocus-readonly-attribute.html:11 > +(document.activeElement == document.getElementById("test")) ? log("PASS"):log("FAIL"); Should we also check document.getElementById("test").focused?

As per my comments at http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033327.html, it's clear to me that the code I wrote in comment 19 is the most spec-compliant code. I'd rather see that patch done, but this patch is fine as is since it's an incremental step in that direction.

(In reply to comment #27) > As per my comments at http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033327.html, it's clear to me that the code I wrote in comment 19 is the most spec-compliant code. Yes. But I think that change can be made in a separate patch since this bug explicitly mentions readonly input element. > I'd rather see that patch done, but this patch is fine as is since it's an incremental step in that direction. Agreed.

Created attachment 108796 [details] Updated Patch Updated patch using onfocus() property to check for the focus instead of activeElement. As focused element is always activeElement but activeElement need not be always focused.

Comment on attachment 108796 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108796&action=review > LayoutTests/fast/forms/autofocus-readonly-attribute.html:9 > +function runTest() > +{ > + if (window.layoutTestController) > + layoutTestController.dumpAsText(); > +} It seems like there's no need for us to wrap this in a function. Please get rid of the function and just run if & dumpAsText(). > LayoutTests/fast/forms/autofocus-readonly-attribute.html:12 > + var ele = document.getElementById("console"); Please don't use the abbreviation "ele".

Created attachment 108804 [details] Patch Reformatted layout test case as per comments.

Comment on attachment 108804 [details] Patch Rejecting attachment 108804 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: LFormControlElement.cpp Hunk #1 FAILED at 127. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/html/HTMLFormControlElement.cpp.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/forms/autofocus-readonly-attribute-expected.txt patching file LayoutTests/fast/forms/autofocus-readonly-attribute.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Ryosuke Niwa', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/9880193

It appears that something is wrong with your patch.

It appears that your patch is conflicting with http://trac.webkit.org/changeset/96078. I'll probably land it manually tomorrow morning.

(In reply to comment #34) > It appears that your patch is conflicting with http://trac.webkit.org/changeset/96078. I'll probably land it manually tomorrow morning. I got that error. Think i needed to updated before sending new patch. Just did that. If possible u can do it now.

Created attachment 108812 [details] Updated Patch

(In reply to comment #36) > Created an attachment (id=108812) [details] > Updated Patch Thanks Darin, seems i forgot to add it as patch :(

Comment on attachment 108812 [details] Updated Patch Clearing flags on attachment: 108812 Committed r96134: <http://trac.webkit.org/changeset/96134>

All reviewed patches have been landed. Closing bug.