Bug 24092 – Autofocus readonly inputs

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Anthony Ricaud]

This test by Lachlan Hunt fails. readonly inputs can't be autofocused.

[Kaustubh Atrawalkar]

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

[Kaustubh Atrawalkar]

Ryoduke can you review patch once? Thanks.

[Ryosuke Niwa]

Comment on attachment 107836 [details]
Patch

What do other browsers do?  What does spec say?

[Alexey Proskuryakov]

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?

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

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

[Kaustubh Atrawalkar]

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.

[Ryosuke Niwa]

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

[Kaustubh Atrawalkar]

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

[Ryosuke Niwa]

Created attachment 108191 [details]
test

[Ryosuke Niwa]

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

[Kaustubh Atrawalkar]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

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.

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

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

[Kaustubh Atrawalkar]

Created attachment 108287 [details]
Updated Patch

Sorry for the wrong patch. Something messed up in svn up.

[Ojan Vafai]

(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 [e-mail] 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();
}

[Ian 'Hixie' Hickson]

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.

[Kaustubh Atrawalkar]

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.

[Ojan Vafai]

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

[Kaustubh Atrawalkar]

Should we land this under regression may be?

[Kaustubh Atrawalkar]

Created attachment 108629 [details]
Updated patch

Adding updated patch.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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?

[Ojan Vafai]

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.

[Ryosuke Niwa]

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

[Kaustubh Atrawalkar]

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.

[Ryosuke Niwa]

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

[Kaustubh Atrawalkar]

Created attachment 108804 [details]
Patch

Reformatted layout test case as per comments.

[WebKit Review Bot]

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

[Ryosuke Niwa]

It appears that something is wrong with your patch.

[Ryosuke Niwa]

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

[Kaustubh Atrawalkar]

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

[Kaustubh Atrawalkar]

Created attachment 108812 [details]
Updated Patch

[Kaustubh Atrawalkar]

(In reply to comment #36)
> Created an attachment (id=108812) [details]
> Updated Patch

Thanks Darin, seems i forgot to add it as patch :(

[WebKit Review Bot]

Comment on attachment 108812 [details]
Updated Patch

Clearing flags on attachment: 108812

Committed r96134: <http://trac.webkit.org/changeset/96134>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.