Bug 18887 - WF2 Support for autofocus controls
: WF2 Support for autofocus controls
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P3 Enhancement
Assigned To: Nobody
http://www.w3.org/TR/web-forms-2/#the...
: LayoutTestFailure
Depends on:
Blocks: HTML5Forms 31031 31032
  Show dependency treegraph
 
Reported: 2008-05-04 13:18 PDT by Michelangelo De Simone
Modified: 2009-11-02 14:33 PST (History)
2 users (show)

See Also:


Attachments
Initial implementation (15.97 KB, patch)
2008-05-04 13:23 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Initial implementation, second revision (22.60 KB, patch)
2008-05-05 13:23 PDT, Michelangelo De Simone
adele: review-
Details | Formatted Diff | Diff
Initial implementation, third revision (11.42 KB, patch)
2008-05-23 07:52 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
New tests imported from Opera autofocus test suite (14.85 KB, patch)
2008-05-30 14:46 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Initial implementation, fourth revision - BAD DIFF FORMATTING, IGNORE (23.37 KB, patch)
2008-06-16 16:11 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Initial implementation, fourth revision (23.58 KB, patch)
2008-06-16 16:15 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Initial implementation, fourth revision (domListEnumeration patch merged) (25.27 KB, patch)
2008-06-16 16:53 PDT, Michelangelo De Simone
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michelangelo De Simone 2008-05-04 13:18:51 PDT
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 Michelangelo De Simone 2008-05-04 13:23:54 PDT
Created attachment 20961 [details]
Initial implementation

New DOM attribute brakes testcase fast/dom/domListEnumeration.
Comment 2 Michelangelo De Simone 2008-05-05 13:23:07 PDT
Created attachment 20973 [details]
Initial implementation, second revision

Fixed bad autofocus behavior on readonly/disabled controls. More tests added.
Comment 3 Adele Peterson 2008-05-09 09:24:49 PDT
Comment on attachment 20973 [details]
Initial implementation, second revision

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 Adele Peterson 2008-05-09 13:39:38 PDT
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 Dave Hyatt 2008-05-13 14:26:19 PDT
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 Michelangelo De Simone 2008-05-23 07:52:28 PDT
Created attachment 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 Adele Peterson 2008-05-27 13:59:11 PDT
Comment on attachment 21310 [details]
Initial implementation, third revision

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 Michelangelo De Simone 2008-05-29 07:12:38 PDT
(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 Michelangelo De Simone 2008-05-30 14:46:02 PDT
Created attachment 21439 [details]
New tests imported from Opera autofocus test suite

Test suite from Opera imported.
Comment 10 Michelangelo De Simone 2008-06-16 11:34:26 PDT
Comment on attachment 21310 [details]
Initial implementation, third revision

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 Michelangelo De Simone 2008-06-16 11:46:48 PDT
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 Michelangelo De Simone 2008-06-16 16:11:44 PDT
Created attachment 21746 [details]
Initial implementation, fourth revision - BAD DIFF FORMATTING, IGNORE

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

Full test suite merged.
Comment 13 Michelangelo De Simone 2008-06-16 16:15:42 PDT
Created attachment 21747 [details]
Initial implementation, fourth revision
Comment 14 Michelangelo De Simone 2008-06-16 16:53:54 PDT
Created attachment 21748 [details]
Initial implementation, fourth revision (domListEnumeration patch merged)

domListEnumeration patched. Changelog now has logs for each method.
Comment 15 Adele Peterson 2008-06-16 17:08:27 PDT
Comment on attachment 21748 [details]
Initial implementation, fourth revision (domListEnumeration patch merged)

yay!
Comment 16 Adele Peterson 2008-06-16 18:10:13 PDT
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 Adele Peterson 2008-06-17 11:47:16 PDT
Committed revision 34626.

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