Bug 18887

Created attachment 20961 [details] Initial implementation New DOM attribute brakes testcase fast/dom/domListEnumeration.

Created attachment 20973 [details] Initial implementation, second revision Fixed bad autofocus behavior on readonly/disabled controls. More tests added.

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!

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.

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

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

(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

Created attachment 21439 [details] New tests imported from Opera autofocus test suite Test suite from Opera imported.

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.

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.

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.

Created attachment 21747 [details] Initial implementation, fourth revision

Created attachment 21748 [details] Initial implementation, fourth revision (domListEnumeration patch merged) domListEnumeration patched. Changelog now has logs for each method.

Comment on attachment 21748 [details] Initial implementation, fourth revision (domListEnumeration patch merged) yay!

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

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