Bug 86707

Summary:

document.activeElement should not return a non-focusable element

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

DOM

Assignee:

Kent Tamura <tkent>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, arpitabahuguna, arv, dglazkov, eric, kling, ksakamoto, morrita, ojan.autocc, simon.fraser, tasak, vijayan.bits, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
repro	none
Patch	none
Another repro	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-01	none
Patch	none
Archive of layout-test-results from gce-cr-linux-08	none
Patch	none
Patch	none
Patch	none

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	Details
Patch (4.44 KB, patch) 2012-06-25 00:32 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Another repro (529 bytes, text/html) 2012-06-27 02:29 PDT, Kent Tamura	no flags	Details
Patch (4.25 KB, patch) 2012-06-27 04:38 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
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	Details
Patch (6.09 KB, patch) 2012-07-11 04:27 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
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	Details
Patch (6.13 KB, patch) 2012-07-12 05:44 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (4.09 KB, patch) 2013-02-07 20:51 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch (4.18 KB, patch) 2013-02-07 21:16 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Arpita Bahuguna

Comment 1 2012-06-25 00:32:37 PDT

Created attachment 149258 [details] Patch

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

Created attachment 149727 [details] Patch

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

Created attachment 151677 [details] Patch

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

Created attachment 151923 [details] Patch

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

Created attachment 187228 [details] Patch

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.