Created attachment 142417 [details] repro document.activeElement returns an element not in the document tree if a new focused element is detached during blur/focusout/domfocusout event. <!DOCTYPE html> <script> document.addEventListener("DOMFocusOut", function() { document.body.removeChild(document.getElementById('holder')); }, false); setTimeout(function() { document.getElementById('console').innerText = 'activeElement should not be an HTMLInputElement: ' + document.activeElement; }, 100); </script> <div id=console></div> <div id=holder> <input autofocus> <input autofocus> </div>
Created attachment 149258 [details] Patch
Comment on attachment 149258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149258&action=review Hi, thanks for the patch. The change basically looks good. I added some comments about style and small caveats. > Source/WebCore/dom/Document.cpp:3769 > + if (newFocusedNode && newFocusedNode->attached()) { Using Node::inDocument() would be preferred in this case. attached() has slightly different semantics. > LayoutTests/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Text test instead of ref test would be preferable in this case because the expectation file can be less cryptic. See "Writing JavaScript-based DOM-only Test Cases" section in http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree Also, this test better fit under dom/HTMLDocument/ because the test is about activeElement. Putting stuff directly under fast/dom is good to avoid if possible.
Comment on attachment 149258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149258&action=review >> Source/WebCore/dom/Document.cpp:3769 >> + if (newFocusedNode && newFocusedNode->attached()) { > > Using Node::inDocument() would be preferred in this case. attached() has slightly different semantics. We should use Node::isFocusable() and/or Node::supportsFocus(). e.g. <input disabled> shouldn't have focus.
> > Using Node::inDocument() would be preferred in this case. attached() has slightly different semantics. > We should use Node::isFocusable() and/or Node::supportsFocus(). Please add a test that would have failed if any of these incorrect checks were used.
(In reply to comment #4) > > > Using Node::inDocument() would be preferred in this case. attached() has slightly different semantics. > > We should use Node::isFocusable() and/or Node::supportsFocus(). > > Please add a test that would have failed if any of these incorrect checks were used. http://trac.webkit.org/changeset/121079 has fixed this issue as well. It can probably be closed now.
(In reply to comment #5) > http://trac.webkit.org/changeset/121079 has fixed this issue as well. It can probably be closed now. No, r121079 is unrelated to this.
(In reply to comment #6) > (In reply to comment #5) > > http://trac.webkit.org/changeset/121079 has fixed this issue as well. It can probably be closed now. > > No, r121079 is unrelated to this. The TreeScope.cpp change [in TreeScope::focusedNode()] seems to fix this issue as well. document.activeElement (for this particular issue) now returns HTMLBodyElement instead of HTMLInputElement.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > http://trac.webkit.org/changeset/121079 has fixed this issue as well. It can probably be closed now. > > > > No, r121079 is unrelated to this. > > The TreeScope.cpp change [in TreeScope::focusedNode()] seems to fix this issue as well. > document.activeElement (for this particular issue) now returns HTMLBodyElement instead of HTMLInputElement. So we can close this. Or if you have time, test-only patch would be also appreciated!
Created attachment 149715 [details] Another repro
(In reply to comment #7) > > No, r121079 is unrelated to this. > > The TreeScope.cpp change [in TreeScope::focusedNode()] seems to fix this issue as well. > document.activeElement (for this particular issue) now returns HTMLBodyElement instead of HTMLInputElement. Ok, r121079 is related to this, but it just hid a particular case of this bug. I'm changing the bug title to cover other cases.
(In reply to comment #10) > (In reply to comment #7) > > > No, r121079 is unrelated to this. > > > > The TreeScope.cpp change [in TreeScope::focusedNode()] seems to fix this issue as well. > > document.activeElement (for this particular issue) now returns HTMLBodyElement instead of HTMLInputElement. > > Ok, r121079 is related to this, but it just hid a particular case of this bug. I'm changing the bug title to cover other cases. That's right. I'll upload another patch for the same.
Created attachment 149727 [details] Patch
Comment on attachment 149727 [details] Patch Attachment 149727 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13094793 New failing tests: plugins/mouse-events-fixedpos.html plugins/keyboard-events.html plugins/mouse-events.html
Created attachment 149742 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 149727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149727&action=review > Source/WebCore/dom/Document.cpp:3763 > + if (newFocusedNode && newFocusedNode->isFocusable()) { I'm not sure if it's safe to use only isFocusable(). Do we need checks similar to Element::focus()?
(In reply to comment #15) > (From update of attachment 149727 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149727&action=review > > > Source/WebCore/dom/Document.cpp:3763 > > + if (newFocusedNode && newFocusedNode->isFocusable()) { > > I'm not sure if it's safe to use only isFocusable(). > Do we need checks similar to Element::focus()? The isFocusable() check on newFocusedNode seems appropriate the way it is in Document::setFocusedNode(). Plugin elements fail the supportsFocus() check (called from isFocusable()) since they do not have rareData set for them. Currently plugin elements are getting the focus since no "focusable" check was carried out in setFocusedNode(). Perhaps we should look at implementing supportsFocus() for Plugin elements as well, similar to say the HTMLMediaElement or HTMLFrameElementBase.
Created attachment 151677 [details] Patch
Have uploaded another patch going by the logic that the isFocusable() call in Document::setFocusedNode() is safe and that supportsFocus() method should instead be implemented for the plugin element as well.
Comment on attachment 151677 [details] Patch Attachment 151677 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13180415 New failing tests: fast/events/tabindex-focus-blur-all.html
Created attachment 151688 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 151923 [details] Patch
Comment on attachment 151923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151923&action=review > Source/WebCore/ChangeLog:24 > + The supportsFocus() check (called from isFocusable() method in Document::setFocusedNode()) > + fails for plugin elements if they do not have rareData set for them. Previously plugin > + elements were able to get the focus since no "focusable" check was carried out in > + setFocusedNode(). That's because Element::supportsFocus checks tabIndexSetExplicitly(), right? Are you saying that the plugin element should always be focusable? If so, then we should just return true in HTMLPlugInElement::supportsFocus. If we do need to check the existence of render, then it's odd that we don't have to do that when there is no rareData. e.g. calling getElementsById will create a rare data. r- because of this. > Source/WebCore/html/HTMLPlugInElement.cpp:115 > + return hasRareData() ? HTMLElement::supportsFocus() : true; This is the line I'm talking about, and we certainly need a test for this. If there is an existing test that catches this problem, then we should mention that in the change log.
(In reply to comment #22) > (From update of attachment 151923 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151923&action=review > > > Source/WebCore/ChangeLog:24 > > + The supportsFocus() check (called from isFocusable() method in Document::setFocusedNode()) > > + fails for plugin elements if they do not have rareData set for them. Previously plugin > > + elements were able to get the focus since no "focusable" check was carried out in > > + setFocusedNode(). > > That's because Element::supportsFocus checks tabIndexSetExplicitly(), right? Are you saying that the plugin element should always be focusable? > If so, then we should just return true in HTMLPlugInElement::supportsFocus. If we do need to check the existence of render, > then it's odd that we don't have to do that when there is no rareData. e.g. calling getElementsById will create a rare data. r- because of this. > > > Source/WebCore/html/HTMLPlugInElement.cpp:115 > > + return hasRareData() ? HTMLElement::supportsFocus() : true; > > This is the line I'm talking about, and we certainly need a test for this. If there is an existing test that catches this problem, > then we should mention that in the change log. Thanks for the review Ryosuke. There are two separate scenarios for which the supportsFocus() implementation in HTMLPlugInElement fails. The second patch submitted by me (which did not contain any supportsFocus() implementation for plugin elements) failed the following test cases: plugins/mouse-events-fixedpos.html plugins/keyboard-events.html plugins/mouse-events.html This was because of having added the isFocusable() check in Document::setFocusedNode(). Ideally we expect that any element to be focused should be able to clear the isFocusable() check.(?) The aforementioned testcases expect the plugin element to get the focus event which it was unable to do so since it lacked rare data and was thus failing the supportsFocus() check called from isFocusable() method. Post this I made another patch which implemented supportsFocus() method for Plugin elements and was essentially just returning true. But this fails the case: fast/events/tabindex-focus-blur-all.html In this case, when trying to set the tabIndex() on the plugin element, supportsFocus() is called which redirects to the supportsFocus() implementation for plugin element (which returned true). But for this particular testcase we expect supportsFocus() to return false since even though it has rare data it fails the tabIndexSetExplicitly() check. Thus we have two separate scenarios wherein, if a plugin does not have rare data set for it, supportsFocus() still returns true (going by the assumption that all plugins should be able to get focus) and the second being that if a plugin has rare data set for it, supportsFocus() should run an additional check for tabIndexSetExplicitly(). I understand that the current implementation of supportsFocus() for plugins indeed does not seem right. I would appreciate any pointers towards a better solution here.
Created attachment 187228 [details] Patch
Comment on attachment 187228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187228&action=review > LayoutTests/fast/dom/HTMLDocument/set-focus-on-valid-element.html:11 > + shouldNotBe("document.activeElement.id", "willBeDisabled"); Just for curious, who gains the focus? it looks there is no focusable element after removing "willBeRemoved" node.
Comment on attachment 187228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187228&action=review >> LayoutTests/fast/dom/HTMLDocument/set-focus-on-valid-element.html:11 >> + shouldNotBe("document.activeElement.id", "willBeDisabled"); > > Just for curious, who gains the focus? it looks there is no focusable element after removing "willBeRemoved" node. Right, there is no focused node. activeElement returns the body, which is the default value in a case of no focused node.
Created attachment 187230 [details] Patch Vefiry activeElement=body
Comment on attachment 187230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187230&action=review > Source/WebCore/dom/Document.cpp:3357 > + if (newFocusedNode && (newFocusedNode->isPluginElement() || newFocusedNode->isFocusable())) { This smells wrong. The node which reaches here should be focusable IMO. We should rather have this as an assert(). In the case on the test, we could just check it on HTMLFormControlElement.cpp:focusPostAttach().
Comment on attachment 187230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187230&action=review >> Source/WebCore/dom/Document.cpp:3357 >> + if (newFocusedNode && (newFocusedNode->isPluginElement() || newFocusedNode->isFocusable())) { > > This smells wrong. The node which reaches here should be focusable IMO. We should rather have this as an assert(). > In the case on the test, we could just check it on HTMLFormControlElement.cpp:focusPostAttach(). The problem is that the newFocusedNode was isFocusable at the beginning of this function, but it became !isFocusable by blur/focusout/DOMFocusOut event handlers. This issue happens with any elements with tabIndex attribute, not only form controls.
Comment on attachment 187230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187230&action=review >>> Source/WebCore/dom/Document.cpp:3357 >>> + if (newFocusedNode && (newFocusedNode->isPluginElement() || newFocusedNode->isFocusable())) { >> >> This smells wrong. The node which reaches here should be focusable IMO. We should rather have this as an assert(). >> In the case on the test, we could just check it on HTMLFormControlElement.cpp:focusPostAttach(). > > The problem is that the newFocusedNode was isFocusable at the beginning of this function, but it became !isFocusable by blur/focusout/DOMFocusOut event handlers. > This issue happens with any elements with tabIndex attribute, not only form controls. Ah, I see the point. That makes sense.
Comment on attachment 187230 [details] Patch Clearing flags on attachment: 187230 Committed r142234: <http://trac.webkit.org/changeset/142234>
All reviewed patches have been landed. Closing bug.
Comment on attachment 187230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187230&action=review >>>> Source/WebCore/dom/Document.cpp:3357 >>>> + if (newFocusedNode && (newFocusedNode->isPluginElement() || newFocusedNode->isFocusable())) { >>> >>> This smells wrong. The node which reaches here should be focusable IMO. We should rather have this as an assert(). >>> In the case on the test, we could just check it on HTMLFormControlElement.cpp:focusPostAttach(). >> >> The problem is that the newFocusedNode was isFocusable at the beginning of this function, but it became !isFocusable by blur/focusout/DOMFocusOut event handlers. >> This issue happens with any elements with tabIndex attribute, not only form controls. > > Ah, I see the point. That makes sense. I think this is tricky enough that it deserves a short explanatory comment. This is "why" comment, not a "what" comment. :) Something like... The blur/focusout event may have made newFocusedNode unfocusable, so we need to check here that it's focusable. Also, why do we need to check if it's a plugin element? Do we have a test that hits this case? Should all plugins just always return true for isFocusable? I don't know the code well enough to answer these questions.
(In reply to comment #33) > Also, why do we need to check if it's a plugin element? Do we have a test that hits this case? Should all plugins just always return true for isFocusable? I don't know the code well enough to answer these questions. Please see the comment #13 to #16 in this bug.