|
|
Created:
4 years, 10 months ago by yuzuchan Modified:
4 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, eae+blinkwatch, kalyank, qsr+mojo_chromium.org, rjkroege, rwlbuis, sadrul, sof, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConsider slots as a focus scope
V1 Slots can now have their own focus scope just like shadow roots, so that the assigned elements would be traversed together, which is visually more consistent than the current implementation. This reflects the results of the discussion in TPAC.
See the discussion here: https://github.com/w3c/webcomponents/issues/375
BUG=591909
Committed: https://crrev.com/856302237805d511bcc93ffc965b674406d7c80a
Cr-Commit-Position: refs/heads/master@{#379523}
Patch Set 1 #Patch Set 2 : Checkout unrelated changes #Patch Set 3 : Refactoring #Patch Set 4 : Refactoring related to OILPAN #Patch Set 5 : Fix constructor usage #Patch Set 6 : Refactoring related to OILPAN #Patch Set 7 : Refactoring related to OILPAN #
Total comments: 1
Patch Set 8 : Implementing new traversal inside FocusController #
Total comments: 67
Patch Set 9 : Refactoring in response to review comments #
Total comments: 15
Patch Set 10 : Refactoring in response to review comments #Patch Set 11 : Refactoring in response to review comments #
Total comments: 53
Patch Set 12 : Refactoring related to OILPAN #Patch Set 13 : Refactoring related to OILPAN #Patch Set 14 : Refactoring in response to review comments #
Total comments: 2
Patch Set 15 : Debug for negative tabindex #Patch Set 16 : Refactoring in response to review comments #Patch Set 17 : Modify layout test #Patch Set 18 : Update moveToLast and LayoutTest result #Patch Set 19 : Checkout unnecessasry changes #Patch Set 20 : Rebase #Patch Set 21 : Adding document distribution #Patch Set 22 : Adding distribution recalc to document #Patch Set 23 : Minor changes #Patch Set 24 : Refactoring for loops #Patch Set 25 : Refactoring for loops #
Total comments: 2
Patch Set 26 : Adding initializations of member variables #Messages
Total messages: 32 (6 generated)
Description was changed from ========== Consider slots as a focus scope BUG= ========== to ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG= ==========
yuzus@chromium.org changed reviewers: + hayato@chromium.org, kochi@chromium.org, kojii@chromium.org
PTAL
Looks promising. Thank you! I'll review in detail soon, but at the first glance, we should use Element, instead of Node, in FocusController as well as your new XXXTraversal, as much as possible, because a focus navigation operates only on Element. Do not worry. It's not your fault. :) It's FocusController's fault. I've filed a bug for that: https://code.google.com/p/chromium/issues/detail?id=587743
On 2016/02/18 08:02:38, hayato wrote: > Looks promising. Thank you! > > I'll review in detail soon, but at the first glance, we should use Element, > instead of Node, in FocusController as well as your new XXXTraversal, as much as > possible, because a focus navigation operates only on Element. Do not worry. > It's not your fault. :) It's FocusController's fault. > > I've filed a bug for that: > https://code.google.com/p/chromium/issues/detail?id=587743 FYI - I don't remember the detail, but when I tried to convert Node to Element, there was some code path that you have to handle the case with ShadowRoot, which is not an Element (but ContainerNode). So that task is not a simple replacement from Node -> Element.
I've done "Node to Element" refactoring for FocusController in the following series of CLs: - https://codereview.chromium.org/1704333002/ - https://codereview.chromium.org/1711673002/ - https://codereview.chromium.org/1707373002/ You might want to rebase your CL after these CLs are landed. You no longer have to use |Node| nor |NodeTraversal|. You can use |Element| and |ElementTraversal| in your traversal function and avoid to use isTextNode(). .. and one more big thing is: It would be great to merge CombinedNodeTraversal and FocusNavigationScope into one class. It looks their functionalities are getting overlapping. Can it be possible? If I can say one more thing, I'd like to see: - You have a new class, say |ScopedFocusNavigation|. Any better name is welcome - Though the signature of the most of the functions in FocusController is something like: - (Before) findFocusableElementRecursivelyForward(const FocusNavigationScope& scope, Node* start) Yeah, this takes two parameters. However, it would be nice to make it something like: - (After) findFocusableElementRecursivelyForward(ScopedFocusNavigation& nav) That mean ScopedFocusNavigation will manage the start element itself so that we have better encapsulation. This class now has a responsibility: - Have an owner of a scope - Have an current element which is traversing - Provide next() and previous() functionality to the outside. Let's chat offline for details. https://codereview.chromium.org/1707443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedNodeTraversal.h (right): https://codereview.chromium.org/1707443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedNodeTraversal.h:1: /* Use 3 lines license comment for new files. See https://www.chromium.org/blink/coding-style#TOC-License
I'll look into this again once hayato's high-level comments are addressed.
PTAL
I'll take further look tomorrow. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core.gypi:2294: 'dom/AssignedElementTraversal.h', Maybe these files can be in dom/shadow ? (FlatTreeTraversal.* are there.) https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:23: // assignedAncestor returns an ancestor element of current which is directly assigned to a slot. Maybe isChildOfV1ShadowHost() is enough for this purpose (instead of using assignedSlot())? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:24: Element* element = const_cast<Element*>(¤t); Is const_cast<> really necessary? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:32: return element; This can be written as: for (Element* element = const_cast<Element*>(¤t); element; element = element->parentElement()) { if (element->assignedSlot()) break; } return element; https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:44: for (size_t i = 0; i < assignedNodes.size()-1; ++i) { Vector<> has find() method, though you should take care of the '-1' part yourself. nit: space around '-'. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:45: if (current == toElement(assignedNodes[i].get())) nit: is toElement() necessary here? (because this is comparing const Element& and Node* ?) https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:46: return toElement(assignedNodes[i+1].get()); nit: space around '+'. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:55: for (size_t i = 0; i < assignedNodes.size()-1; ++i) { This is almost same as line 44-. Probably you can factor out this as a function (or class method) as nextAssignedElement(slot, current)? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:68: } nit: if the body is one-liner, omit {}. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:71: for (size_t i = assignedNodes.size()-1; i > 0; --i) { Vector<> also has reverseFind() https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:87: return false; You can write this as return !!AssignedElementTraversal::assignedAncestor(current); https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedElementTraversal.h (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.h:11: namespace blink { It would be enough to insert forward declarations of class Element; class HTMLSlotElement; here (in namespace blink) and you can remove the 2 includes above. (then of course, you need to include them in .cpp file, but you already do :) ) note: forward declaration is preferred to #include to make the compile time shorter for those .cpp which includes this header. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:2278: return nullptr; Why this shortcut is necessary? isChildOfV1ShadowHost() is doing essentially line 2280-2281, so this looks redundant. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:78: void currentElement(Element*); setCurrentElement() https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:80: Element* previousElement(); Looking at current usage of these 2 methods, I'd be alluded to change the return type to void and rename to moveToNext()/moveToPrevious(). https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:84: static ScopedFocusNavigation focusNavigationScopeOf(const Element&, const Element*); If a function has 2 same types (or similar types), you need to have parameter name so they are distinguishable. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:88: static ScopedFocusNavigation ownedByHTMLSlotElement(HTMLSlotElement&); Is it possible to make this parameter const reference? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:158: } nit: both if- and else- clasue are one-liner, omit {}. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:214: return ScopedFocusNavigation::ownedByHTMLSlotElement(toHTMLSlotElement(element)); Weird indent? https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:344: current = scope.currentElement(); nit: You can remove line 342 and write this line as Element* current = scope.currentElement(); https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:358: current = scope.currentElement(); nit: You can remove line 356 and write this line as Element* current = scope.currentElement(); https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:375: current = scope.currentElement(); nit: You can remove line 373 and write this line as Element* current = scope.currentElement(); https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:412: scope.currentElement(scope.firstElement()); Doesn't scope.firstElement() set scope's m_current internally? I'd suggest rename the method to scope.moveToFirst(), or make firstElement() as const method and do not update m_current internally. In Blink's naming convention, noun-like method is usually just something, without side effects and verb-like method does something and may or may not return value. .firstElement() violates the convention and thus it is confusing. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:419: scope.currentElement(scope.firstElement()); Ditto https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:439: for (scope.previousElement(); scope.currentElement(); scope.previousElement()) { If the return value of the first statement (first scope.previousElement()) isn't used or assigned to any variable, please move it out of for (). https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:449: nit: an unnecessary empty line sneaked in. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:539: // Find focusable node in descendant scope. If not found, find next focusable node within the Do not revert back from element to node. A node is not focusable, an element is focusable. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:652: // change to my traversal???? Could you resolve this? :)
Thank you for the review kochi@ san! https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core.gypi:2294: 'dom/AssignedElementTraversal.h', On 2016/03/01 12:55:40, kochi wrote: > Maybe these files can be in dom/shadow ? > (FlatTreeTraversal.* are there.) Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/01 12:55:40, kochi wrote: > 2016? Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:23: // assignedAncestor returns an ancestor element of current which is directly assigned to a slot. On 2016/03/01 12:55:40, kochi wrote: > Maybe isChildOfV1ShadowHost() is enough for this purpose (instead of using > assignedSlot())? isChildOfV1ShadowHost() returns true even if the node is not assigned to any slot. Since this method returns an ancestor element which is assigned to a slot, isChildOfV1ShadowHost() is not enough here. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:24: Element* element = const_cast<Element*>(¤t); On 2016/03/01 12:55:40, kochi wrote: > Is const_cast<> really necessary? Yes, because I update element in the for loop below. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:32: return element; On 2016/03/01 12:55:40, kochi wrote: > This can be written as: > > for (Element* element = const_cast<Element*>(¤t); element; element = > element->parentElement()) { > if (element->assignedSlot()) > break; > } > return element; I kept Element* element = const_cast<Element*>(¤t); in the first line of this method, since I would not be able to return element if I put it in for loop initialization. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:44: for (size_t i = 0; i < assignedNodes.size()-1; ++i) { On 2016/03/01 12:55:40, kochi wrote: > Vector<> has find() method, though you should take care of the '-1' part > yourself. > nit: space around '-'. > Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:45: if (current == toElement(assignedNodes[i].get())) On 2016/03/01 12:55:40, kochi wrote: > nit: is toElement() necessary here? > > (because this is comparing const Element& and Node* ?) Yes exactly. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:46: return toElement(assignedNodes[i+1].get()); On 2016/03/01 12:55:40, kochi wrote: > nit: space around '+'. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:55: for (size_t i = 0; i < assignedNodes.size()-1; ++i) { On 2016/03/01 12:55:40, kochi wrote: > This is almost same as line 44-. > Probably you can factor out this as a function (or class method) as > nextAssignedElement(slot, current)? I refactored this part of code so that find method is written only once. PTAL of an updated version of this CL. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:68: } On 2016/03/01 12:55:40, kochi wrote: > nit: if the body is one-liner, omit {}. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:71: for (size_t i = assignedNodes.size()-1; i > 0; --i) { On 2016/03/01 12:55:40, kochi wrote: > Vector<> also has reverseFind() Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:87: return false; On 2016/03/01 12:55:40, kochi wrote: > You can write this as > return !!AssignedElementTraversal::assignedAncestor(current); Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedElementTraversal.h (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/01 12:55:40, kochi wrote: > 2016? Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.h:11: namespace blink { On 2016/03/01 12:55:40, kochi wrote: > It would be enough to insert forward declarations of > > class Element; > class HTMLSlotElement; > > here (in namespace blink) and you can remove the 2 includes above. > (then of course, you need to include them in .cpp file, but you already do :) ) > > note: forward declaration is preferred to #include to make the compile time > shorter for > those .cpp which includes this header. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:78: void currentElement(Element*); On 2016/03/01 12:55:41, kochi wrote: > setCurrentElement() Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:80: Element* previousElement(); On 2016/03/01 12:55:40, kochi wrote: > Looking at current usage of these 2 methods, I'd be alluded to change the return > type to > void and rename to moveToNext()/moveToPrevious(). Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:84: static ScopedFocusNavigation focusNavigationScopeOf(const Element&, const Element*); On 2016/03/01 12:55:41, kochi wrote: > If a function has 2 same types (or similar types), you need to have parameter > name so they are > distinguishable. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:84: static ScopedFocusNavigation focusNavigationScopeOf(const Element&, const Element*); On 2016/03/01 12:55:41, kochi wrote: > If a function has 2 same types (or similar types), you need to have parameter > name so they are > distinguishable. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:88: static ScopedFocusNavigation ownedByHTMLSlotElement(HTMLSlotElement&); On 2016/03/01 12:55:41, kochi wrote: > Is it possible to make this parameter const reference? Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:158: } On 2016/03/01 12:55:41, kochi wrote: > nit: both if- and else- clasue are one-liner, omit {}. hayato@ told me not to! https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:214: return ScopedFocusNavigation::ownedByHTMLSlotElement(toHTMLSlotElement(element)); On 2016/03/01 12:55:40, kochi wrote: > Weird indent? Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:344: current = scope.currentElement(); On 2016/03/01 12:55:41, kochi wrote: > nit: You can remove line 342 and write this line as Element* current = > scope.currentElement(); Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:358: current = scope.currentElement(); On 2016/03/01 12:55:41, kochi wrote: > nit: You can remove line 356 and write this line as Element* current = > scope.currentElement(); Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:375: current = scope.currentElement(); On 2016/03/01 12:55:41, kochi wrote: > nit: You can remove line 373 and write this line as Element* current = > scope.currentElement(); Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:412: scope.currentElement(scope.firstElement()); On 2016/03/01 12:55:41, kochi wrote: > Doesn't scope.firstElement() set scope's m_current internally? > > I'd suggest rename the method to scope.moveToFirst(), or > make firstElement() as const method and do not update m_current internally. > > In Blink's naming convention, noun-like method is usually just something, > without side effects > and verb-like method does something and may or may not return value. > .firstElement() violates the convention and thus it is confusing. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:419: scope.currentElement(scope.firstElement()); On 2016/03/01 12:55:41, kochi wrote: > Ditto Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:439: for (scope.previousElement(); scope.currentElement(); scope.previousElement()) { On 2016/03/01 12:55:41, kochi wrote: > If the return value of the first statement (first scope.previousElement()) isn't > used or assigned to any variable, > please move it out of for (). Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:449: On 2016/03/01 12:55:41, kochi wrote: > nit: an unnecessary empty line sneaked in. Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:652: // change to my traversal???? On 2016/03/01 12:55:41, kochi wrote: > Could you resolve this? :) Done.
https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:23: // assignedAncestor returns an ancestor element of current which is directly assigned to a slot. On 2016/03/02 09:02:14, yuzuchan wrote: > On 2016/03/01 12:55:40, kochi wrote: > > Maybe isChildOfV1ShadowHost() is enough for this purpose (instead of using > > assignedSlot())? > > isChildOfV1ShadowHost() returns true even if the node is not assigned to any > slot. > Since this method returns an ancestor element which is assigned to a slot, > isChildOfV1ShadowHost() is not enough here. Acknowledged. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:24: Element* element = const_cast<Element*>(¤t); On 2016/03/02 09:02:14, yuzuchan wrote: > On 2016/03/01 12:55:40, kochi wrote: > > Is const_cast<> really necessary? > > Yes, because I update element in the for loop below. That is not the reason. You can write const Element* element = ¤t; then you can update element below, like element = element->parentElement();. What you cannot update is the content of element. that said, you end up in returning element on line 32, and that line require const_cast<> anyway. So it doesn't make much difference in the end :( https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:32: return element; On 2016/03/02 09:02:14, yuzuchan wrote: > On 2016/03/01 12:55:40, kochi wrote: > > This can be written as: > > > > for (Element* element = const_cast<Element*>(¤t); element; element = > > element->parentElement()) { > > if (element->assignedSlot()) > > break; > > } > > return element; > > I kept Element* element = const_cast<Element*>(¤t); in the first line of > this method, since I would not be able to return element if I put it in for loop > initialization. I see your point... but Element* element = const_cast<Element*>(¤t); for (; element; element->parentElement()) { if (element->assignedSlot()) break; } return element; is shorter, isn't it? or for (Element* element = const_cast<Element*>(¤t); element; element = element->parentElement()) { if (element->assignedSlot()) return element; } return nullptr; (you already done the former!) https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:158: } On 2016/03/02 09:02:14, yuzuchan wrote: > On 2016/03/01 12:55:41, kochi wrote: > > nit: both if- and else- clasue are one-liner, omit {}. > > hayato@ told me not to! See https://www.chromium.org/blink/coding-style#TOC-Braces You can keep the braces, but you can remove. The rule is that keep it consistent for both if- and else-. I don't see strong reason not to remove braces here. As enclosing if-clause has braces around this if-else, you won't hit so-called "dangling else" issue. https://en.wikipedia.org/wiki/Dangling_else https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:17: } nit: you can remove {}. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:48: assignedNodes = AssignedElementTraversal::slot(*assignedAncestor)->getAssignedNodes(); nit: I feel like on this line it should be slot = AssignedElementTraversal::slot(*assignedAncestor); and remove line 37 and 42, then after this if-else, WillBeHeapVector<RefPtrWillbeMember<Node>> assignedNodes = slot->getAsignedNodes(); ? https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:77: return AssignedElementTraversal::assignedAncestor(current); aha, you don't need !! in front of AssingedElementTraversal::... this line. (because of automatic pointer -> bool conversion) https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:121: } Insert newline here, between 2 functions. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:124: if (m_current) { I think the previous structure look better. i.e. if (!m_current) return; ... With that style you can save one level of indent after that. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:137: if (m_current) { Same as previous function, please make this like if (!m_current) return; ... https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:637: toElement(node); It looks you need "return" here.
https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:637: toElement(node); On 2016/03/02 13:26:56, kochi wrote: > It looks you need "return" here. Probably the layout test failure is due to this. Could you check?
https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:24: Element* element = const_cast<Element*>(¤t); On 2016/03/02 13:26:56, kochi wrote: > On 2016/03/02 09:02:14, yuzuchan wrote: > > On 2016/03/01 12:55:40, kochi wrote: > > > Is const_cast<> really necessary? > > > > Yes, because I update element in the for loop below. > > That is not the reason. > You can write > > const Element* element = ¤t; > > then you can update element below, like element = element->parentElement();. > What you cannot update is the content of element. > > that said, you end up in returning element on line 32, > and that line require const_cast<> anyway. > > So it doesn't make much difference in the end :( Acknowledged. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:32: return element; On 2016/03/02 13:26:56, kochi wrote: > On 2016/03/02 09:02:14, yuzuchan wrote: > > On 2016/03/01 12:55:40, kochi wrote: > > > This can be written as: > > > > > > for (Element* element = const_cast<Element*>(¤t); element; element = > > > element->parentElement()) { > > > if (element->assignedSlot()) > > > break; > > > } > > > return element; > > > > I kept Element* element = const_cast<Element*>(¤t); in the first line of > > this method, since I would not be able to return element if I put it in for > loop > > initialization. > > I see your point... but > > Element* element = const_cast<Element*>(¤t); > for (; element; element->parentElement()) { > if (element->assignedSlot()) > break; > } > return element; > > is shorter, isn't it? > > or > > for (Element* element = const_cast<Element*>(¤t); element; element = > element->parentElement()) { > if (element->assignedSlot()) > return element; > } > return nullptr; > > (you already done the former!) Acknowledged. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:158: } On 2016/03/02 13:26:56, kochi wrote: > On 2016/03/02 09:02:14, yuzuchan wrote: > > On 2016/03/01 12:55:41, kochi wrote: > > > nit: both if- and else- clasue are one-liner, omit {}. > > > > hayato@ told me not to! > > See https://www.chromium.org/blink/coding-style#TOC-Braces > You can keep the braces, but you can remove. > The rule is that keep it consistent for both if- and else-. > > I don't see strong reason not to remove braces here. > > As enclosing if-clause has braces around this if-else, you won't > hit so-called "dangling else" issue. > https://en.wikipedia.org/wiki/Dangling_else Done. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:539: // Find focusable node in descendant scope. If not found, find next focusable node within the On 2016/03/01 12:55:41, kochi wrote: > Do not revert back from element to node. > A node is not focusable, an element is focusable. Done. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:17: } On 2016/03/02 13:26:56, kochi wrote: > nit: you can remove {}. Done. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:48: assignedNodes = AssignedElementTraversal::slot(*assignedAncestor)->getAssignedNodes(); On 2016/03/02 13:26:56, kochi wrote: > nit: I feel like on this line it should be > slot = AssignedElementTraversal::slot(*assignedAncestor); > and remove line 37 and 42, then after this if-else, > WillBeHeapVector<RefPtrWillbeMember<Node>> assignedNodes = > slot->getAsignedNodes(); > ? Done. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:77: return AssignedElementTraversal::assignedAncestor(current); On 2016/03/02 13:26:56, kochi wrote: > aha, you don't need !! in front of AssingedElementTraversal::... this line. > (because of automatic pointer -> bool conversion) Acknowledged. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:121: } On 2016/03/02 13:26:56, kochi wrote: > Insert newline here, between 2 functions. Done. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:124: if (m_current) { On 2016/03/02 13:26:56, kochi wrote: > I think the previous structure look better. i.e. > > if (!m_current) > return; > ... > > With that style you can save one level of indent after that. Done. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:137: if (m_current) { On 2016/03/02 13:26:56, kochi wrote: > Same as previous function, please make this like > > if (!m_current) > return; > ... Done. https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:637: toElement(node); On 2016/03/03 00:54:17, kochi wrote: > On 2016/03/02 13:26:56, kochi wrote: > > It looks you need "return" here. > > Probably the layout test failure is due to this. > Could you check? I accidentally removed return here and then I put it back, but some layout tests are still failing. I'm guessing the fact that adjustToElement is not using the new traversal (ScopedFocusNavigation traversal) is the cause of test failures. Let me address the issue in the next patchset.
Yuzu, thank you for updating your CL. It looks great. The approach itself looks correct. I think we can land this CL after fixing minor issues. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html:51: 'light-child-B', Is this an intentional change? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core.gypi:2625: 'dom/shadow/AssignedElementTraversal.h', Nit: How about renaming this to SlotAssignedSubtreesTraversal? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:2278: if (!isChildOfV1ShadowHost()) Is it possible to move ASSERT(!needsDistributionRecalc()) before this line? Will it cause a crash? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:40: // if current is indirectly assigned to a slot, find a directly-assigned ancestor. A sentence in a comment should start with a capital letter. I understand what *directly/indirectly* means here, but could you avoid to use *directly/indirectly* here? e.g. // If current is not assigned to a slot, find an ancestor which is assigned to a slot. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:41: Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current); Could you assert ASSERT(assignedAncestor)? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:44: slot = AssignedElementTraversal::slot(*assignedAncestor); "slot = assingedAncestor.assignedSlot()" might be enough. Nit: ASSERT(slot) can be inserted. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:47: size_t currentIndex = assignedNodes.find(current); Looks |assignedAncestor| should be used instead of |current| if "current.assignedSlot()" is null. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:48: if (currentIndex != kNotFound && currentIndex < assignedNodes.size() - 1) We can ASSERT(currentIndex != kNotFound). It looks we always passes an element which exists in slot.getAssignedNodes(). https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:49: return toElement(assignedNodes[currentIndex + 1]); assignedNodes can contain a non-Element Node, such as a TextNode. We have to skip that! https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:55: Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current); Can we assert ASSERT(assignedAncestor)? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:61: size_t currentIndex = assignedNodes.reverseFind(current); Looks we have to pass |assignedAncestor|, instead of |current|, to assignedNodes.reverseFind(). https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:62: if (currentIndex != kNotFound && currentIndex > 0) { ASSERT(currentIndex != kNotFound) https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:64: if (Node* lastChild = assignedPrevious->lastChild()) Looks ElementTraversal::lastWithin(assignedPrevious) should be used here. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:66: // if the previous assigned node has no children, return the assigned node itself assignedPrevious can be non-element Node, such as TextNode. We should skip a non-element Node and continue to find an Element. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:72: bool AssignedElementTraversal::isInAssignedScope(const Element& current) Nit: How about renaming this to isInSubtreeAssignedToSlot? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h:15: static HTMLSlotElement* slot(const Element& current); Nit: How about renaming this to a more descriptive name? e.g. assignedSlotOfNearestAncestor https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h:16: static Element* assignedAncestor(const Element& current); Nit: How about renaming this function to nearestAncestorAssignedToSlot? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h:19: static bool isInAssignedScope(const Element& current); Nit: It might be difficult to understand what isInAssignedScope means. How about renaming this to isDescendantOfSlot? https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:84: static ScopedFocusNavigation focusNavigationScopeOf(const Element& root, const Element* current); You might want to rename |focusNavigationScopeOf| to something else. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:92: explicit ScopedFocusNavigation(TreeScope*, const Element*); We do not need to specify |explicit| if a constructor takes more than one parameters. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:99: ScopedFocusNavigation::ScopedFocusNavigation(TreeScope* treeScope, const Element* current) "TreeScope*" can be "TreeScope&". https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:106: ScopedFocusNavigation::ScopedFocusNavigation(HTMLSlotElement* slot, const Element* current) "HTMLSlotElement*" can be "HTMLSlotElement&". Then, you can remove ASSERT(slot) in line 110. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:125: if (!m_current) If |moveToNext| is only called when |m_current| is non-null, you can ASSERT(m_current) here. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:138: if (!m_current) ditto https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:153: m_current = toElement(m_rootSlot->getAssignedNodes().first().get()); ditto: getAssignedNodes().first() could be a non-element Node. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:168: m_current = toElement(m_rootSlot->getAssignedNodes().last().get()); ditto: getAssignedNodes().last() could be a non-element Node.
Thank you for review kochi san hayato san! https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html:51: 'light-child-B', On 2016/03/03 at 05:42:21, hayato wrote: > Is this an intentional change? Yes, it is. I inserted this line in order to make all the expectations PASS, instead of having some FAILs. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core.gypi:2625: 'dom/shadow/AssignedElementTraversal.h', On 2016/03/03 at 05:42:21, hayato wrote: > Nit: How about renaming this to SlotAssignedSubtreesTraversal? I changed the name to SlotScopedTraversal to shorten it. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:2278: if (!isChildOfV1ShadowHost()) On 2016/03/03 at 05:42:21, hayato wrote: > Is it possible to move ASSERT(!needsDistributionRecalc()) before this line? > Will it cause a crash? Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:40: // if current is indirectly assigned to a slot, find a directly-assigned ancestor. On 2016/03/03 at 05:42:21, hayato wrote: > A sentence in a comment should start with a capital letter. > > I understand what *directly/indirectly* means here, but could you avoid to use *directly/indirectly* here? > > e.g. > // If current is not assigned to a slot, find an ancestor which is assigned to a slot. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:41: Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current); On 2016/03/03 at 05:42:21, hayato wrote: > Could you assert ASSERT(assignedAncestor)? Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:44: slot = AssignedElementTraversal::slot(*assignedAncestor); On 2016/03/03 at 05:42:21, hayato wrote: > "slot = assingedAncestor.assignedSlot()" might be enough. > > Nit: ASSERT(slot) can be inserted. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:47: size_t currentIndex = assignedNodes.find(current); On 2016/03/03 at 05:42:21, hayato wrote: > Looks |assignedAncestor| should be used instead of |current| if "current.assignedSlot()" is null. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:48: if (currentIndex != kNotFound && currentIndex < assignedNodes.size() - 1) On 2016/03/03 at 05:42:21, hayato wrote: > We can ASSERT(currentIndex != kNotFound). It looks we always passes an element which exists in slot.getAssignedNodes(). Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:49: return toElement(assignedNodes[currentIndex + 1]); On 2016/03/03 at 05:42:21, hayato wrote: > assignedNodes can contain a non-Element Node, such as a TextNode. > We have to skip that! Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:55: Element* assignedAncestor = AssignedElementTraversal::assignedAncestor(current); On 2016/03/03 at 05:42:21, hayato wrote: > Can we assert ASSERT(assignedAncestor)? Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:61: size_t currentIndex = assignedNodes.reverseFind(current); On 2016/03/03 at 05:42:21, hayato wrote: > Looks we have to pass |assignedAncestor|, instead of |current|, to assignedNodes.reverseFind(). Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:62: if (currentIndex != kNotFound && currentIndex > 0) { On 2016/03/03 at 05:42:21, hayato wrote: > ASSERT(currentIndex != kNotFound) Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:64: if (Node* lastChild = assignedPrevious->lastChild()) On 2016/03/03 at 05:42:22, hayato wrote: > Looks ElementTraversal::lastWithin(assignedPrevious) should be used here. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:66: // if the previous assigned node has no children, return the assigned node itself On 2016/03/03 at 05:42:21, hayato wrote: > assignedPrevious can be non-element Node, such as TextNode. > We should skip a non-element Node and continue to find an Element. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.cpp:72: bool AssignedElementTraversal::isInAssignedScope(const Element& current) On 2016/03/03 at 05:42:21, hayato wrote: > Nit: How about renaming this to isInSubtreeAssignedToSlot? Renamed to isSlotScoped. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h:15: static HTMLSlotElement* slot(const Element& current); On 2016/03/03 at 05:42:22, hayato wrote: > Nit: How about renaming this to a more descriptive name? e.g. assignedSlotOfNearestAncestor Renamed to findScopeOwnerSlot. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h:16: static Element* assignedAncestor(const Element& current); On 2016/03/03 at 05:42:22, hayato wrote: > Nit: How about renaming this function to nearestAncestorAssignedToSlot? Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/AssignedElementTraversal.h:19: static bool isInAssignedScope(const Element& current); On 2016/03/03 at 05:42:22, hayato wrote: > Nit: It might be difficult to understand what isInAssignedScope means. How about renaming this to isDescendantOfSlot? Renamed to isSlotScoped https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:84: static ScopedFocusNavigation focusNavigationScopeOf(const Element& root, const Element* current); On 2016/03/03 at 05:42:22, hayato wrote: > You might want to rename |focusNavigationScopeOf| to something else. Renamed to createScopedFocusNavigation https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:92: explicit ScopedFocusNavigation(TreeScope*, const Element*); On 2016/03/03 at 05:42:22, hayato wrote: > We do not need to specify |explicit| if a constructor takes more than one parameters. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:99: ScopedFocusNavigation::ScopedFocusNavigation(TreeScope* treeScope, const Element* current) On 2016/03/03 at 05:42:22, hayato wrote: > "TreeScope*" can be "TreeScope&". Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:106: ScopedFocusNavigation::ScopedFocusNavigation(HTMLSlotElement* slot, const Element* current) On 2016/03/03 at 05:42:22, hayato wrote: > "HTMLSlotElement*" can be "HTMLSlotElement&". > Then, you can remove ASSERT(slot) in line 110. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:125: if (!m_current) On 2016/03/03 at 05:42:22, hayato wrote: > If |moveToNext| is only called when |m_current| is non-null, you can ASSERT(m_current) here. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:138: if (!m_current) On 2016/03/03 at 05:42:22, hayato wrote: > ditto Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:153: m_current = toElement(m_rootSlot->getAssignedNodes().first().get()); On 2016/03/03 at 05:42:22, hayato wrote: > ditto: getAssignedNodes().first() could be a non-element Node. Done. https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:168: m_current = toElement(m_rootSlot->getAssignedNodes().last().get()); On 2016/03/03 at 05:42:22, hayato wrote: > ditto: getAssignedNodes().last() could be a non-element Node. Done.
https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/core.gypi:2625: 'dom/shadow/AssignedElementTraversal.h', On 2016/03/04 03:48:36, yuzuchan wrote: > On 2016/03/03 at 05:42:21, hayato wrote: > > Nit: How about renaming this to SlotAssignedSubtreesTraversal? > > I changed the name to SlotScopedTraversal to shorten it. Sounds good. https://codereview.chromium.org/1707443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1707443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:2280: return nullptr; isChildOfV1ShadowHost() is almost identical to lines 2281-2282, and you can remove this. bool Node::isChildOfV1ShadowHost() const { ElementShadow* parentShadow = parentElementShadow(); return parentShadow && parentShadow->isV1(); }
Filed crbug.com/591909 Could you update BUG= line in the description?
Description was changed from ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG= ========== to ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG=591909 ==========
On 2016/03/04 at 04:20:37, kochi wrote: > Filed crbug.com/591909 > Could you update BUG= line in the description? Done. Thank you for filing the bug!
PTAL https://codereview.chromium.org/1707443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1707443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:2280: return nullptr; On 2016/03/04 at 04:16:30, kochi wrote: > isChildOfV1ShadowHost() is almost identical to lines 2281-2282, and you can remove this. > > bool Node::isChildOfV1ShadowHost() const > { > ElementShadow* parentShadow = parentElementShadow(); > return parentShadow && parentShadow->isV1(); > } Done.
almost look good, could you check oilpan bot failure?
On 2016/03/04 at 08:42:40, kochi wrote: > almost look good, could you check oilpan bot failure? Thanks, I'm working on it. It seems like OILPAN bot is failing because ASSERT(!needsDistributionRecalc) fails with iframes.
Description was changed from ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG=591909 ========== to ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots, so that the assigned elements would be traversed together, which is visually more consistent than the current implementation. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG=591909 ==========
Yuzu, I think you could reproduce a crash in linux_blink_oilpan bot, as follows: 1. Go to the build page: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oi... 2. Click "steps > stdio" => https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oi... 3. Find a text: "gn gen": You can see: "/b/build/slave/linux_oilpan/build/src/buildtools/linux64/gn gen //out/Release '--args=enable_oilpan=false is_debug=false is_component_build=false use_goma=true goma_dir="/home/chrome-bot/goma" symbol_level=1 dcheck_always_on=true symbol_level=1 target_cpu="x64"' --check" It looks linux_blink_oilpand_rel is using 'enable_oilpan=false', which looks confusing :( 4. In your linux machine, run "gn args" with enable_oilpan=false # e.g. $ gn gen $(chrome_dir)/out/Default --args="use_goma=true enable_oilpan=false is_debug=true" 4. To run webkit_unit_tests, which seem to fail: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oi... 1. Add blink_tests to ninja's build targets # e.g. $ ninja $(chrome_dir)/out/Default content_shell blink_tests 2. Run the unit tests $(chrome_dir)/out/Default/webkit_unit_tests # You can filter the tests to be run as follows: $(chrome_dir)/out/Default/webkit_unit_tests --gtest_filter="WebViewTest.NonUserInputTextUpdate' Good luck.
https://codereview.chromium.org/1707443003/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:101: , m_current(const_cast<Element*>(current)) It looks all member variables will be raw pointers in non-oilpan build. Thus, you have to initialize a pointer with nullptr explicitly. e.g. : m_rootNode(treeScope.rootNode()) , m_rootSlot(nullptr) , m_current(const_cast<Element*>(current))
Thanks hayato-san for the detailed instructions, I think the oilpan bot failure is now resolved. https://codereview.chromium.org/1707443003/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:101: , m_current(const_cast<Element*>(current)) On 2016/03/07 at 04:09:48, hayato wrote: > It looks all member variables will be raw pointers in non-oilpan build. > Thus, you have to initialize a pointer with nullptr explicitly. > > e.g. > : m_rootNode(treeScope.rootNode()) > , m_rootSlot(nullptr) > , m_current(const_cast<Element*>(current)) Done. Thanks, this seems to resolve oilpan bot failures.
Thank you. You can land this after confirming all bots become green (・∀・) LGTM
The CQ bit was checked by yuzus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707443003/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707443003/490001
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Description was changed from ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots, so that the assigned elements would be traversed together, which is visually more consistent than the current implementation. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG=591909 ========== to ========== Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots, so that the assigned elements would be traversed together, which is visually more consistent than the current implementation. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG=591909 Committed: https://crrev.com/856302237805d511bcc93ffc965b674406d7c80a Cr-Commit-Position: refs/heads/master@{#379523} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/856302237805d511bcc93ffc965b674406d7c80a Cr-Commit-Position: refs/heads/master@{#379523} |