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).
*** Bug 67603 has been marked as a duplicate of this bug. ***
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.
Element::blur() does the same thing, which we might able to use here.
Created attachment 106734 [details] Patch
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.?
Created attachment 106741 [details] Updated patch Please ignore my previous patch.
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();
Also, please verify whether the new behavior matches both Firefox and IE.
(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.?
Created attachment 107039 [details] Updated Patch Added Layout test to ensure element does not retain the focus if display property set to NONE.
(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.
> 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 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.
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.
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.
> > 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?
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.
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."
Given that IE and WebKit agree here, it seems unwise to change our behavior. Can the spec be corrected to match IE and WebKit?
Any change here should wait on the resolution of https://github.com/w3c/uievents/issues/236
*** This bug has been marked as a duplicate of bug 29241 ***