Bug 29241 – Hiding a focused element should unfocus it and fire a blur event

• Component: DOM
• Status: RESOLVED WONTFIX

[Evgenii Schepotiev]

1. Try to focus on HTMLInput element
2. Than hide it for example with js
3. You can still type in it & focus is still there

In DOM spec you should blur input.

Sincerely yours, YouTrack Team
http://www.jetbrains.com/youtrack/index.html

[Darin Adler]

*** Bug 120273 has been marked as a duplicate of this bug. ***

[Darin Adler]

(In reply to comment #8 in bug 120273)
> I tried using updateFocusAppearance timer, but Document::updateFocusAppearanceTimerFired calls Document::updateLayout, from Document::updateLayout() again we are scheduling a timer. It cases  above 2 functions being called repeatedly.

Please explain in more detail why the two functions are called repeatedly. I’d assume that after the first time there would no longer be a focused element, so there would be no repeated call.

[Arunprasad Rajkumar]

(In reply to comment #2)
> (In reply to comment #8 in bug 120273)
> > I tried using updateFocusAppearance timer, but Document::updateFocusAppearanceTimerFired calls Document::updateLayout, from Document::updateLayout() again we are scheduling a timer. It cases  above 2 functions being called repeatedly.
> 
> Please explain in more detail why the two functions are called repeatedly. I’d assume that after the first time there would no longer be a focused element, so there would be no repeated call.


void Document::updateFocusAppearanceTimerFired(Timer<Document>*)
{
    Element* element = focusedElement();
    if (!element)
        return;

    updateLayout();
>> From this timer callback Document::updateLayout is called before resetting the focus. Again from Document::updateLayout() updateFocusAppearance timer will be scheduled.

    if (element->isFocusable())
        element->updateFocusAppearance(m_updateFocusAppearanceRestoresSelection);
    else
        setFocusedElement(0);

>> I'm don't think calling a updateLayout() from this statement is good idea.
}

Also once the Element is hidden using "display: none", from the rendererObject detach Document::cancelFocusAppearanceUpdate() is been called, which stops the updateFocusAppearance timer.(so we can't reuse updateFocusAppearance timer anyway)

What I'm currently trying is scheduling a separate timer from Document::cancelFocusAppearanceUpdate() to reset the focus. It works fine for "display: none", but not for "visibility:hidden". I'm trying to make it work for "visibility: hidden" and "width:0px" || "height: 0px";

[Arunprasad Rajkumar]

Created attachment 210164 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #4)
> Created an attachment (id=210164) [details]
> Patch

Chrome is not passing the modified test-case.(i.e setting display:none,visibility:hidden via timer).

[Darin Adler]

Comment on attachment 210164 [details]
Patch

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);

[Arunprasad Rajkumar]

(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 :)

[Darin Adler]

(In reply to comment #7)
> 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?

Read HTMLFormControlElement::isFocusable, for example. It checks the renderer’s size. I am not an expressing an opinion about whether it should do that or not. But it does. And if we haven’t done layout, we can’t check a renderer’s size.

[Darin Adler]

(In reply to comment #7)
> Initially I felt Document::cancelFocusAppearanceUpdate() was the right place

I don’t think that is the right place, and I am not sure why you do.

> > 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.

This new logic needs to run any time we possibly changed what isFocusable will return for the focused element.

I’m don’t think that cancelFocusAppearanceUpdate needs to be called in those same places, though. We don’t just want to cancel that timer because something might have changed. I don’t think you’re on the right track here.

[Arunprasad Rajkumar]

Created attachment 210193 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #9)
> (In reply to comment #7)
> > Initially I felt Document::cancelFocusAppearanceUpdate() was the right place
> 
> I don’t think that is the right place, and I am not sure why you do.
> 
> > > 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.
> 
> This new logic needs to run any time we possibly changed what isFocusable will return for the focused element.
> 
> I’m don’t think that cancelFocusAppearanceUpdate needs to be called in those same places, though. We don’t just want to cancel that timer because something might have changed. I don’t think you’re on the right track here.

Without seeing your comment I uploaded a patch. I will upload a new patch which meets those scenarios.

[Arunprasad Rajkumar]

Created attachment 210200 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #12)
> Created an attachment (id=210200) [details]
> Patch

The patch handles the both layout & style change.

[Arunprasad Rajkumar]

Created attachment 212195 [details]
Patch

[Darin Adler]

Comment on attachment 212195 [details]
Patch

One risk is that some call sites might call layout directly on the FrameView rather than through Document.

[WebKit Commit Bot]

Comment on attachment 212195 [details]
Patch

Clearing flags on attachment: 212195

Committed r156185: <http://trac.webkit.org/changeset/156185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121727

[Alexey Proskuryakov]

This caused a lot of crashes on bots: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156186%20(10285)/results.html>.

Also, the test from this patch fails: <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r156208%20(11940)/fast/dom/HTMLDocument/active-element-gets-unfocusable-pretty-diff.html>.

Rolling out in a moment.

[Alexey Proskuryakov]

Rolled out in <https://trac.webkit.org/r156210>.

[Arunprasad Rajkumar]

Created attachment 212271 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #19)
> This caused a lot of crashes on bots: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r156186%20(10285)/results.html>.


Sorry for breaking the LayoutTests. Failures were due the Assertions in method " Element::isFocusable ()" to confirm the Layout is not required in that state, (I didn't test the patch in the Debug build).

New patch considers that scenario as well (thanks to https://chromium.googlesource.com/chromium/blink/+/c58f636fd18fc27944c42e27d6a92a36867c57e1).

> 
> Also, the test from this patch fails: <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r156208%20(11940)/fast/dom/HTMLDocument/active-element-gets-unfocusable-pretty-diff.html>.
> 

http://trac.webkit.org/r155250 moved the js-test-pre.js & js-test-post.js to LayoutTests/resources. That's why it was failing.

> Rolling out in a moment.

[Darin Adler]

Comment on attachment 212271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review

> Source/WebCore/dom/Document.cpp:4719
> +    if (element->renderer() && element->renderer()->needsLayout())
> +        return;

This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree.

Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong.

[Darin Adler]

Comment on attachment 212271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review

>> Source/WebCore/dom/Document.cpp:4719
>> +        return;
> 
> This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree.
> 
> Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong.

it’s *not* a fully correct fix

[Darin Adler]

Comment on attachment 212271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review

>>> Source/WebCore/dom/Document.cpp:4719
>>> +        return;
>> 
>> This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree.
>> 
>> Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong.
> 
> it’s *not* a fully correct fix

A correct fix would be something more like this:

    if (FrameView* view = this->view()) {
        if (view->needsLayout())
            return;
    }

And also, we have to add code to make sure that this “reset hidden focus element logic” runs when the layout is done.

[Kent Tamura]

*** Bug 112992 has been marked as a duplicate of this bug. ***

[Arunprasad Rajkumar]

(In reply to comment #25)
> (From update of attachment 212271 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212271&action=review
> 
> >>> Source/WebCore/dom/Document.cpp:4719
> >>> +        return;
> >> 
> >> This may be sufficient to make the assertion go away, but it’s a fully correct fix. It’s not safe to call setFocusedElement until layout is done, not just for one element, but for the entire render tree.
> >> 
> >> Instead, this just checks what the assertion will check, which may seem mechanically correct, but is wrong.
> > 
> > it’s *not* a fully correct fix
> 
> A correct fix would be something more like this:
> 
>     if (FrameView* view = this->view()) {
>         if (view->needsLayout())
>             return;
>     }
> 
> And also, we have to add code to make sure that this “reset hidden focus element logic” runs when the layout is done.

Yes, already resetHiddenFocusElementSoon() is called(from Document::updateLayout) after finishing the layout.

    // Only do a layout if changes have occurred that make it necessary.      
    if (frameView && renderView() && (frameView->layoutPending() || renderView()->needsLayout()))
        **frameView->layout();**

    // Active focus element's isFocusable() state may change after Layout. e.g. width: 0px or height: 0px.
    **resetHiddenFocusElementSoon();**

[Arunprasad Rajkumar]

Created attachment 212303 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 212303 [details]
Patch

Clearing flags on attachment: 212303

Committed r156252: <http://trac.webkit.org/changeset/156252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Looks like this patch broke facebook.com on iOS.

When the user taps on the status on facebook.com, the page focuses an textarea, which is then made momentarily visible by setting display:none.  The page then removes display:none later and expects the textarea to be still focused.

With this patch, the focus is removed after the page sets display:none and the keyboard disappears.  Since the focus is never reset on the textarea, the user can never type in anything.

The specification may need to change here given that this (rather odd) behavior/expectation exists on one of the most popular websites on the Web.

[Darin Adler]

*** Bug 216248 has been marked as a duplicate of this bug. ***

[Darin Adler]

Any change here should wait on the resolution of https://github.com/w3c/uievents/issues/236

[Darin Adler]

*** Bug 40338 has been marked as a duplicate of this bug. ***

[Darin Adler]

Bug 216418 is the same issue, but with "visibility: hidden".