RESOLVED FIXED 86707
document.activeElement should not return a non-focusable element
https://bugs.webkit.org/show_bug.cgi?id=86707
Summary document.activeElement should not return a non-focusable element
Kent Tamura
Reported 2012-05-16 22:53:46 PDT
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>
Attachments
repro (410 bytes, text/html)
2012-05-16 22:53 PDT, Kent Tamura
no flags
Patch (4.44 KB, patch)
2012-06-25 00:32 PDT, Arpita Bahuguna
no flags
Another repro (529 bytes, text/html)
2012-06-27 02:29 PDT, Kent Tamura
no flags
Patch (4.25 KB, patch)
2012-06-27 04:38 PDT, Arpita Bahuguna
no flags
Archive of layout-test-results from ec2-cr-linux-01 (674.15 KB, application/zip)
2012-06-27 07:00 PDT, WebKit Review Bot
no flags
Patch (6.09 KB, patch)
2012-07-11 04:27 PDT, Arpita Bahuguna
no flags
Archive of layout-test-results from gce-cr-linux-08 (463.91 KB, application/zip)
2012-07-11 05:03 PDT, WebKit Review Bot
no flags
Patch (6.13 KB, patch)
2012-07-12 05:44 PDT, Arpita Bahuguna
no flags
Patch (4.09 KB, patch)
2013-02-07 20:51 PST, Kent Tamura
no flags
Patch (4.18 KB, patch)
2013-02-07 21:16 PST, Kent Tamura
no flags
Arpita Bahuguna
Comment 1 2012-06-25 00:32:37 PDT
Hajime Morrita
Comment 2 2012-06-25 20:35:40 PDT
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.
Kent Tamura
Comment 3 2012-06-25 20:47:48 PDT
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.
Alexey Proskuryakov
Comment 4 2012-06-26 20:57:51 PDT
> > 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.
Arpita Bahuguna
Comment 5 2012-06-26 23:52:37 PDT
(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.
Kent Tamura
Comment 6 2012-06-26 23:56:12 PDT
(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.
Arpita Bahuguna
Comment 7 2012-06-27 01:58:06 PDT
(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.
Hajime Morrita
Comment 8 2012-06-27 02:07:19 PDT
(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!
Kent Tamura
Comment 9 2012-06-27 02:29:39 PDT
Created attachment 149715 [details] Another repro
Kent Tamura
Comment 10 2012-06-27 02:38:09 PDT
(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.
Arpita Bahuguna
Comment 11 2012-06-27 03:22:44 PDT
(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.
Arpita Bahuguna
Comment 12 2012-06-27 04:38:20 PDT
WebKit Review Bot
Comment 13 2012-06-27 07:00:45 PDT
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
WebKit Review Bot
Comment 14 2012-06-27 07:00:50 PDT
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
Kent Tamura
Comment 15 2012-06-28 00:11:37 PDT
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()?
Arpita Bahuguna
Comment 16 2012-06-29 00:28:01 PDT
(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.
Arpita Bahuguna
Comment 17 2012-07-11 04:27:08 PDT
Arpita Bahuguna
Comment 18 2012-07-11 04:44:17 PDT
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.
WebKit Review Bot
Comment 19 2012-07-11 05:02:57 PDT
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
WebKit Review Bot
Comment 20 2012-07-11 05:03:02 PDT
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
Arpita Bahuguna
Comment 21 2012-07-12 05:44:27 PDT
Ryosuke Niwa
Comment 22 2012-07-13 14:59:08 PDT
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.
Arpita Bahuguna
Comment 23 2012-07-16 01:38:13 PDT
(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.
Kent Tamura
Comment 24 2013-02-07 20:51:52 PST
Hajime Morrita
Comment 25 2013-02-07 21:05:59 PST
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.
Kent Tamura
Comment 26 2013-02-07 21:08:09 PST
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.
Kent Tamura
Comment 27 2013-02-07 21:16:56 PST
Created attachment 187230 [details] Patch Vefiry activeElement=body
Hajime Morrita
Comment 28 2013-02-07 21:50:07 PST
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().
Kent Tamura
Comment 29 2013-02-07 21:56:14 PST
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.
Hajime Morrita
Comment 30 2013-02-07 22:24:06 PST
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.
WebKit Review Bot
Comment 31 2013-02-07 23:19:42 PST
Comment on attachment 187230 [details] Patch Clearing flags on attachment: 187230 Committed r142234: <http://trac.webkit.org/changeset/142234>
WebKit Review Bot
Comment 32 2013-02-07 23:19:48 PST
All reviewed patches have been landed. Closing bug.
Ojan Vafai
Comment 33 2013-02-16 10:58:21 PST
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.
Kent Tamura
Comment 34 2013-03-17 15:51:00 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.