[webkit-reviews] review granted: [Bug 29241] focus is not automatically blured after element hide : [Attachment 210164] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Arunprasad Rajkumar
<arurajku at cisco.com>'s request for review:
Bug 29241: focus is not automatically blured after element hide
https://bugs.webkit.org/show_bug.cgi?id=29241

Attachment 210164: Patch
https://bugs.webkit.org/attachment.cgi?id=210164&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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);