Bug 29241

Summary: Hiding a focused element should unfocus it and fire a blur event
Product: WebKit Reporter: Evgenii Schepotiev <evgenii.schepotiev>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: a.bah, ap, ararunprasad, arurajku, commit-queue, darin, esprehn+autocc, kangil.han, ojan, rniwa, simon.fraser, syoichi, tkent, tonikitoo
Priority: P3    
Version: 523.x (Safari 3)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=40338
https://bugs.webkit.org/show_bug.cgi?id=216418
https://bugs.webkit.org/show_bug.cgi?id=217240
Bug Depends on: 121727, 133196    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Evgenii Schepotiev 2009-09-14 04:55:09 PDT
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
Comment 1 Darin Adler 2013-08-29 10:16:24 PDT
*** Bug 120273 has been marked as a duplicate of this bug. ***
Comment 2 Darin Adler 2013-08-29 10:21:28 PDT
(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.
Comment 3 Arunprasad Rajkumar 2013-08-30 10:28:11 PDT
(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";
Comment 4 Arunprasad Rajkumar 2013-08-30 14:54:38 PDT
Created attachment 210164 [details]
Patch
Comment 5 Arunprasad Rajkumar 2013-08-30 14:57:28 PDT
(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).
Comment 6 Darin Adler 2013-08-30 15:20:05 PDT
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);
Comment 7 Arunprasad Rajkumar 2013-08-30 23:43:34 PDT
(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 :)
Comment 8 Darin Adler 2013-08-31 08:21:22 PDT
(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.
Comment 9 Darin Adler 2013-08-31 08:24:17 PDT
(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.
Comment 10 Arunprasad Rajkumar 2013-08-31 08:39:31 PDT
Created attachment 210193 [details]
Patch
Comment 11 Arunprasad Rajkumar 2013-08-31 09:29:41 PDT
(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.
Comment 12 Arunprasad Rajkumar 2013-08-31 11:58:19 PDT
Created attachment 210200 [details]
Patch
Comment 13 Arunprasad Rajkumar 2013-08-31 11:59:18 PDT
(In reply to comment #12)
> Created an attachment (id=210200) [details]
> Patch

The patch handles the both layout & style change.
Comment 14 Arunprasad Rajkumar 2013-09-20 10:57:43 PDT
Created attachment 212195 [details]
Patch
Comment 15 Darin Adler 2013-09-20 11:44:22 PDT
Comment on attachment 212195 [details]
Patch

One risk is that some call sites might call layout directly on the FrameView rather than through Document.
Comment 16 WebKit Commit Bot 2013-09-20 12:05:46 PDT
Comment on attachment 212195 [details]
Patch

Clearing flags on attachment: 212195

Committed r156185: <http://trac.webkit.org/changeset/156185>
Comment 17 WebKit Commit Bot 2013-09-20 12:05:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Commit Bot 2013-09-20 16:53:41 PDT
Re-opened since this is blocked by bug 121727
Comment 20 Alexey Proskuryakov 2013-09-20 16:58:54 PDT
Rolled out in <https://trac.webkit.org/r156210>.
Comment 21 Arunprasad Rajkumar 2013-09-21 04:28:26 PDT
Created attachment 212271 [details]
Patch
Comment 22 Arunprasad Rajkumar 2013-09-21 04:45:10 PDT
(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.
Comment 23 Darin Adler 2013-09-21 21:03:16 PDT
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.
Comment 24 Darin Adler 2013-09-21 21:03:44 PDT
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
Comment 25 Darin Adler 2013-09-21 21:07:56 PDT
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.
Comment 26 Kent Tamura 2013-09-22 06:19:07 PDT
*** Bug 112992 has been marked as a duplicate of this bug. ***
Comment 27 Arunprasad Rajkumar 2013-09-22 07:32:45 PDT
(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();**
Comment 28 Arunprasad Rajkumar 2013-09-22 09:35:00 PDT
Created attachment 212303 [details]
Patch
Comment 29 WebKit Commit Bot 2013-09-22 16:18:07 PDT
Comment on attachment 212303 [details]
Patch

Clearing flags on attachment: 212303

Committed r156252: <http://trac.webkit.org/changeset/156252>
Comment 30 WebKit Commit Bot 2013-09-22 16:18:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Ryosuke Niwa 2014-05-22 17:43:12 PDT
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.
Comment 32 Darin Adler 2020-09-10 04:13:24 PDT
*** Bug 216248 has been marked as a duplicate of this bug. ***
Comment 33 Darin Adler 2020-09-12 12:44:23 PDT
Any change here should wait on the resolution of https://github.com/w3c/uievents/issues/236
Comment 34 Darin Adler 2020-09-12 12:45:15 PDT
*** Bug 40338 has been marked as a duplicate of this bug. ***
Comment 35 Darin Adler 2020-09-12 12:46:42 PDT
Bug 216418 is the same issue, but with "visibility: hidden".