Bug 31032

A patch to address this bug as well as 24092 and 24093 is underway. CC'ing Darin, Adele and Anthony to keep them posted.

Created attachment 43227 [details] Proposed patch, v1

Created attachment 43228 [details] Proposed patch, v1

Created attachment 43229 [details] Proposed patch, v1

Created attachment 43230 [details] Proposed patch, v1... again

Comment on attachment 43230 [details] Proposed patch, v1... again There's a tab in that change log. > + * fast/forms/script-tests/autofocus.js: Added. > + (autofocus_element.onfocus): > + (): Please remove text like this. I don't see the point of having "():" in our change log. The test coverage here seems much too light. Setting a JavaScript property on an element and then checking that the value is stored in the object doesn't really test much at all. It would work even if there was no autofocus support in the engine, because you can set and get arbitrary JavaScript properties on any element. Here are things the tests should probably cover, in many cases repeated for each type of element that can be autofocused: 1) Does setting the JavaScript property on the element cause the attribute to be set? Test different values. What do values other than true and false do? Such as null and the string "false". 2) Does setting the attribute on the element cause the JavaScript property to have the correct value? Test unusual values such as "false" and "no". 3) What does autofocus do when nothing is focused? What about when the document is focused? (Are those even really separate states?) What about when an <a> element is focused? 4) What is the timing of autofocus? Is implicitClose the right time? I'd like to see tests that cover the last thing that happens before the focus and the first thing after to make it clear what the timing is. In cases with an explicit calls to close() and in others cases where the close time is determined differently. Especially concentrating on timing relative to the "load" event. 5) Tests at least one case where autofocus is set with markup rather than by setAttribute or setting a JavaScript property. Check to be sure it's case sensitive when appropriate, and not when it should not be. 6) What happens when multiple elements request autofocus? Which one wins? To write the test you need to know what the design is. Which one is supposed to win? Does the order in which the renderers are created effect this? To force renderers to be created before they normally would you can call functions like "offsetLeft" on some element in the document. Those functions force layout at the time they are called. 7) Does having an autofocus in a subframe result in more than one focused element? Can autofocus in a frame take focus away from another frame? Should it, or is that a bug? The name autofocusedFormControlElement is too long. Instead I'd suggest autofocusedControl. It can still have HTMLFormControlElement as its type. But also, I think it's a little strange to call it "autofocused" if it's not autofocused. I think that in plain speech we would say that this is the element that "requests autofocus" or "has the autofocus attribute set", so we probably need some other term than "autofocused", which seems to mean that it's already successfully gotten focus. So maybe controlRequestingAutofocus would be good? Or maybe there's some even better name. > + if (autofocusedFormControlElement()) { > + Node* n = focusedNode(); > + if (!n) > + autofocusedFormControlElement()->focus(); > + else if (n->isElementNode()) { > + Element* e = static_cast<Element*>(n); > + if (!e->isFormControlElement()) > + autofocusedFormControlElement()->focus(); > + } > + } The above could be written more simply and clearly. One of the best ways would be to make a helper function. Here's one that exactly matches your logic: static bool nodePreventsAutofocus(Node* node) { return node && (!node->isElementNode() || static_cast<Element*>(focusedNode)->isFormControlElement()); } Here's logic that makes more sense to me. static bool nodePreventsAutofocus(Node* node) { return node && node->isElementNode() && static_cast<Element*>(focusedNode)->isFormControlElement(); } I believe node can be the document itself and that's the only non-Element value it can have, and I'd like to understand why document having focus would prevent auto-focus. I'd also like to understand why autofocus should take effect if a non-form-element is focused? Is that correct? The above function could alternatively be a private member function that considers m_focusedNode instead of taking a node argument. The code at the call site would then be: if (m_controlRequestingAutofocus && !nodePreventsAutofocus(m_focusedNode.get()()) m_controlRequestingAutofocus->focus(); I think the above structure makes it easier to understand what the code does. > + virtual bool isAutofocusableFormControlElement() const { return false; } This should be named isAutofocusable. And the patch uses this function only on HTMLFormControlElement. I suggest adding it there rather than adding it to Element. > + if (autofocus() && !disabled() && isAutofocusableFormControlElement()) > + document()->setAutofocusedFormControlElement(this); Doing this in the attach function implements a "last one wins" policy which is probably wrong. If you add a new element to the document, whether it becomes the element that gets focused depends on the order that renderers are created. So adding first one element, then triggering layout to create renderers, then adding another later in the document, gives a different autofocus target than if had added both elements before renderers were created. This needs to be fixed. You have to figure out the rule for which element wins if two both specify autofocus, and then implement that rule. And we need test cases that cover this. > bool HTMLFormControlElement::autofocus() const > { > - return hasAttribute(autofocusAttr); > + return m_autofocus; > } Why do we need to store an m_autofocus bit? It seems better to not store it as a bit and just get the attribute each time. What are we optimizing by storing it?

Another important test case is to put an element in the document, make it the one that should be autofocused, then force renderers to be created (or whatever triggers setting the document's pointer to the node), then remove the node. Does the engine incorrectly try to focus the node that is now no longer in the document in that case? With the current code I believe it would, because nothing ever clears the node from the document data member.

A quick recap about how "autofocus" should behave: when specified this attribute should trigger focus on the desired element once the page is loaded, so user has just to type what he needs to. [Multiple elements with autofocus = true] On 4.10.15.4 specs determine that "there must not be more than one element in the document with the autofocus attribute specified", leaving no predetermined behavior in case there're multiple form controls with autofocus set to true. In my implementation focus is fired on "attachment order": the last attached form control element "wins" its focus. [Ignoring autofocus] Relevant text in 4.10.15.4 is: "User agents may ignore this attribute if the user has indicated (for example, by starting to type in a form control) that he does not wish focus to be changed". In my implementation autofocus is ignored if there's another focused FormControlElement. (In reply to comment #6) > > + * fast/forms/script-tests/autofocus.js: Added. > > + (autofocus_element.onfocus): > > + (): > Please remove text like this. I don't see the point of having "():" in our > change log. Sure. [Test] > 3) What does autofocus do when nothing is focused? What about when the > document is focused? (Are those even really separate states?) What about when > an <a> element is focused? That should not be our concern, according to specs. We're supposed to be dealing only with cases when user has clearly stated his intention of "using another form control" (eg: typing into a textbox). > 4) What is the timing of autofocus? Is implicitClose the right time? I'd > like to see tests that cover the last thing that happens before the focus and > the first thing after to make it clear what the timing is. In cases with an > explicit calls to close() and in others cases where the close time is > determined differently. Especially concentrating on timing relative to the > "load" event. I'd particularly like your opinion here. Do you think that would be wiser to trigger focus on autofocus elements in a separate "load" callback? > 6) What happens when multiple elements request autofocus? Which one wins? > To write the test you need to know what the design is. Which one is supposed to > win? Does the order in which the renderers are created effect this? To force This is nice: it's not strictly predictable according to specs.:-) > 7) Does having an autofocus in a subframe result in more than one focused > element? Can autofocus in a frame take focus away from another frame? Should > it, or is that a bug? Noted... > successfully gotten focus. So maybe controlRequestingAutofocus would be good? Ok. > Here's logic that makes more sense to me. Ok. > I'd also like to understand why autofocus should take effect if a > non-form-element is focused? Is that correct? Yes, it "should be" correct: specs only state that autofocus "may be" ignored if user has indicated that focus should not be changed (typing in a control...). > The above function could alternatively be a private member function that > considers m_focusedNode instead of taking a node argument. Ok. > This should be named isAutofocusable. And the patch uses this function only on > HTMLFormControlElement. I suggest adding it there rather than adding it to > Element. Agreed on the name issue, however decision to implement it in Element is due to HTMLDataGridElement, which is not a FormControlElement and seems to be needing autofocus facilities. NOTE: DataGrid's autofocus will be issued separately once this is fixed. > > + if (autofocus() && !disabled() && isAutofocusableFormControlElement()) > > + document()->setAutofocusedFormControlElement(this); > Doing this in the attach function implements a "last one wins" policy which is > probably wrong. If you add a new element to the document, whether it becomes > the element that gets focused depends on the order that renderers are created. > So adding first one element, then triggering layout to create renderers, then > adding another later in the document, gives a different autofocus target than > if had added both elements before renderers were created. If I implement this in a different fashion (in a "load" callback) we would be sure to focus only the "last" (real) element to be attached because all renderers would already be doing their job. Is that correct? Thank you for your invaluable feedback.

(In reply to comment #8) > > 3) What does autofocus do when nothing is focused? What about when the > > document is focused? (Are those even really separate states?) What about when > > an <a> element is focused? > > That should not be our concern, according to specs. We're supposed to be > dealing only with cases when user has clearly stated his intention of "using > another form control" (eg: typing into a textbox). You say "should not be our concern", but I don't understand. Either it behaves one way, or the other. For example, you can use the Tab or Option-Tab key to focus a link. And you might do that while the page is loading before the autofocus behavior triggers. The code you posted doens't seem to handle that case correctly. > > 4) What is the timing of autofocus? Is implicitClose the right time? I'd > > like to see tests that cover the last thing that happens before the focus and > > the first thing after to make it clear what the timing is. In cases with an > > explicit calls to close() and in others cases where the close time is > > determined differently. Especially concentrating on timing relative to the > > "load" event. > > I'd particularly like your opinion here. Do you think that would be wiser to > trigger focus on autofocus elements in a separate "load" callback? My opinion is that we should specify this in the HTML5 specification and match it, and other browsers. If I was designing this I would suggest starting with the notion that the element gets focus soon as it is inserted into the document, but I am not sure that rule would work. We need to be consistent. > > 6) What happens when multiple elements request autofocus? Which one wins? > > To write the test you need to know what the design is. Which one is supposed to > > win? Does the order in which the renderers are created effect this? To force > > This is nice: it's not strictly predictable according to specs.:-) That seems like a problem. Lets get it that fixed. > > I'd also like to understand why autofocus should take effect if a > > non-form-element is focused? Is that correct? > > Yes, it "should be" correct: specs only state that autofocus "may be" ignored > if user has indicated that focus should not be changed (typing in a > control...). OK. Then I think there needs to be code that treats the user's change of focus different from programmatic changes of focus. I also think that user changes of focus at the application level might have to be considered too. For example, in Safari if I changed the focus from the page to the Google search field, the page shouldn't grab the focus back. > > > + if (autofocus() && !disabled() && isAutofocusableFormControlElement()) > > > + document()->setAutofocusedFormControlElement(this); > > Doing this in the attach function implements a "last one wins" policy which is > > probably wrong. If you add a new element to the document, whether it becomes > > the element that gets focused depends on the order that renderers are created. > > So adding first one element, then triggering layout to create renderers, then > > adding another later in the document, gives a different autofocus target than > > if had added both elements before renderers were created. > > If I implement this in a different fashion (in a "load" callback) we would be > sure to focus only the "last" (real) element to be attached because all > renderers would already be doing their job. Is that correct? Possibly. But "last to be attached" is still not a good rule. Layouts can change what "last to be attached" is as a side effect. For example, if a new node is inserted earlier in the document it might be the first to be attached if the layout is done after it's added, but the last to be attached if the layout was done before. Order of call to "attach" seems like it's guaranteed to be a bad way to do this. First we need to understand what rule we are implementing and then we can discuss efficient ways to implement the rule. For example, getElementById implements a "first element in document order with a certain ID" algorithm in a way that is efficient in all normal cases, but can also handle more unusual cases correctly. It might be sufficient to just have a bit to say "there might be an element with autofocus in the document" and then walk the entire document when the bit is set. A single pass over the entire document is probably not too slow. Or we could do that only in cases where there might be more than one autofocus element in the entire document.

The spec says: ====8<==== 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. User agents may also change the scrolling position of the document, or perform some other action that brings the element to the user's attention. The task source for this task is the DOM manipulation task source. User agents may ignore this attribute if the user has indicated (for example, by starting to type in a form control) that he does not wish focus to be changed. ====8<==== This doesn't seem ambiguous.

I don't think we should do this. There are good use-cases that this would kill. For example, for a long-lived page like gmail, they may want to autofocus an input (e.g. the subject field) when you go to compose a new email. Surely we wouldn't want this to work only the first time around. Making them use script seems unnecessary. The spec makes doing this optional. I think we should only make this change if we can point to at least a couple (real) sites where the current behavior leads to a bad user-experience.