|Summary:||WF2 Support for autofocus controls|
|Product:||WebKit||Reporter:||Michelangelo De Simone <michelangelo>|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
|Bug Blocks:||19264, 31031, 31032|
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().