[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





--- Comment #7 from Arunprasad Rajkumar <arurajku at cisco.com>  2013-08-30 23:43:34 PST ---
(In reply to comment #6)
> (From update of attachment 210164 [details])
> 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.
Initially I felt Document::cancelFocusAppearanceUpdate() was the right place, But unfortunately it wasn't called for "visibility:hidden". I will try to find-out the possibilities to call Document::cancelFocusAppearanceUpdate() from appropriate place to satisfy the "visibility: hidden" case. Once it is done, we can start a timer to reset a focus from Document::cancelFocusAppearanceUpdate().

> 
> 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.
I'm not sure t how layout could affect the focus state, do you mean setting width:0px || height:0px to an element should unfocus it?

> 
> 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.
Definitely. As mentioned earlier, I will try to call Document::cancelFocusAppearanceUpdate() for "visibility:hidden" also.
> 
> > 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);

Sure, I will change this :)

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