Bug 40338 – display:none element should not retain focus

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: HTML Editing

[Ojan Vafai]

Created attachment 58193 [details]
test case

If display:none is set on the focused element, then document.activeElement should no longer return that element and it should not receive key events. In Gecko, display:none elements don't receive key events, but Gecko does have the document.activeElement bug as well (https://bugzilla.mozilla.org/show_bug.cgi?id=570835).

[Alexey Proskuryakov]

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

[Kaustubh Atrawalkar]

here the issue seems to be cancelFocusAppearenceUpdate() call. Which does remove the focus appearance but does not set the focus back to 0. The current usage of this call is only in focus() & detach() calls in Element.cpp. We can add setFocus(0) in this function definition to avoid retaining of the focus after display:none.

[Kaustubh Atrawalkar]

Element::blur() does the same thing, which we might able to use here.

[Vineet Chaudhary (vineetc)]

Created attachment 106734 [details]
Patch

[Vineet Chaudhary (vineetc)]

Kaustubh As you said cancelFocusAppearenceUpdate() just updates the focus appearance, I have tried to setFocus(0) on Element::detach() and it fixes the issue, but we can't directly use blur() to just avoid calling cancelFocusAppearenceUpdate() twice.

Alexey Could you please review this patch.?

[Vineet Chaudhary (vineetc)]

Created attachment 106741 [details]
Updated patch

Please ignore my previous patch.

[Darin Adler]

Comment on attachment 106741 [details]
Updated patch

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

WebKit bug fixes need to come with regression tests. Please create a test and include it in the patch along with the fix.

> Source/WebCore/dom/Element.cpp:1758
> +    Document* doc = document();

Please don’t use the abbreviation "doc" instead of a word. The preferred way to write this in WebKit code is:

    Document* document = this->document();

[Alexey Proskuryakov]

Also, please verify whether the new behavior matches both Firefox and IE.

[Kaustubh Atrawalkar]

(In reply to comment #5)

I was suggesting to use blur instead of cancelFocusApperance() rather than adding new function with the same definition. Why to just increase loc :)
> Kaustubh As you said cancelFocusAppearenceUpdate() just updates the focus appearance, I have tried to setFocus(0) on Element::detach() and it fixes the issue, but we can't directly use blur() to just avoid calling cancelFocusAppearenceUpdate() twice.
> 
> Alexey Could you please review this patch.?

[Vineet Chaudhary (vineetc)]

Created attachment 107039 [details]
Updated Patch

Added Layout test to ensure element does not retain the focus if display property set to NONE.

[Vineet Chaudhary (vineetc)]

(In reply to comment #8)
> Also, please verify whether the new behavior matches both Firefox and IE.

I tested the new behavior with firefox & IE.

Firefox 6.0.2 : It doesn't sends key events to element when hidden.
IE 7.0        : It does sends key events to element when hidden.

So new new behavior matches with the firefox but not IE.

[Alexey Proskuryakov]

> Firefox 6.0.2 : It doesn't sends key events to element when hidden.
> IE 7.0        : It does sends key events to element when hidden.

If WebKit matches IE here, then we probably shouldn't make any change. Given that combined market share or IE and WebKit based browsers is much higher than that of Gecko (Firefox), it's likely best for compatibility to keep existing behavior.

[Ryosuke Niwa]

Comment on attachment 107039 [details]
Updated Patch

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

> Source/WebCore/ChangeLog:8
> +        When elements gets hidden its renderer gets drop, such element should

the word hidden makes me think of visibility: hidden.  Should we remove focus in that case as well?  If so, we need a test for that.

> Source/WebCore/page/FrameView.cpp:2212
> +    // As HTMLAreaElement doesn't have renderer() we dont want to set focus to 0.

Is this tested somewhere?

> LayoutTests/ChangeLog:8
> +        Added test case to check the behavior.

"check the behavior" is too general.

> LayoutTests/fast/events/remove-focus-on-hidden-elelment.html:4
> +

Why do we need this line?

> LayoutTests/fast/events/remove-focus-on-hidden-elelment.html:12
> +if (window.layoutTestController) {
> + layoutTestController.dumpAsText();
> +}

No curly brackets around a single line statement.

> LayoutTests/fast/events/remove-focus-on-hidden-elelment.html:28
> +testArea.addEventListener('keydown', function (event) { keycount++ ;});
> +
> +testArea.style.display = 'none';
> +
> +// Send keyboard event
> +eventSender.keyDown("A");
> +
> +logResult(keycount ? "FAIL" : "PASS");
> +
> +function logResult(str) {
> + document.getElementById('result').innerHTML = str;
> +}

Why do we check that the node no longer retain focus by counting keydown event? It seems like we should directly check focused property.

[Vineet Chaudhary (vineetc)]

Created attachment 107508 [details]
Updated Patch

(In reply to comment #13)

> the word hidden makes me think of visibility: hidden.  Should we remove focus in that case as well?  If so, we need a test for that.

Yes actually word should be for display:none only, As for hidden elements renderers are not dropped.

> > +    // As HTMLAreaElement doesn't have renderer() we dont want to set focus to 0.
> Is this tested somewhere?
In LayoutTests/fast/events/mouse-focus-imagemap.html this test case AreaElement doesn't have renderer and hence we loose focus on image.
 
> "check the behavior" is too general.
> 
> Why do we need this line?
> 
> No curly brackets around a single line statement.

Done
 
> Why do we check that the node no longer retain focus by counting keydown event? It seems like we should directly check focused property.

I have modified layout test case again because even after setting display to none "document.activeElement" still remains HTMLTextAreaElement,
as we reset focus in performPostLayoutTasks() which is async call (called on timer). So that we should check the "activeElement" only after
performPostLayoutTasks() is called.

[Alexey Proskuryakov]

Please provide a rationale for making this change.

Handling key events on an invisible element is a fairly common trick (whether it's commonly done on visibility:hidden or display:none, I don't know). As your testing indicates that vast majority of browsers agrees on this, the right resolution seems to be WONTFIX at this time.

[Ryosuke Niwa]

> > the word hidden makes me think of visibility: hidden.  Should we remove focus in that case as well?  If so, we need a test for that.
> 
> Yes actually word should be for display:none only, As for hidden elements renderers are not dropped.

That seems wrong. visibility:hidden elements are not selectable in WebKit why should they be focusable?

[Eric Seidel (no email)]

Per ap and rniwa's comments above, marking this WONTFIX.  Feel free to re-open if you have more evidence of disagreement with other browsers or with the HTML5 spec.

[Ian 'Hixie' Hickson]

FWIW, the spec and Gecko seem to agree that this is a valid (not WONTFIX) bug. The spec says:

"When an element that is focused stops being a focusable element, or stops being focused without another element being explicitly focused in its stead, the user agent should synchronously run the focusing steps for the body element, if there is one; if there is not, then the user agent should synchronously run the unfocusing steps for the affected element only."

[Alexey Proskuryakov]

Given that IE and WebKit agree here, it seems unwise to change our behavior. Can the spec be corrected to match IE and WebKit?

[Darin Adler]

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

[Darin Adler]

*** This bug has been marked as a duplicate of bug 29241 ***