WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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.
Top of Page
Format For Printing
XML
Clone This Bug