Bug 81102 – blur() on shadow host should work when a shadow host contains a focused element in its shadow DOM subtrees.

• Component: DOM
• Status: RESOLVED FIXED

[Hayato Ito]

The example is here:

var shadowHost = document.createElement("div");
document.body.appendChild(shadowHost);
var shadowRoot = new WebKitShadowRoot(shadowHost);

var childInShadowRoot = document.createElement("p");
childInShadowRoot.tabIndex = "1";
shadowRoot.appendChild(childInShadowRoot);

shouldBe("childInShadowRoot.focus();document.activeElement", "shadowHost");
shouldBe("shadowHost.blur();document.activeElement", "document.body");  // (A)

(A) fails. The current implementation does not blur a focused element when its shadowHost's blur() is called().

[Kaustubh Atrawalkar]

This need to be fixed in Element::blur function to support blurring of ShadowTree children when blur is called on ShadowHost(). Will provide fix soon.

[Kaustubh Atrawalkar]

Created attachment 132048 [details]
Patch

[Hayato Ito]

Comment on attachment 132048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132048&action=review

> Source/WebCore/dom/Element.cpp:1600
> +    } else if (hasShadowRoot()) {

You can make this function more simple, avoiding duplication of duplication L1616-1619.
Element::blur could be:

void Element::blur()
{
  ...
  Node* focused = doc->focusedNode();
  ...
  if (hasShadowRoot) {
    ...
    ..
    // Omit L1616-1619
  }

  if (focused != this)
   return;

  if (doc->frame())
      doc->frame()->page()->focusController()->setFocusedNode(0, doc->frame());
  else
      doc->setFocusedNode(0);
}

> LayoutTests/fast/dom/shadow/shadow-root-blur-expected.txt:1
> +This tests the activeElement property of a ShadowRoot.

Need to update.

> LayoutTests/fast/dom/shadow/shadow-root-blur.html:31
> +// First ShodowHost

It's optional, but you might want to use some functions on LayoutTests/fast/dom/shadow/resources/shadow-dom.js.
Using createDOM and createShadowRoot, you don't have to construct Shadow DOM tree manually and make tree in one expression.

That helps reviewers to understand the tree structure easily and you don't have to add comments which only explain the tree structure.

> LayoutTests/fast/dom/shadow/shadow-root-blur.html:73
> +evalAndLog("shadowHostInside.blur();");

Could you add the following test case?

shouldBe("childInTreeTwo.focus();document.activeElement", "shadowHost");
evalAndLog("shadowHost.blur();");
shouldBe(...);
shouldBe(...);
...

[Hayato Ito]

Could you verify the 'blur' event is called on ShadowHost?
I guess it should work, but we need to make sure by a test case.


(In reply to comment #2)
> Created an attachment (id=132048) [details]
> Patch

[Kaustubh Atrawalkar]

Created attachment 132226 [details]
Patch

[Hayato Ito]

Comment on attachment 132226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132226&action=review

> Source/WebCore/dom/Element.cpp:1608
> +    // if shadowHost is not found return. This handles case where focused node

Could you modify this comment?
I think we can remove a comment of 'if shadowHost is not found return.'  since it does not explain the exact situation here.

> LayoutTests/fast/dom/shadow/shadow-root-blur.html:41
> +shouldBe("getElementInShadowTreeStack('shadowHostA/').activeElement", "null");

We should rename getElementInShadowTreeStack since it can return ShadowRoot, which is not Element.
That is an unintentional usage for me that it can return ShadowRoot. So we should rename it to getNodeInShadowTreeStack.

Could you update shadow-dom.js and other files which use getElementInShadowTreeStack?
It is okay that you will do it in another patch. If you do it in another patch, please add 'FIXME: Rename ...' comment on getElemetnInShadowTreeStack in shadow-dom.js.

[Kaustubh Atrawalkar]

Created attachment 132233 [details]
Updated Patch

Thanks Hayato for the review. I have updated the comment and added FIXME. Will do it in the next patch. I will file a bug soon.

[Hayato Ito]

Looks good to me. Thank you for fixing.
You need r+ from reviewers.

[Hajime Morrita]

Comment on attachment 132233 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132233&action=review

The approach seems sane. Could you reorganize code a bit?

> Source/WebCore/dom/Element.cpp:1599
> +    if (hasShadowRoot()) {

It looks TreeScope or ShadowRoot should have focusNode(). 
In general, it is good practice to separate finding something and modifying something.
In this case, the function can be splited into two part:
- Checking if the focus is in the shadow tree or myself, and
- unsetting the current focus.

> LayoutTests/fast/dom/shadow/shadow-root-blur.html:1
> +<!DOCTYPE html>

This test looks great!

[Kaustubh Atrawalkar]

Created attachment 132243 [details]
Updated Patch

Reorganized the code to make it systematic.

[WebKit Review Bot]

Attachment 132243 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/dom/Element.cpp:1596:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kaustubh Atrawalkar]

Created attachment 132246 [details]
Style Fix

Style issue fixed. ChangeLog updated.

[Hajime Morrita]

Comment on attachment 132243 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132243&action=review

> Source/WebCore/dom/Element.cpp:1592
> +{

I find this method confusing. What are we are really looking for here?
Maybe what we want have is Element::nodeIsInShadowTree(Node*) or TreeScope::contains(Node*) or something.
I'm sorry that my original suggestion was wrong. But it's hard to explain...

[Hajime Morrita]

Comment on attachment 132246 [details]
Style Fix

Oops the patch was crossing the comment. Please check the comment on previous patch.

[Kaustubh Atrawalkar]

(In reply to comment #14)
> (From update of attachment 132246 [details])
> Oops the patch was crossing the comment. Please check the comment on previous patch.

No problem. I check the comments on the earlier patch. What we looking for here is if the element on which blur is called and the element which is focused are in the same treeScope for ShadowTree. I think nodeIsInShadowTree sounds a good function. This will check if the same, if focused node is in the same treeScope of the element (which calls blur).

[Kaustubh Atrawalkar]

Created attachment 132250 [details]
Updated Patch

[Hajime Morrita]

Comment on attachment 132250 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132250&action=review

Almost good. Could you take care of small points?

> Source/WebCore/dom/Element.cpp:1591
> +bool Element::nodeIsInShadowTree(Node* focused)

|focused| looks too specific name.
This method doesn't need to mention about focus.

> Source/WebCore/dom/Element.cpp:1594
> +    if (hasShadowRoot()) {

Let's return here.
WebKit prefers early return style.

> Source/WebCore/dom/Element.cpp:1613
> +    Node* focusedNode = doc->focusedNode();

Yeah, this looks making more sense.

[Hajime Morrita]

Comment on attachment 132250 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132250&action=review

>> Source/WebCore/dom/Element.cpp:1591
>> +bool Element::nodeIsInShadowTree(Node* focused)
> 
> |focused| looks too specific name.
> This method doesn't need to mention about focus.

Also, it looks we can ASSERT(focused != this) because the check is done by the caller site.
It will clarify the point because this function check if the "node is in shadow tree."

[Kaustubh Atrawalkar]

Created attachment 132257 [details]
Updated Patch

Added ASSERT to make sure node == this check has been done by caller site.

[Hayato Ito]

Could you share code between Element::blur() and TreeScope::activeElement()?

Very rough idea is:

Element::blue()
{
  if (treeScope()->activeElement() == this) {
    doBlur();
  ...
}

[Kaustubh Atrawalkar]

Created attachment 132259 [details]
Updated Patch

[Kaustubh Atrawalkar]

Created attachment 132261 [details]
Updated Patch

Code sharing made it simple :)

[Hayato Ito]

Comment on attachment 132261 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132261&action=review

> Source/WebCore/dom/Element.cpp:1595
> +    if (treeScope()->activeElement() == this) {

You can not use the result of activeElement() 'as is' since it may return document.body if there is no focused node.

Could you check whether we can call document.body.blur() and it work correctly or not?

[Kaustubh Atrawalkar]

(In reply to comment #23)
> (From update of attachment 132261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132261&action=review
> 
> > Source/WebCore/dom/Element.cpp:1595
> > +    if (treeScope()->activeElement() == this) {
> 
> You can not use the result of activeElement() 'as is' since it may return document.body if there is no focused node.
> 
> Could you check whether we can call document.body.blur() and it work correctly or not?

document.body.blur() does not give any error. but body.blur will result in nothing as body's focus can not be unset. Tried that without the change in blur(). 
Steps -
Focus an element. 
call blur on body. 
Focus still remains on the element.

[Kaustubh Atrawalkar]

Created attachment 132277 [details]
Patch

Patch with TreeScope::activElement splited

[Hayato Ito]

Comment on attachment 132277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132277&action=review

> Source/WebCore/dom/TreeScope.cpp:190
> +    Document* document = rootNode()->document();

Could you move this line to inside of 'if' statement (L192)? Since 'document' is used only in the if statement.

> Source/WebCore/dom/TreeScope.h:50
> +    Node* focusedNodeInTree();

I don't think focusedNodeInTree() is intuitive name because the meaning of 'Tree' here is not clear.
focusedNodeInTreeScope() might be better. Or focusedNode() is simply enough in this context.

[Kaustubh Atrawalkar]

Created attachment 132552 [details]
Updated Patch

Updated as per comments from Hayato. Can u review once?

[Hayato Ito]

Comment on attachment 132552 [details]
Updated Patch

Looks good to me. Thank you.

View in context: https://bugs.webkit.org/attachment.cgi?id=132552&action=review

> Source/WebCore/ChangeLog:13
> +        (WebCore::TreeScope::focusedNodeInTree):

You need to update ChageLog.

[Kaustubh Atrawalkar]

Created attachment 132554 [details]
Fixed ChangeLog

Aah..missed that :) Thanks.

[Hajime Morrita]

Comment on attachment 132554 [details]
Fixed ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=132554&action=review

This looks a good direction! Thanks for clarification. Could you iterate once more?

> Source/WebCore/dom/TreeScope.cpp:188
> +Element* TreeScope::activeElement()

We can make activeElement() virtual. then we know if this is document more clearly.

[Kaustubh Atrawalkar]

Created attachment 132566 [details]
Updated Patch

Updated patch.

[Hajime Morrita]

Comment on attachment 132566 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132566&action=review

I'm confused. Apparently the previous patch is much simpler, no?

> Source/WebCore/dom/TreeScope.cpp:172
> +        return this == document ? document->body() : 0;

Why do we have this check even after introducing the virtual method?
Is it for eliminating this, right?

> Source/WebCore/dom/TreeScope.h:51
> +    virtual Element* activeElement();

It looks nobody inherits this.

[Kaustubh Atrawalkar]

(In reply to comment #32)
> (From update of attachment 132566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132566&action=review
> 
> I'm confused. Apparently the previous patch is much simpler, no?
> 
> > Source/WebCore/dom/TreeScope.cpp:172
> > +        return this == document ? document->body() : 0;
> 
> Why do we have this check even after introducing the virtual method?
> Is it for eliminating this, right?
> 
> > Source/WebCore/dom/TreeScope.h:51
> > +    virtual Element* activeElement();
> 
> It looks nobody inherits this.

Yes, both ShadowRoot & HTMLDocument shares the same code in TreeScope::activeElement().

[Hajime Morrita]

Comment on attachment 132554 [details]
Fixed ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=132554&action=review

> Source/WebCore/dom/TreeScope.cpp:193
> +        return this == document ? document->body() : 0;

If you have override this as Document::activeElement(), you can do this check there instead of here.
The point is that Document is a subclass of TreeScope and we can take this logic as a specialization for Document.

[Hajime Morrita]

Comment on attachment 132566 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132566&action=review

>>> Source/WebCore/dom/TreeScope.cpp:172
>>> +        return this == document ? document->body() : 0;
>> 
>> Why do we have this check even after introducing the virtual method?
>> Is it for eliminating this, right?
> 
> Yes, both ShadowRoot & HTMLDocument shares the same code in TreeScope::activeElement().

Then the document specific behavior should be in Document::activeElement(), not TreeScope::activeElement().

[Kaustubh Atrawalkar]

(In reply to comment #35)
> (From update of attachment 132566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132566&action=review
> 
> >>> Source/WebCore/dom/TreeScope.cpp:172
> >>> +        return this == document ? document->body() : 0;
> >> 
> >> Why do we have this check even after introducing the virtual method?
> >> Is it for eliminating this, right?
> > 
> > Yes, both ShadowRoot & HTMLDocument shares the same code in TreeScope::activeElement().
> 
> Then the document specific behavior should be in Document::activeElement(), not TreeScope::activeElement().

The reason I have this code in TreeScope is, activeElement property is for HTMLDocument class and not Document class. And ShadowRoot is derived from TreeScope. So the common base class is TreeScope.

[Hajime Morrita]

(In reply to comment #36)
> (In reply to comment #35)
> > (From update of attachment 132566 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=132566&action=review
> > 
> > >>> Source/WebCore/dom/TreeScope.cpp:172
> > >>> +        return this == document ? document->body() : 0;
> > >> 
> > >> Why do we have this check even after introducing the virtual method?
> > >> Is it for eliminating this, right?
> > > 
> > > Yes, both ShadowRoot & HTMLDocument shares the same code in TreeScope::activeElement().
> > 
> > Then the document specific behavior should be in Document::activeElement(), not TreeScope::activeElement().
> 

> The reason I have this code in TreeScope is, activeElement property is for HTMLDocument class and not Document class. And ShadowRoot is derived from TreeScope. So the common base class is TreeScope.
It's fine to have activeElement in Document. HTMLDocument.
Then you can override it in HTMLDocument. Superclass shouldn't know about its subclass if possible.
That's what runtime polymorphism (virtual) is designed for.

[Hajime Morrita]

I think I got your point. This comment is for the second last patch:
We can just have two separate activeElement() implementation.
one is for ShadowRoot and another is for Document (or HTMLDocument.)
Since the hard part is done in focusedNode(), such duplication will be fine.

[Kaustubh Atrawalkar]

(In reply to comment #38)
> I think I got your point. This comment is for the second last patch:
> We can just have two separate activeElement() implementation.
> one is for ShadowRoot and another is for Document (or HTMLDocument.)
> Since the hard part is done in focusedNode(), such duplication will be fine.

Ok, so to brief here is what it will be - Move HTMLDocument::activeElement() imlpementation to Document so that it can share the code between blur()
 & ShadowRoot will have separate activeElement implementation. Remove treeScope::activeElement().

[Kaustubh Atrawalkar]

Created attachment 133012 [details]
Patch

Updated patch as per discussion.

[Hajime Morrita]

Comment on attachment 133012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133012&action=review

Code looks good! what we only miss is changelog explanation.
Thanks to exercise this long iteration.

> Source/WebCore/ChangeLog:7
> +

Could you have some explanation here?
It's recent trend: http://markmail.org/message/aov6bcu7he4tbyoy

[Kaustubh Atrawalkar]

Created attachment 133209 [details]
Updated ChangeLog

[Kaustubh Atrawalkar]

Created attachment 133218 [details]
Fixed ChangeLog

[WebKit Review Bot]

Comment on attachment 133218 [details]
Fixed ChangeLog

Clearing flags on attachment: 133218

Committed r111672: <http://trac.webkit.org/changeset/111672>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.