[Webkit-unassigned] [Bug 29241] focus is not automatically blured after element hide

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #210164|review?                     |review+
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2013-08-30 15:20:05 PST ---
(From update of attachment 210164)
View in context: https://bugs.webkit.org/attachment.cgi?id=210164&action=review

I’m going to say review+, but I don’t think this is 100% right.

> Source/WebCore/dom/Document.cpp:1829
> +    // Style change may reset the focus, e.g. display: none, visibility: hidden.
> +    if (!m_resetHiddenFocusElementTimer.isActive())
> +        m_resetHiddenFocusElementTimer.startOneShot(0);

I’m not sure this is the right place to hook this in.

For one thing, this function doesn’t always update style, so it’s strange to unconditionally set this timer every time.

For another, layout changes could also have an effect, so I don’t understand why having it only in the updateStyleIfNeeded function is sufficient.

This should go in the right place, not just a place that happens to work. We should think about exactly what can affect isFocusable and put this in the one or more bottleneck functions that cover all of those things, but not set up the timer if if there is no focused element or no actual change.

> Source/WebCore/dom/Document.cpp:4714
> +    Element* element = focusedElement();
> +    if (!element)
> +        return;
> +
> +    if (!element->isFocusable())
> +        setFocusedElement(0);

It’s a little strange to use early return for the case of no focused element, but use a nested if statement for the “is still focusable” element. It could and should just be all one if statement. Also, within the class I think it’s better to use the data member rather than a public function.

    if (m_focusedElement && !m_focusedElement->isFocusable())
        setFocusedElement(0);

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