Summary: | blur() on shadow host should work when a shadow host contains a focused element in its shadow DOM subtrees. | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hayato Ito <hayato> | ||||||||||||||||||||||||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, dominicc, kaustubh.ra, morrita, rolandsteiner, shinyak, webkit.review.bot | ||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||
Bug Blocks: | 63606 | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Hayato Ito
2012-03-14 06:58:26 PDT
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. Created attachment 132048 [details]
Patch
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(...); ... 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 Created attachment 132226 [details]
Patch
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. 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.
Looks good to me. Thank you for fixing. You need r+ from reviewers. 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! Created attachment 132243 [details]
Updated Patch
Reorganized the code to make it systematic.
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.
Created attachment 132246 [details]
Style Fix
Style issue fixed. ChangeLog updated.
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... Comment on attachment 132246 [details]
Style Fix
Oops the patch was crossing the comment. Please check the comment on previous patch.
(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). Created attachment 132250 [details]
Updated Patch
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. 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." Created attachment 132257 [details]
Updated Patch
Added ASSERT to make sure node == this check has been done by caller site.
Could you share code between Element::blur() and TreeScope::activeElement()? Very rough idea is: Element::blue() { if (treeScope()->activeElement() == this) { doBlur(); ... } Created attachment 132259 [details]
Updated Patch
Created attachment 132261 [details]
Updated Patch
Code sharing made it simple :)
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? (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. Created attachment 132277 [details]
Patch
Patch with TreeScope::activElement splited
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. Created attachment 132552 [details]
Updated Patch
Updated as per comments from Hayato. Can u review once?
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. Created attachment 132554 [details]
Fixed ChangeLog
Aah..missed that :) Thanks.
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. Created attachment 132566 [details]
Updated Patch
Updated patch.
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. (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(). 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. 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(). (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. (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. 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. (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(). Created attachment 133012 [details]
Patch
Updated patch as per discussion.
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 Created attachment 133209 [details]
Updated ChangeLog
Created attachment 133218 [details]
Fixed ChangeLog
Comment on attachment 133218 [details] Fixed ChangeLog Clearing flags on attachment: 133218 Committed r111672: <http://trac.webkit.org/changeset/111672> All reviewed patches have been landed. Closing bug. |