User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6) Gecko/20100114 Gentoo Firefox/3.5.6 Build Identifier: We should implement autofocus attribute from HTML5 Forms specification. This attribute aims to have an element focused when the page is loaded. This attribute applies to input, button, select, textearea and keygen elements at the moment. For more information, look at $URL Reproducible: Always

There is a check to not autofocus if something is already focused in the document (this is optional in the specs). However, if the user is only reading the document, the page may scroll to the focused element which could be annoying for some users. I think it could be appreciated to add an option in about:config to disable the autofocus feature. I will open a follow-up if you agree.

Comment on attachment 435140 [details] [diff] [review] Patch v0.1 Please change the interface ID for all the interfaces which you're modifying.

Also, you should handle the keygen element.

Thanks for you feedbacks Ehsan. I will fix the IID (I always forget to update them!). For the keygen element, as I've never see any interface for it, I do not even understand why it is available. I will check deeply but at the moment, I can't add the autofocus attribute.

This patch is updating UUID's.

(In reply to comment #6) > Thanks for you feedbacks Ehsan. I will fix the IID (I always forget to update > them!). > For the keygen element, as I've never see any interface for it, I do not even > understand why it is available. I will check deeply but at the moment, I can't > add the autofocus attribute. I _think_ that the parser creates a span content node for keygen elements, though I'm not sure. Maybe Henri knows? Anyway, the HTML5 spec clearly states that autofocus should also work in keygen elements, so this patch at its current status is incomplete.

Gecko has a bogus <keygen> implementation. There isn't a proper element node class or interface for it. Instead, the old parser treats <keygen> as a parser macro that expands to a <select> element with a special marker attribute and generated children. Contrary to the HTML5 spec, the HTML5 parser currently implements a parser macro, too, by cloning the exact keygen behavior of the old parser (including bugs). The plan is to have a real keygen element eventually (bug 101019). Keygen is available, because its functionality is necessary for certificate enrollment workflows (unless the browser implements Microsoft's enrollment API and also VBScript, etc.).

Some interesting test cases: http://lachy.id.au/dev/markup/tests/html5/autofocus/ This patch is passing most of them. The reason of the failures are: - a test is doing element.autofocus=true; and is testing if element.getAttribute('autofocus') is equal to 'autofocus' which is not the case. AFAIK, a bool attribute is true as long as the attribute is defined so... - some tests are assuming where there are two elements with the autofocus attribute, the last one should be focused. The specs say only one element should have the autofocus attribute in a document so that mean, in my understanding, we can do whatever we want in the case there are more than one element. As I decided to not autofocus an element when an element already have the focus in the document for the reasons mentioned in comment 3, tests assuming that are not passing.

By the way, I think we could create a follow-up for the keygen element because I do not think having a working-as-expected keygen element is going to be trivial.

FYI, if you set a boolean IDL attribute to true its corresponding boolean content attribute value is to be set to the attribute name, so that test is correct. Furthermore, that the last element with autofocus specified is to be focused follows from the requirements in the specification as well. You are looking at author conformance requirements but they have no impact on what implementations are required to do.

For the boolean IDL attribute / content attribute relation, can you provide a link ? By the way, it looks like last opera version is now filing the content attribute with 'true'. (AFAIK, those tests come from Opera) For the second point, in my opinion, two sentences are more or less opposite: "There must not be more than one element in the document with the autofocus attribute specified." and "Whenever an element with the autofocus attribute specified is inserted into a document, the user agent should queue a task that checks to see if the element is focusable, and if so, runs the focusing steps for that element." The first one makes me think the UA can behave as it wants when there is more than one autofocus attribute specified and the second let me think the last one should be focused. As I am not experienced enough if W3C specifications I may have misunderstand that. Anne, if you can tell me more about how those sentence should be understood, it would be great.

http://www.whatwg.org/html#reflecting-content-attributes-in-idl-attributes (I guess Opera may have a regression, I didn't check.) For the second point. Of those two statement the first places a requirement on authors. By virtue of it being a requirement on authors it does not apply to implementors and implementors should not try to extract meaning out of it. The second requirement clearly places a requirement on implementors which is what we should follow. E.g. consider that it is forbidden for authors to have <bogus> in their documents. That does not imply <bogus> will not end up in the document if it is included. It in fact implies nothing at all about what implementors have to do.

(mid-air collision) (In reply to comment #13) > For the boolean IDL attribute / content attribute relation, can you provide a > link ? By the way, it looks like last opera version is now filing the content > attribute with 'true'. (AFAIK, those tests come from Opera) "If a reflecting IDL attribute is a boolean attribute, then on getting the IDL attribute must return true if the content attribute is set, and false if it is absent. On setting, the content attribute must be removed if the IDL attribute is set to false, and must be set to have the same value as its name if the IDL attribute is set to true." http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes > For the second point, in my opinion, two sentences are more or less opposite: > "There must not be more than one element in the document with the autofocus > attribute specified." and "Whenever an element with the autofocus attribute > specified is inserted into a document, the user agent should queue a task that > checks to see if the element is focusable, and if so, runs the focusing steps > for that element." > The first one makes me think the UA can behave as it wants when there is more > than one autofocus attribute specified and the second let me think the last one > should be focused. As I am not experienced enough if W3C specifications I may > have misunderstand that. Anne, if you can tell me more about how those sentence > should be understood, it would be great. HTML5 is written differently from some other specs. Errors are always handled in a well-defined manner, because experience has shown that users serve pages with errors no matter what, and browsers have to reverse-engineer each other's error-handling behavior to ensure interoperability in practice if it's not specced. You might want to refer to the section on conformance requirements: "Note: There is no implied relationship between document conformance requirements and implementation conformance requirements. User agents are not free to handle non-conformant documents as they please; the processing model described in this specification applies to implementations regardless of the conformity of the input documents." http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.html#conformance-requirements

I've open bug 556805 about the boolean IDL/content attribute issue. For the other issue, I will provide a new patch fixing that. Thank you Anne and Aryeh for your help !

This patch pass all tests from the source in comment 10 except the one linked to bug 556805. I see two follow-ups: - an option in about:config to disable autofocus - do not autofocus when the user is already doing something (like typing/focus on something else, ...)

I think I'd prefer to not wait for a followup to add the pref. It should be simple to implement a pref if you use nsContentUtils::AddBoolPrefVarCache. As for the second issue. Maybe you could at least check if another element in the document currently has focus and only honor the attribute if that is not the case. Also, this patch only allow forms controls to be autofocused. The spec says that any focusable element should honor it. Finally, would it make sense to only honor the attribute while the document is being loaded? It seems likely to me that people will move around form controls in the DOM and thus accidentally re-insert an element with autofocus set. Though maybe this won't be much of a problem if we implement the idea in the second paragraph above.

Comment on attachment 436747 [details] [diff] [review] Tests v2 Unsetting review request based on previous comment. Btw, is there a reason not to combine the test and code changes into a single patch?

Comment on attachment 436748 [details] [diff] [review] Patch v2 Oh, and feel free to rerequest if you disagree with all my comments :)

(In reply to comment #19) > I think I'd prefer to not wait for a followup to add the pref. It should be > simple to implement a pref if you use nsContentUtils::AddBoolPrefVarCache. Ok. > As for the second issue. Maybe you could at least check if another element in > the document currently has focus and only honor the attribute if that is not > the case. My first patch was doing that but it is conflicting with one part of the spec saying when an element with autofocus is inserted into the document, it has to be focused. If two elements have 'autofocus', the second one should be focused. The only solution I see is to add an information to the focus manager so I can check if the focused element has been focused with autofocus or by user input. I don't think that's a good idea to autofocus an element when another element has been previously autofocused. > Also, this patch only allow forms controls to be autofocused. The spec says > that any focusable element should honor it. $URL is about "Autofocusing a form control" I don't see where it says it should apply to all focusable element. > Finally, would it make sense to only honor the attribute while the document is > being loaded? It seems likely to me that people will move around form controls > in the DOM and thus accidentally re-insert an element with autofocus set. > Though maybe this won't be much of a problem if we implement the idea in the > second paragraph above. I don't think we can prevent that except by preventing autofocus when it sounds undesirable. When an element already has the focus with something else than autofocus sounds the only reasonable way to check that. Maybe two things we could ask to be added to the specs: - only autofocus the first element with the autofocus attribute so we can't have the focus moving through the page with autofocus. For the webpage author, having more than one element with autofocus is forbidden (so that's not changing anything for them) and for the user it may be more convenient ; - do not autofocus after the page is loaded. It sounds out of the use of autofocus and .focus js function should be used instead. What is your opinion ? (In reply to comment #20) > (From update of attachment 436747 [details] [diff] [review]) > Unsetting review request based on previous comment. > > Btw, is there a reason not to combine the test and code changes into a single > patch? No reason. I only do that for readability. I'm writing tests then code but I can attach them in the same patch if you think it's more convenient.

> > As for the second issue. Maybe you could at least check if another element > > in the document currently has focus and only honor the attribute if that is > > not the case. > > My first patch was doing that but it is conflicting with one part of the spec > saying when an element with autofocus is inserted into the document, it has to > be focused. If two elements have 'autofocus', the second one should be > focused. The only solution I see is to add an information to the focus > manager so I can check if the focused element has been focused with autofocus > or by user input. Yeah, that seems like a very bad thing. Plus you still wouldn't catch the case when one form control has been focused using an autofocus attribute and then the user starts typing in that control. While the user is typing another control is inserted in the DOM (say, because we parse more of the page). At this point it would be really annoying if the new form control steals focus in the middle of the user typing. I think the most sensible solution here is to ask the spec to be changed such that the first autofocus'ed element is the one that receives focus. I really can't see any other good solution? Can you email whatwg/htmlwg, or do you want me to? > > Also, this patch only allow forms controls to be autofocused. The spec says > > that any focusable element should honor it. > > $URL is about "Autofocusing a form control" I don't see where it says it > should apply to all focusable element. I'm not sure that the title can be considered as part of the normative requirements. The normative requirements say: "Whenever an element with the autofocus attribute specified is inserted into a document whose browsing context did not have the sandboxed automatic features browsing context flag set when the Document was created, the user agent should queue a task that checks to see if the element is focusable, and if so, runs the focusing steps for that element." So no restrictions there on that this only applies to form controls. However since the autofocus attribute is only specified on form controls I think the intent was that this should only happen for form controls. So I think you're correct here. However it would be great to get this clarified in the spec. For example by saying that this only happens on elements where the autofocus attribute is defined. > > Finally, would it make sense to only honor the attribute while the document > > is being loaded? It seems likely to me that people will move around form > > controls in the DOM and thus accidentally re-insert an element with > > autofocus set. Though maybe this won't be much of a problem if we implement > > the idea in the second paragraph above. > > I don't think we can prevent that except by preventing autofocus when it > sounds undesirable. When an element already has the focus with something else > than autofocus sounds the only reasonable way to check that. > Maybe two things we could ask to be added to the specs: > - only autofocus the first element with the autofocus attribute so we can't > have the focus moving through the page with autofocus. For the webpage author, > having more than one element with autofocus is forbidden (so that's not > changing anything for them) and for the user it may be more convenient ; > - do not autofocus after the page is loaded. It sounds out of the use of > autofocus and .focus js function should be used instead. > > What is your opinion ? Agreed on both accounts :) It's debatable if autofocus should be "disabled" after the 'load' event has fired, or after the 'DOMContentLoaded' event has fired. I'm fine either way. And either is trivial to implement by looking at the documents readyState (nsIDocument::GetReadyStateEnum) > (In reply to comment #20) > > (From update of attachment 436747 [details] [diff] [review] [details]) > > Unsetting review request based on previous comment. > > > > Btw, is there a reason not to combine the test and code changes into a single > > patch? > > No reason. I only do that for readability. I'm writing tests then code but I > can attach them in the same patch if you think it's more convenient. Ah, makes sense. Since they're written separately I'll treat them separately. In fact, I'll go review the tests right away :)

Comment on attachment 436747 [details] [diff] [review] Tests v2 >+++ b/content/html/content/test/test_bug546995-2.html Fri Apr 02 21:52:05 2010 +0200 >+function runTests() >+{ >+ is(gFocusHandler, 1, "Input element should have been focused"); >+ >+ addElementWithAutofocus('button', true); >+ setTimeout("addElementWithAutofocus('select')", 100); >+ setTimeout("addElementWithAutofocus('textarea')", 200); >+ >+ setTimeout("is(gFocusHandler, 4, 'All elements should have been focused')", >+ 300); Unfortunately this is no good. Waiting for X milliseconds to see that the focus event has fired won't work reliably enough. Our mochitest servers are notorious for randomly taking forever to perform a specific action, causing tests like this to randomly go orange. Instead what you should do is to wait until the focus event fires, and only then run the next test. (I've found it really neat to use generators to run asynchronous tests. For an example, see here: http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug544642.html?force=1 Just make sure to set the script type to "application/javascript;version=1.8". Definitely feel free to not use this pattern though.) >+ // If we try to autofocus when an element is already focused, >+ // the inserted element should be focused. >+ setTimeout("addElementWithAutofocus('input')", 400); >+ setTimeout( >+ "is(gFocusHandler, 5, 'The inserted element should have been focused')", >+ 500); If we are able to go through with the spec changes, then this test would need to be changed obviously. >+ // If we try to autofocus an element which does not allow focus, >+ // nothing should happen. >+ setTimeout("addNotFocusableElementWithAutofocus()", 600); >+ setTimeout("is(gFocusHandler, 5, 'No element should have been focused')", 700); Unfortunately this is tricky. Here you obviously can't wait for the focus event to fire, as it shouldn't fire. I can't really think of any better solution than essentially doing what you are doing here. Wait X milliseconds and then check if the event fired. The upshot is that if we start failing this test, we'll correctly fail most of the time, and only occasionally erroneously pass the test. Let me know if that makes sense? :) Minusing based on the timeout for when the focus is expected to fire as that will cause random oranges.

The propositions have been sent to the whatwg mailing list. For the tests and patch, I will update them after getting some return to see how that's moving. I will probably increase the timeout value for the no-focus tests to minimize chances to get an erroneous pass.

Sounds great! Thanks for your work!

HTML WG bugs: http://www.w3.org/Bugs/Public/show_bug.cgi?id=9547 http://www.w3.org/Bugs/Public/show_bug.cgi?id=9548

For what it's worth, it seemed like there was enough agreement on the whatwg list that I think we can go ahead and implement the proposal. This way we also gather implementation experience with our proposed behavior. I don't think we need to wait for an official response from the editor. Worst case we can always change the code if it ultimately gets rejected.

This is not testing the behavior for all available elements because it would need a test file for each element and that will probably too much verbose. If you think every elements should be tested, let me know.

This patch is including the proposed changes to the specifications. I'm using a virtual |AcceptAutofocus| method which has to be re-defined when an element accepts autofocus. That's a personal taste but I've see most similar functions (like |CanBeDisabled|) are not virtual. If you think I should use the |CanBeDisabled| approach, I can.

Comment on attachment 439731 [details] [diff] [review] Tests v3 >diff -r d5a2312e96a0 content/html/content/test/test_bug546995-2.html >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 >+++ b/content/html/content/test/test_bug546995-2.html Sun Apr 18 00:57:05 2010 +0200 >@@ -0,0 +1,68 @@ >+<!DOCTYPE HTML> >+<html> >+<!-- >+https://bugzilla.mozilla.org/show_bug.cgi?id=546995 >+--> >+<head> >+ <title>Test for Bug 546995</title> >+ <script type="application/javascript" src="/MochiKit/packed.js"></script> >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> >+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> >+</head> >+<body onload="window.setTimeout(runTests, 0);"> This is still relying on ordering of various events where the order is essentially undefined. You're basically hoping that by the time that this timeout has run we've fired all the needed focus events for the various elements. Instead make the focus event handler for the element that should receive focus start a timeout that is long enough to test that no other focus events fire. >+function onFocusHandler() >+{ >+ focus4 = true; >+} ... >+ var e = document.createElement('input'); >+ e.autofocus = true; >+ e.onfocus = onFocusHandler; >+ c.appendChild(e); Would probably be easier to follow to simply do: e.onfocus = function() { focus4 = true; }; and get rid of the 'onFocusHandler' function. >+ setTimeout("ok(!focus4, \"After loading, elements can't be autofocused\")", >+ 500); >+ >+ setTimeout(SimpleTest.finish, 600); This is also a bad idea. You're relying on that the first timer fires before the second one. Simply put the call to SimpleTest.finish() into the first timeout. A lot of these comments apply to other tests in this patch too. Hope this makes sense? Feel free to ping me on irc if you have questions btw. I know writing reliable tests in the face of our unreliable mochitest infrastructure is a pain.

Comment on attachment 439733 [details] [diff] [review] Patch v3 >+class nsAutoFocusEvent : public nsRunnable { >+public: >+ nsAutoFocusEvent(nsGenericHTMLFormElement* aElement) : mElement(aElement) {} >+ >+ NS_IMETHOD Run() { >+ nsFocusManager* fm = nsFocusManager::GetFocusManager(); >+ if (!fm) { >+ return NS_ERROR_NULL_POINTER; >+ } >+ >+ if (fm->sAutofocusEnabled) { >+ return NS_OK; >+ } >+ >+ nsIDocument* document = mElement->GetDocument(); >+ >+ // Do not autofocus an element if the document is loaded. >+ if (document->GetReadyStateEnum() == nsIDocument::READYSTATE_COMPLETE) { >+ return NS_OK; >+ } It would probably be better to put the readystate test before we schedule the runnable. As it is, you're running the risk that we'll finish parsing the document between scheduling the runnable and it actually running. >+ // If something is focused in the same document, ignore autofocus. >+ if (!fm->GetFocusedContent() || >+ fm->GetFocusedContent()->GetDocument() != document) { >+ return mElement->Focus(); >+ } Keep this test here though. This is looking good. However GetDocument() is deprecated. Use GetOwnerDoc(). See documentation in nsINode.h. Same thing with the call to mElement->GetDocument() above. >@@ -2397,26 +2439,34 @@ nsGenericHTMLFrameElement::IsHTMLFocusab > *aIsFocusable = isFocusable; > if (!isFocusable && aTabIndex) { > *aTabIndex = -1; > } > > return PR_FALSE; > } > >+ > nsresult Nit: This change seems unnecessary. Looking good otherwise! Minusing since I'd like to see the above things fixed though. Also I would like to check with Neil Deakin if the changes to the focus manager are ok, or if he would prefer to keep that code in nsGenericHTMLElement or some such.

> Also I would like to check with Neil Deakin if the changes to the focus manager > are ok, or if he would prefer to keep that code in nsGenericHTMLElement or some > such. Adding to nsGenericHTMLElement or similar would seem better, although I'm fine with it being in nsFocusManager if there's no suitable initialization point. > + ok(!focus1, "First input element should not have been focused"); > + ok(focus2, "Second input element should be focused"); > + ok(!focus3, "First textarea should not have been focused"); It doesn't seem like 'autofocus' relies on the element actually being frontmost or firing events. After loading, document.activeElement should be set to 'focus2' here I would think, which avoids having to wait for the events. All that needs to happen is to ensure that runTests runs after the autofocus handling. + for (var i=0; i<c.childNodes.length; i++) + { + if (c.childNodes[i] instanceof HTMLElement) { + c.childNodes[i].blur(); + } + } document.activeElement.blur() will accomplish this.

(In reply to comment #33) > > Also I would like to check with Neil Deakin if the changes to the focus manager > > are ok, or if he would prefer to keep that code in nsGenericHTMLElement or some > > such. > > Adding to nsGenericHTMLElement or similar would seem better, although I'm fine > with it being in nsFocusManager if there's no suitable initialization point. Actually, I do not see a good initialization point in nsGenericHTMLElement. We /may/ found one but the initialization of a singleton is probably a good one. In addition, I think if someone is looking for the init point of the parameter, the focus manager will come in mind before nsGenericHTMLElement. > > + ok(!focus1, "First input element should not have been focused"); > > + ok(focus2, "Second input element should be focused"); > > + ok(!focus3, "First textarea should not have been focused"); > > It doesn't seem like 'autofocus' relies on the element actually being frontmost > or firing events. After loading, document.activeElement should be set to > 'focus2' here I would think, which avoids having to wait for the events. All > that needs to happen is to ensure that runTests runs after the autofocus > handling. > > + for (var i=0; i<c.childNodes.length; i++) > + { > + if (c.childNodes[i] instanceof HTMLElement) { > + c.childNodes[i].blur(); > + } > + } > > document.activeElement.blur() will accomplish this. Thanks for this hint ;) I will update tests and patches tomorrow.

Comment on attachment 441309 [details] [diff] [review] Patch v4 Neil, could you review this patch to confirm the change in |nsFocusManager| is okay ?

(In reply to comment #34) > Actually, I do not see a good initialization point in nsGenericHTMLElement. We > /may/ found one but the initialization of a singleton is probably a good one. > In addition, I think if someone is looking for the init point of the parameter, > the focus manager will come in mind before nsGenericHTMLElement. Since it's only applies to html elements and is only used in the html element code it makes more sense to just be a static in nsGenericHTMLElement. Why do you even need to cache the preference value at all? This patch is getting the value at startup, but it would make more sense to cache it only when used, since this wouldn't be a heavily used feature.

I guess I don't care super much if we cache the pref value or not.

Comment on attachment 441308 [details] [diff] [review] Tests v4 >diff -r 6b15adef4222 content/html/content/test/test_bug546995-2.html ... >+/** Test for Bug 546995 **/ >+ >+SimpleTest.waitForExplicitFinish(); >+ >+var gGen = runTests(); >+addLoadEvent(function() { gGen.next(); }); >+ >+var gElement = ""; >+ >+function runTests() >+{ >+ var iframe = document.getElementById("iframe"); >+ var iframeCw = iframe.contentWindow; >+ >+ // TODO: keygen should be added when correctly implemented, see bug 101019. >+ var elements = Array("input", "textarea", "select", "button"); Nit: You could also do: var elements = ["input", "textarea", ... ]; >+ for each(element in elements) { >+ gElement = element; >+ iframeCw.location = "file_bug546995.html"; >+ yield; >+ is(iframe.contentDocument.activeElement, >+ iframe.contentDocument.getElementById('a'), >+ "The first inserted element with autofocus should be focused"); >+ } >+ >+ // Now we want to focus the body element. >+ document.activeElement.blur(); >+ ok(document.activeElement instanceof HTMLBodyElement, "foo"); The "foo" description isn't very helpful ;) You could also do: is (document.activeElement, document.body, ...); >+ // Adding elements with autofocus. >+ for each(element in elements) { >+ var e = document.createElement(element); >+ e.autofocus = true; >+ document.getElementById('content').appendChild(e); >+ } >+ >+ // The element should still be focused. Waiting a second to be sure. >+ setTimeout("ok(document.activeElement instanceof HTMLBodyElement,\ >+ \"After loading, elements can't be autofocused\");\ >+ SimpleTest.finish();", 1000); >+ yield; It'd be more readable to do: setTimeout(function() { ok(document.activeElement instanceof HTMLBodyElement, "After loading, elements can't be autofocused"); SimpleTest.finish(); }, 1000); yield; Or even: setTimeout(function() { gGen.next(); }, 1000); yield; ok(document.activeElement instanceof HTMLBodyElement, "After loading, elements can't be autofocused"); SimpleTest.finish(); yield;

I've removed all the code from |nsFocusManager| and I'm no longer caching the pref value. Thank you for your comments Neil.

Comment on attachment 441793 [details] [diff] [review] Patch v4.1 >+class nsAutoFocusEvent : public nsRunnable { { should be in the next line. >+public: >+ nsAutoFocusEvent(nsGenericHTMLFormElement* aElement) : mElement(aElement) {} >+ >+ NS_IMETHOD Run() { >+ nsFocusManager* fm = nsFocusManager::GetFocusManager(); >+ if (!fm) { >+ return NS_ERROR_NULL_POINTER; >+ } >+ >+ nsIDocument* document = mElement->GetOwnerDoc(); >+ if (!document) { >+ return NS_OK; >+ } >+ >+ // If something is focused in the same document, ignore autofocus. >+ if (!fm->GetFocusedContent() || >+ fm->GetFocusedContent()->GetOwnerDoc() != document) { >+ return mElement->Focus(); >+ } So how does this all work when the page containing autofocus is loaded in a background tab? I believe when bringing that tab to front, focus should be set to the element which has autofocus. >@@ -2367,16 +2403,30 @@ nsGenericHTMLFormElement::BindToTree(nsI > nsIContent* aParent, > nsIContent* aBindingParent, > PRBool aCompileEventHandlers) > { > nsresult rv = nsGenericHTMLElement::BindToTree(aDocument, aParent, > aBindingParent, > aCompileEventHandlers); > NS_ENSURE_SUCCESS(rv, rv); >+ >+ // An autofocus event has to be launched if the autofocus attribute is >+ // specified and the element accept the autofocus attribute. In addition, >+ // the document should not be already loaded and the "browser.autofocus" >+ // preference should be 'true'. >+ if (AcceptAutofocus() && HasAttr(kNameSpaceID_None, nsGkAtoms::autofocus) && >+ aDocument && >+ aDocument->GetReadyStateEnum() != nsIDocument::READYSTATE_COMPLETE && >+ nsContentUtils::GetBoolPref("browser.autofocus", PR_TRUE)) { >+ nsCOMPtr<nsIRunnable> event = new nsAutoFocusEvent(this); >+ rv = NS_DispatchToCurrentThread(event); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ >+++ b/content/html/content/src/nsHTMLButtonElement.cpp Tue Apr 27 15:58:50 2010 +0200 >@@ -106,16 +106,18 @@ public: ... > protected: >+ PRBool AcceptAutofocus() const >+ { >+ return PR_TRUE; >+ } So in nsGenericHTMLElement.cpp this is public and virtual. Same problem also elsewhere. sr-, waiting for answer to the background tab problem.

So, please test how/if autofocus works in background tab. And another thing; how should autofocus work if some subdocument is focused before autofocus runs? I mean, what if the page has <iframe> which has <input> and use focuses that.

(In reply to comment #44) > So, please test how/if autofocus works in background tab. I think we should also have an automated test for that behavior.

(In reply to comment #44) > I mean, what if the page has <iframe> which has <input> and use focuses that. s/use/user/ And yes, automated tests would be great for this all.

I did the change to prevent focusing a parent window when the focus is in a sub-window and the tests are working. However, I just can't write a mochitest to test if autofocus is working correctly when it is used in a backgrounded window. Indeed, I can't focus the caller window after window.open(). The best I can do is to have the caller window back in foreground but the focus is still in the popup. I tried |popupWindow.focus()| and |focusManager.focusedWindow(window)|... is there something I should try ? The only solution I see is to write a browser (or chrome ?) test.

Yes, a Chrome test is needed to test that. See here for how to do it: https://developer.mozilla.org/en/Chrome_tests

I've added the two tests for the focus in the iframe and the browser chrome test for the autofocus in the background.

This is making the new tests pass. Btw, I realized something like that: <input autofocus onfocus="blur();"> /* lot of stuff */ <input autofocus> would probably make the focus moving into the document with autofocus during the page load. I'm not sure we should make this impossible because it would be a lot of work for something not so important but we will have to make sure the specification changes we've requested are not too restrictive.

Tests: r=sicking Patch: r=sicking sr=smaug

r=sicking Updated patch against the HEAD.

r=sicking sr=smaug Updated patch against the HEAD.

I really really don't like these tests waiting for 2 seconds for no good reason. What causes us to have to wait for 2 seconds in these tests?

(In reply to comment #55) > I really really don't like these tests waiting for 2 seconds for no good > reason. What causes us to have to wait for 2 seconds in these tests? Because when @autofocus is found, a focus is added to the event queue so we can't assume the focus event will be done when the page is loaded. It could be a bit later. However, I assume you want me to had a handler for the focus event and just let the test timeout if the focus event is never sent/catch, right ?

Timing out if an event that should fire doesn't fire is quite ok. I thought that was what I asked for a few times. However I thought we were also testing that an asynchronous event did *not* fire. That I don't know how to do other than simply waiting for a bit and then checking that it still hasn't fired.

(In reply to comment #57) > However I thought we were also testing that an asynchronous event did *not* > fire. That I don't know how to do other than simply waiting for a bit and then > checking that it still hasn't fired. This could be done though with a very small timeout (perhaps even 0 or by using executeSoon), and iterating a few times until a few seconds have elapsed and then failing. In the usual case where the test works, this won't cause much delay. You don't need to wait for an event, you could just compare document.activeElement.

(In reply to comment #57) > Timing out if an event that should fire doesn't fire is quite ok. I thought > that was what I asked for a few times. > > However I thought we were also testing that an asynchronous event did *not* > fire. That I don't know how to do other than simply waiting for a bit and then > checking that it still hasn't fired. Indeed. Sorry, it's sometimes hard to get back to some old code. (In reply to comment #58) > (In reply to comment #57) > > However I thought we were also testing that an asynchronous event did *not* > > fire. That I don't know how to do other than simply waiting for a bit and then > > checking that it still hasn't fired. > > This could be done though with a very small timeout (perhaps even 0 or by using > executeSoon), and iterating a few times until a few seconds have elapsed and > then failing. In the usual case where the test works, this won't cause much > delay. You don't need to wait for an event, you could just compare > document.activeElement. Don't we want the exact opposite ? For example, test-4 is checking that the <input> is not focused. If we are iterating to check it is not focused, the usual case will have us do all the iterations. By the way, I think the setTimout of test-5 could be removed and the browser test could be improved. I will open follow-ups for that.

(In reply to comment #59) > Don't we want the exact opposite ? For example, test-4 is checking that the > <input> is not focused. If we are iterating to check it is not focused, the > usual case will have us do all the iterations. Hmm. I guess so then. I didn't see there was special logic when an iframe was focused. Or is that just when anything happens to already be focused?

(In reply to comment #60) > (In reply to comment #59) > > Don't we want the exact opposite ? For example, test-4 is checking that the > > <input> is not focused. If we are iterating to check it is not focused, the > > usual case will have us do all the iterations. > > Hmm. I guess so then. I didn't see there was special logic when an iframe was > focused. Or is that just when anything happens to already be focused? Indeed and the same type of behavior is checked in test-3.

I've open bug 567098 to remove some |setTimeout|.

So I think that for the cases where you need to make sure that an async event has executed, you can do it with one (or in some cases, two) executeSoon calls. And like jst said before, timing out is a valid failure mode for tests, you don't need to write the tests in a way that they always fail explicitly.

As well as background tabs/windows, what about autofocus in sidebar documents?

This is documented on the appropriate elements, and mentioned here: https://developer.mozilla.org/en/HTML/HTML5/Forms_in_HTML5