RESOLVED FIXED 61413
Make document.activeElement work with shadow content. 
https://bugs.webkit.org/show_bug.cgi?id=61413
Summary Make document.activeElement work with shadow content.
Hayato Ito
Reported 2011-05-24 19:29:20 PDT
We should return a right element when 'document.activeElement' is used in JavaScript if some elements in shadow content is currently focused. I think we should return a shadow host element in that case. We should make sure what will be returned.
Attachments
Add a guard to document.activeElement (5.27 KB, patch)
2011-05-27 06:03 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Patch (5.46 KB, patch)
2011-06-07 22:14 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Patch (10.54 KB, patch)
2011-06-08 03:57 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hayato Ito
Comment 1 2011-05-27 06:03:06 PDT
Created attachment 95163 [details] Add a guard to document.activeElement
Dimitri Glazkov (Google)
Comment 2 2011-05-27 14:27:14 PDT
Please don't forget to cc peeps who should review! I didn't even realize this was here.
Dimitri Glazkov (Google)
Comment 3 2011-05-27 14:29:55 PDT
Comment on attachment 95163 [details] Add a guard to document.activeElement actually.... shouldn't we be doing this in HTMLDocument::focusedNode?
Hayato Ito
Comment 4 2011-05-29 22:32:41 PDT
Thank you for the review. Yes, it's nice that document.activeElement should be the same to the return value of HTMLDocument::focusedNode(). Its on my rader, but there remains things to be solved to achieve that. I explained the current status and my rough idea in bug 61532 (https://bugs.webkit.org/show_bug.cgi?id=61532#c1). That might expain why we cannot simply modify Document::focusedNode() in this case. I'd appreciate if you could see that and give me your thoughts. (In reply to comment #3) > (From update of attachment 95163 [details]) > actually.... shouldn't we be doing this in HTMLDocument::focusedNode?
Hajime Morrita
Comment 5 2011-06-06 19:26:30 PDT
Comment on attachment 95163 [details] Add a guard to document.activeElement View in context: https://bugs.webkit.org/attachment.cgi?id=95163&action=review > Source/WebCore/html/HTMLDocument.cpp:142 > + if (!node) { Could you split out this loop logic into a static function or Frame method? nested if-if-for-if looks complicated enough. > Source/WebCore/html/HTMLDocument.cpp:152 > + ASSERT(!node || node->document() == this); I'd like to return early here when node is null.
Hayato Ito
Comment 6 2011-06-07 22:14:52 PDT
Created attachment 96380 [details] Patch
Hayato Ito
Comment 7 2011-06-07 22:15:20 PDT
Comment on attachment 95163 [details] Add a guard to document.activeElement View in context: https://bugs.webkit.org/attachment.cgi?id=95163&action=review >> Source/WebCore/html/HTMLDocument.cpp:142 >> + if (!node) { > > Could you split out this loop logic into a static function or Frame method? nested if-if-for-if looks complicated enough. Sure. I splited it out to a static function. >> Source/WebCore/html/HTMLDocument.cpp:152 >> + ASSERT(!node || node->document() == this); > > I'd like to return early here when node is null. Done.
Hajime Morrita
Comment 8 2011-06-07 23:41:30 PDT
Comment on attachment 96380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96380&action=review Code looks good! Only thing left is paying a duty... > LayoutTests/fast/dom/shadow/activeelement-should-be-shadowhost-expected.txt:8 > + I'd like to have more test cases that - returns <body> - when the focus is inside an iframe - when the focus is inside a nested iframe - call activeElement() of detached document - call activeElement() of iframed-document - with several focus variations. These should be done by original authors. But I bet it isn't... > Source/WebCore/html/HTMLDocument.cpp:159 > + return toElement(node); Can |node| be null?
Hayato Ito
Comment 9 2011-06-08 03:57:25 PDT
Created attachment 96403 [details] Patch
Hayato Ito
Comment 10 2011-06-08 03:58:00 PDT
Comment on attachment 96380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96380&action=review >> LayoutTests/fast/dom/shadow/activeelement-should-be-shadowhost-expected.txt:8 >> + > > I'd like to have more test cases that > - returns <body> > - when the focus is inside an iframe > - when the focus is inside a nested iframe > - call activeElement() of detached document > - call activeElement() of iframed-document > - with several focus variations. > These should be done by original authors. But I bet it isn't... The most cases are already covered in fast/dom/HTMLDocument/active-element-frames.html. I agree that we need more coverage which involves shadow, so I rewrote the test, adding the following cases: - shadow in iframe - shadow in shadow - iframe in shadow >> Source/WebCore/html/HTMLDocument.cpp:159 >> + return toElement(node); > > Can |node| be null? Yeah, that cannot be null. Before |node| becomes null, we surely reach a node whose treescope is same to this document. I removed the null check and added assertion instead.
Hajime Morrita
Comment 11 2011-06-08 20:17:27 PDT
Comment on attachment 96403 [details] Patch Now it got good coverage! Let's land this.
WebKit Review Bot
Comment 12 2011-06-08 20:55:29 PDT
Comment on attachment 96403 [details] Patch Clearing flags on attachment: 96403 Committed r88418: <http://trac.webkit.org/changeset/88418>
WebKit Review Bot
Comment 13 2011-06-08 20:55:34 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.