Bug 40338 - display:none element should not retain focus
Summary: display:none element should not retain focus
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 67603 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-08 15:27 PDT by Ojan Vafai
Modified: 2019-07-01 10:02 PDT (History)
7 users (show)

See Also:


Attachments
test case (860 bytes, text/html)
2010-06-08 15:27 PDT, Ojan Vafai
no flags Details
Patch (1.89 KB, patch)
2011-09-08 07:37 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff
Updated patch (2.25 KB, patch)
2011-09-08 08:30 PDT, Vineet Chaudhary (vineetc)
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (4.50 KB, patch)
2011-09-12 04:38 PDT, Vineet Chaudhary (vineetc)
rniwa: review-
Details | Formatted Diff | Diff
Updated Patch (4.68 KB, patch)
2011-09-15 09:57 PDT, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-06-08 15:27:30 PDT
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).
Comment 1 Alexey Proskuryakov 2011-09-05 13:49:56 PDT
*** Bug 67603 has been marked as a duplicate of this bug. ***
Comment 2 Kaustubh Atrawalkar 2011-09-06 23:56:48 PDT
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.
Comment 3 Kaustubh Atrawalkar 2011-09-07 00:36:15 PDT
Element::blur() does the same thing, which we might able to use here.
Comment 4 Vineet Chaudhary (vineetc) 2011-09-08 07:37:17 PDT
Created attachment 106734 [details]
Patch
Comment 5 Vineet Chaudhary (vineetc) 2011-09-08 07:47:27 PDT
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.?
Comment 6 Vineet Chaudhary (vineetc) 2011-09-08 08:30:03 PDT
Created attachment 106741 [details]
Updated patch

Please ignore my previous patch.
Comment 7 Darin Adler 2011-09-08 08:32:33 PDT
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();
Comment 8 Alexey Proskuryakov 2011-09-08 08:42:20 PDT
Also, please verify whether the new behavior matches both Firefox and IE.
Comment 9 Kaustubh Atrawalkar 2011-09-08 20:23:54 PDT
(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.?
Comment 10 Vineet Chaudhary (vineetc) 2011-09-12 04:38:07 PDT
Created attachment 107039 [details]
Updated Patch

Added Layout test to ensure element does not retain the focus if display property set to NONE.
Comment 11 Vineet Chaudhary (vineetc) 2011-09-12 08:21:15 PDT
(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.
Comment 12 Alexey Proskuryakov 2011-09-12 09:03:23 PDT
> 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.
Comment 13 Ryosuke Niwa 2011-09-14 20:04:20 PDT
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.
Comment 14 Vineet Chaudhary (vineetc) 2011-09-15 09:57:50 PDT
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.
Comment 15 Alexey Proskuryakov 2011-09-15 10:38:20 PDT
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.
Comment 16 Ryosuke Niwa 2011-09-15 10:54:20 PDT
> > 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?
Comment 17 Eric Seidel (no email) 2011-12-09 14:32:27 PST
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.
Comment 18 Ian 'Hixie' Hickson 2012-02-21 21:28:18 PST
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."
Comment 19 Alexey Proskuryakov 2012-02-21 22:23:46 PST
Given that IE and WebKit agree here, it seems unwise to change our behavior. Can the spec be corrected to match IE and WebKit?