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.
Created attachment 95163 [details] Add a guard to document.activeElement
Please don't forget to cc peeps who should review! I didn't even realize this was here.
Comment on attachment 95163 [details] Add a guard to document.activeElement actually.... shouldn't we be doing this in HTMLDocument::focusedNode?
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?
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.
Created attachment 96380 [details] Patch
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.
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?
Created attachment 96403 [details] Patch
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.
Comment on attachment 96403 [details] Patch Now it got good coverage! Let's land this.
Comment on attachment 96403 [details] Patch Clearing flags on attachment: 96403 Committed r88418: <http://trac.webkit.org/changeset/88418>
All reviewed patches have been landed. Closing bug.