Bug 18887

Summary: WF2 Support for autofocus controls
Product: WebKit Reporter: Michelangelo De Simone <michelangelo@webkit.org>
Component: FormsAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Enhancement CC: adele@apple.com, hyatt@apple.com
Priority: P3 Keywords: LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/web-forms-2/#the-autofocus
Bug Depends on:    
Bug Blocks: 19264, 31031, 31032    
Attachments:
Description Flags
Initial implementation
none
Initial implementation, second revision
adele: review-
Initial implementation, third revision
none
New tests imported from Opera autofocus test suite
none
Initial implementation, fourth revision - BAD DIFF FORMATTING, IGNORE
none
Initial implementation, fourth revision
none
Initial implementation, fourth revision (domListEnumeration patch merged) adele: review+

Description From 2008-05-04 13:18:51 PST
WebCore controls should honour the autofocus attribute, whenever applicable according to W3C/WHATWG specs.

When a form control is marked with the autofocus attribute it should be immediatly focused, as soon as the document is loaded. This applies to all controls, except for hidden ones. On document loading the viewport is "moved" accordingly to the last autofocus control.
------- Comment #1 From 2008-05-04 13:23:54 PST -------
Created an attachment (id=20961) [details]
Initial implementation

New DOM attribute brakes testcase fast/dom/domListEnumeration.
------- Comment #2 From 2008-05-05 13:23:07 PST -------
Created an attachment (id=20973) [details]
Initial implementation, second revision

Fixed bad autofocus behavior on readonly/disabled controls. More tests added.
------- Comment #3 From 2008-05-09 09:24:49 PST -------
(From update of attachment 20973 [details])
This is a really good first cut, and I'm glad you posted this patch!

I have  a few things that I'd like you to change.

I think that instead of the code you have in the Document class, you can call focus() directly from the HTMLGenericFormElement class when you parse the attribute.  focus() will automatically handle updating the appearance style at the right time.  You shouldn't need to explicitly call theme()->stateChanged.

If you do all that work in HTMLGenericFormElement, then you shouldn't need the hasAutofocus method.  You can just do that check when you parse the attr.  And you can check document->ignoreAutofocus() at that time too.

I think you're on the right track, and I'm looking forward to seeing what you post next!
------- Comment #4 From 2008-05-09 13:39:38 PST -------
Michelangelo and I talked more about this on IRC, and I see now that when you parse the attribute, it really is too early to set focus.  You don't really know if its a focusable control at that point (even if it does have no renderer, you might not have parsed the disabled attr yet).  I think I'm ok with storing the autofocus node in the document, but I'm not sure waiting for the DOMContentLoaded event is the right time to call focus().

CC'ing Hyatt, so he can weigh in, and add any other comments.
------- Comment #5 From 2008-05-13 14:26:19 PST -------
You should have a change to the autofocus node kick off a timer to call focus() on the autofocus node as soon as possible rather than listening for DOMContentLoaded.  If the autofocus node changes and the timer is already running, you just update the node and don't restart the timer.  The timer callback can then just call focus() on the node.

Then you can remove all that code in the attribute parsing that is calling setChanged and updating the theme state (since focus() in the timer callback will do that for you).
------- Comment #6 From 2008-05-23 07:52:28 PST -------
Created an attachment (id=21310) [details]
Initial implementation, third revision

Third revision. Code refactored, major rationalization. WebCore::Document is now minimally affected. There was no need to set a timer: focus() already does that pretty well.

Now passes all autofocus test suite from Opera:
http://tc.labs.opera.com/html/forms/input/common-attributes/autofocus/
------- Comment #7 From 2008-05-27 13:59:11 PST -------
(From update of attachment 21310 [details])
Great work so far-- you're almost there!  A few comments...

1)The test you added is a good basic test, but it only tests that the attribute is right.  Not that the focus behavior is right.  It would be nice to add versions of those Opera tests too for a more thorough test suite.  It would also be good to have at least one test for "ignoreAutofocus", if that's possible.

2) The ChangeLog should have detailed entries for each change.

3) In HTMLGenericFormElement::attach, you shouldn't need to check for disabled since that is already checked in focus() by calling supportsFocus() and isFocusable().  In fact...should isFocusable() include a check for read-only?
------- Comment #8 From 2008-05-29 07:12:38 PST -------
(In reply to comment #7)

> 1)The test you added is a good basic test, but it only tests that the attribute
> is right.  Not that the focus behavior is right.  It would be nice to add
> versions of those Opera tests too for a more thorough test suite.  It would
> also be good to have at least one test for "ignoreAutofocus", if that's
> possible.

Opera autofocus test suite is being imported; results in platform/mac/fast/forms. 

> 3) In HTMLGenericFormElement::attach, you shouldn't need to check for disabled
> since that is already checked in focus() by calling supportsFocus() and
> isFocusable().  In fact...should isFocusable() include a check for read-only?

It could but having isReadOnlyControl() checked during isFocusable() would brake regression on fast/forms/input-appearance-readonly.html
------- Comment #9 From 2008-05-30 14:46:02 PST -------
Created an attachment (id=21439) [details]
New tests imported from Opera autofocus test suite

Test suite from Opera imported.
------- Comment #10 From 2008-06-16 11:34:26 PST -------
(From update of attachment 21310 [details])
I and Adele discussed about the patch and decided that the approach (seems) correct. No way to use "isFocusable()" because at the the autofocus is fired the that check always fails.

More tests waiting for later review.
------- Comment #11 From 2008-06-16 11:46:48 PST -------
Previous comment was to "buggy", reposting.

I and Adele discussed about the patch and decided that the approach (seems) correct. No way to use "isFocusable()" because at the time autofocus is fired that check always fails.

More tests waiting for later review.
------- Comment #12 From 2008-06-16 16:11:44 PST -------
Created an attachment (id=21746) [details]
Initial implementation, fourth revision

Minor changes: cut m_autofocus (no need for it), HTMLGenericFormElement=>HTMLFormElementControl rename, HTMLAttributeNames format change.

Full test suite merged.
------- Comment #13 From 2008-06-16 16:15:42 PST -------
Created an attachment (id=21747) [details]
Initial implementation, fourth revision
------- Comment #14 From 2008-06-16 16:53:54 PST -------
Created an attachment (id=21748) [details]
Initial implementation, fourth revision (domListEnumeration patch merged)

domListEnumeration patched. Changelog now has logs for each method.
------- Comment #15 From 2008-06-16 17:08:27 PST -------
(From update of attachment 21748 [details])
yay!
------- Comment #16 From 2008-06-16 18:10:13 PST -------
I applied the patch, and ran the layout tests, and I'm actually seeing some failures.  So I'm going to investigate that before checking in
------- Comment #17 From 2008-06-17 11:47:16 PST -------
Committed revision 34626.

I altered two of the opera tests (4 and 5)  to use layoutTestController.waitUntilDone() and layoutTestController.notifyDone().