Bug 24092 - Autofocus readonly inputs
Summary: Autofocus readonly inputs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://lachy.id.au/dev/markup/tests/h...
Keywords: HasReduction
Depends on:
Blocks: HTML5Forms
  Show dependency treegraph
 
Reported: 2009-02-23 06:29 PST by Anthony Ricaud
Modified: 2011-09-27 11:16 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2011-09-19 05:24 PDT, Kaustubh Atrawalkar
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
test (189 bytes, text/html)
2011-09-21 11:23 PDT, Ryosuke Niwa
no flags Details
Updated Patch (3.16 KB, patch)
2011-09-22 00:09 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (3.12 KB, patch)
2011-09-22 00:17 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated patch (3.12 KB, patch)
2011-09-26 00:05 PDT, Kaustubh Atrawalkar
rniwa: review+
Details | Formatted Diff | Diff
Updated Patch (3.13 KB, patch)
2011-09-27 00:24 PDT, Kaustubh Atrawalkar
rniwa: review-
Details | Formatted Diff | Diff
Patch (3.10 KB, patch)
2011-09-27 00:51 PDT, Kaustubh Atrawalkar
rniwa: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (3.11 KB, patch)
2011-09-27 01:51 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anthony Ricaud 2009-02-23 06:29:39 PST
This test by Lachlan Hunt fails. readonly inputs can't be autofocused.
Comment 1 Kaustubh Atrawalkar 2011-09-19 05:24:00 PDT
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
Comment 2 Kaustubh Atrawalkar 2011-09-19 09:55:22 PDT
Ryoduke can you review patch once? Thanks.
Comment 3 Ryosuke Niwa 2011-09-19 09:58:06 PDT
Comment on attachment 107836 [details]
Patch

What do other browsers do?  What does spec say?
Comment 4 Alexey Proskuryakov 2011-09-19 10:00:21 PDT
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?
Comment 5 Ryosuke Niwa 2011-09-19 10:03:49 PDT
The check was introduced http://trac.webkit.org/changeset/34626, the original implementation of autofocus attribute.
Comment 6 Alexey Proskuryakov 2011-09-19 10:06:15 PDT
"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.
Comment 7 Kaustubh Atrawalkar 2011-09-19 23:37:26 PDT
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.
Comment 8 Ryosuke Niwa 2011-09-20 08:33:20 PDT
(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.
Comment 9 Kaustubh Atrawalkar 2011-09-20 23:40:10 PDT
> 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?
Comment 10 Ryosuke Niwa 2011-09-21 11:23:43 PDT
Created attachment 108191 [details]
test
Comment 11 Ryosuke Niwa 2011-09-21 11:36:45 PDT
With my test, IE9 doesn't autofocus while FF6 does.
Comment 12 Kaustubh Atrawalkar 2011-09-21 23:18:51 PDT
(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?
Comment 13 Ryosuke Niwa 2011-09-21 23:38:36 PDT
It seems reasonable to fix this bug. Please update your patch.
Comment 14 Ryosuke Niwa 2011-09-21 23:43:18 PDT
One more thing. Are there websites affected by this bug?
Comment 15 Alexey Proskuryakov 2011-09-22 00:01:02 PDT
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.
Comment 16 Kaustubh Atrawalkar 2011-09-22 00:09:59 PDT
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.
Comment 17 Kaustubh Atrawalkar 2011-09-22 00:13:06 PDT
(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.
Comment 18 Kaustubh Atrawalkar 2011-09-22 00:17:22 PDT
Created attachment 108287 [details]
Updated Patch

Sorry for the wrong patch. Something messed up in svn up.
Comment 19 Ojan Vafai 2011-09-22 19:00:50 PDT
(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();
}
Comment 20 Ian 'Hixie' Hickson 2011-09-22 19:50:39 PDT
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.
Comment 21 Kaustubh Atrawalkar 2011-09-22 21:19:23 PDT
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.
Comment 23 Kaustubh Atrawalkar 2011-09-25 23:56:07 PDT
Should we land this under regression may be?
Comment 24 Kaustubh Atrawalkar 2011-09-26 00:05:35 PDT
Created attachment 108629 [details]
Updated patch

Adding updated patch.
Comment 25 Ryosuke Niwa 2011-09-26 12:19:22 PDT
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 26 Ryosuke Niwa 2011-09-26 12:20:05 PDT
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?
Comment 27 Ojan Vafai 2011-09-26 14:22:26 PDT
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.
Comment 28 Ryosuke Niwa 2011-09-26 14:25:17 PDT
(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.
Comment 29 Kaustubh Atrawalkar 2011-09-27 00:24:45 PDT
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 30 Ryosuke Niwa 2011-09-27 00:34:20 PDT
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".
Comment 31 Kaustubh Atrawalkar 2011-09-27 00:51:08 PDT
Created attachment 108804 [details]
Patch

Reformatted layout test case as per comments.
Comment 32 WebKit Review Bot 2011-09-27 00:58:01 PDT
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
Comment 33 Ryosuke Niwa 2011-09-27 01:42:49 PDT
It appears that something is wrong with your patch.
Comment 34 Ryosuke Niwa 2011-09-27 01:45:51 PDT
It appears that your patch is conflicting with http://trac.webkit.org/changeset/96078. I'll probably land it manually tomorrow morning.
Comment 35 Kaustubh Atrawalkar 2011-09-27 01:51:24 PDT
(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.
Comment 36 Kaustubh Atrawalkar 2011-09-27 01:51:52 PDT
Created attachment 108812 [details]
Updated Patch
Comment 37 Kaustubh Atrawalkar 2011-09-27 10:41:42 PDT
(In reply to comment #36)
> Created an attachment (id=108812) [details]
> Updated Patch

Thanks Darin, seems i forgot to add it as patch :(
Comment 38 WebKit Review Bot 2011-09-27 11:16:38 PDT
Comment on attachment 108812 [details]
Updated Patch

Clearing flags on attachment: 108812

Committed r96134: <http://trac.webkit.org/changeset/96134>
Comment 39 WebKit Review Bot 2011-09-27 11:16:45 PDT
All reviewed patches have been landed.  Closing bug.