[Webkit-unassigned] [Bug 31032] "ignoreAutoFocus" flag is never set

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Nov 14 11:55:51 PST 2009


https://bugs.webkit.org/show_bug.cgi?id=31032


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #43230|review?                     |review-
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2009-11-14 11:55:49 PST ---
(From update of attachment 43230)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list