Bug 79886

Created attachment 129438 [details] Patch Hi Morrita, I have implemented activeElement attribute for the ShadowRoot. Please check the attached patch.

Comment on attachment 129438 [details] Patch Thanks for taking this. But it's apparently too naive. Please see HTMLDocument::activeElement() and consider sharing the logic.

(In reply to comment #2) > (From update of attachment 129438 [details]) > Thanks for taking this. But it's apparently too naive. > Please see HTMLDocument::activeElement() and consider sharing the logic. Hi Morrita, thanks for the review. I was thinking to move this HTMLDocument::activeElement to Document and share it between both HTMLDocument & ShadowRoot. But I saw there is specific fix being done in HTMLDocument::activeElement to make sure that document.activeElement won't be an element in shadow root. So if we want to share the logic, we might need to revert the logic. In my patch, I was assuming shadowRoot.activeElement will return active element only from the current shadowDOM tree as specified in the spec. The spec says "To maintain upper-boundary encapsulation, the value of the Document object's focus API property activeElement must be adjusted. To prevent loss of information when adjusting this value, each shadow root must also have an activeElement property to store the value of the focused element in the shadow DOM subtree." So the document.activeElement will return shadowHost as expected but shadowRoot.activeElement will return "div1" (where div1 is focused and is child of shadowRoot) Please correct me if wrong.

The point is that It must be avoided to traverse all nodes to find a focusedNode. That takes O(N). We can find a shadowRoot.activeElement in O(nest level of TreeScope), I think.

Created attachment 130076 [details] Updated Patch I have updated the patch and test case according as per our communication over IRC. Please review. Thanks.

Comment on attachment 130076 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130076&action=review > Source/WebCore/dom/TreeScope.cpp:177 > + while (node->treeScope() != document) { This looks wrong for me. This loop should be: while (node->treeScope() != this) isn't it? In your patch, shadowRoot.activeElement returns the same node of document.activeElement(). > LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:42 > +shouldBe("shadowRoot.activeElement", "shadowHost"); I am afraid you misunderstand the spec. In this case, shadowRoot.activeElement should be 'p1', not shadowHost.

(In reply to comment #6) > I am afraid you misunderstand the spec. In this case, shadowRoot.activeElement should be 'p1', not shadowHost. yes, seems so. My earlier patch was targeting for the same. But I guess i got confused in the wordings of the spec.

Comment on attachment 130076 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130076&action=review > Source/WebCore/dom/TreeScope.cpp:167 > +Element* TreeScope::getActiveElement() Please don't prefix this function's name by "get". The prefix "get" should be used for only functions that return values via out arguments. > Source/WebCore/dom/TreeScope.cpp:178 > + node = node->parentOrHostNode(); This is very inefficient way to walking up the tree. You should be able to skip all nodes between two tree scopes. e.g. Node* treeScope; for (; node; node = treeScope->shadowHost()) { treeScope = node->treeScope(); if (treeScope == this) return node; } > Source/WebCore/dom/TreeScope.cpp:183 > + return document->body(); I don't think we should always be returning body element here.

Created attachment 130610 [details] Updated Patch Updated patch and test case as per comments. Please review once. Thanks.

Created attachment 130612 [details] Updated Patch Updated patch with optimization of walking up the tree and fixed test case.

Comment on attachment 130612 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130612&action=review > Source/WebCore/dom/ShadowRoot.cpp:162 > + if (this->document()->isHTMLDocument()) Why do we need this->? > Source/WebCore/dom/TreeScope.cpp:177 > + Node* treeScope = node->treeScope()->rootNode(); Why are you calling the root node a tree scope? That makes very little sense. r- because of this. > Source/WebCore/dom/TreeScope.cpp:178 > + while (treeScope != this->rootNode()) { Why do we need this->? Also, you can just use m_rootNode instead. > Source/WebCore/dom/TreeScope.cpp:181 > + if (treeScope == document) > + break; You can merge this condition with treeScope != this->rootNode(). > LayoutTests/fast/dom/shadow/shadow-root-activeElement-expected.txt:9 > +p1 is focused Please don't use two-letter variables like p1. You should give a descriptive name (e.g. firstElementInShadowDOM). > LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:20 > +shadowRoot = new WebKitShadowRoot(shadowHost); We definitely need a test case where we have a shadow DOM inside another shadow DOM. > LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:32 > +debug("Check if activeElement attribute is defined"); This debug output isn't useful at all. It repeats what the code says. > LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:37 > +debug("p1 is focused"); > +p1.focus(); > +shouldBe("shadowRoot.activeElement", "p1"); Instead of saying that p1 is focused, it's much more clear to just do: shouldBe("p1.focus();shadowRoot.activeElement", "p1"); (of course, after giving p1 a more descriptive name).

Created attachment 130803 [details] Updated Patch Update patch as per comments. Added test cases of nested ShadowDOM tree.

Comment on attachment 130803 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130803&action=review > Source/WebCore/dom/ShadowRoot.cpp:165 > +Element* ShadowRoot::activeElement() const > +{ > + if (document()->isHTMLDocument()) > + return treeScope()->activeElement(); > + return 0; > +} I think it'll be better to make this function inline so that we don't introduce a new function call in Document::activeElement. > Source/WebCore/dom/TreeScope.cpp:179 > + // If treeScope becomes document, document->shadowHost() fails. As toElement(0) throws ASSERT. I think this comment is more confusing than helpful. Please remove.

Comment on attachment 130803 [details] Updated Patch Ugh... I meant cq-.

Created attachment 130995 [details] Fixed Nits

Comment on attachment 130995 [details] Fixed Nits Clearing flags on attachment: 130995 Committed r110310: <http://trac.webkit.org/changeset/110310>

All reviewed patches have been landed. Closing bug.

ASSERTION FAILED: node->document() == this third_party/WebKit/Source/WebCore/dom/TreeScope.cpp(176) : WebCore::Element* WebCore::TreeScope::activeElement()

(In reply to comment #18) > ASSERTION FAILED: node->document() == this > third_party/WebKit/Source/WebCore/dom/TreeScope.cpp(176) : WebCore::Element* WebCore::TreeScope::activeElement() Oops, sorry about missing that in the review :( Fixed in http://trac.webkit.org/changeset/110321.