ShadowRoot should have activeElement

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#api-shadow-root-active-element

[Kaustubh Atrawalkar]

Created attachment 129438 [details]
Patch

Hi Morrita, I have implemented activeElement attribute for the ShadowRoot. Please check the attached patch.

[Hajime Morrita]

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.

[Kaustubh Atrawalkar]

(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.

[Hayato Ito]

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.

[Kaustubh Atrawalkar]

Created attachment 130076 [details]
Updated Patch

I have updated the patch and test case according as per our communication over IRC. Please review. Thanks.

[Hayato Ito]

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.

[Kaustubh Atrawalkar]

(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.

[Ryosuke Niwa]

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.

[Kaustubh Atrawalkar]

Created attachment 130610 [details]
Updated Patch

Updated patch and test case as per comments. Please review once. Thanks.

[Kaustubh Atrawalkar]

Created attachment 130612 [details]
Updated Patch

Updated patch with optimization of walking up the tree and fixed test case.

[Ryosuke Niwa]

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).

[Kaustubh Atrawalkar]

Created attachment 130803 [details]
Updated Patch

Update patch as per comments. Added test cases of nested ShadowDOM tree.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 130803 [details]
Updated Patch

Ugh... I meant cq-.

[Kaustubh Atrawalkar]

Created attachment 130995 [details]
Fixed Nits

[WebKit Review Bot]

Comment on attachment 130995 [details]
Fixed Nits

Clearing flags on attachment: 130995

Committed r110310: <http://trac.webkit.org/changeset/110310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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

[Ryosuke Niwa]

(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.