Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2432293002:
Fix focus navigation for nested slot case (Closed)
|
Created:
4 years, 2 months ago by kochi Modified:
4 years, 1 month ago Reviewers:
hayato CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix focus navigation for nested slot case
For forward navigation, SlotScopedTraversal was not
excluding an element that is assigned to another slot.
For backward navigation, the original logic in
FocusController/SlotScopedTraversal failed to
detect case when shadow hosts exist in a slot scope.
Elements that are slotted to slots in their shadow roots
should be skipped in traversal.
BUG=618587
Committed: https://crrev.com/5639462246b2b792a1d75ba418fce6c6edd76940
Cr-Commit-Position: refs/heads/master@{#428682}
Patch Set 1 : (not ready for review) #Patch Set 2 : Fix for delegatesFocus case. #Patch Set 3 : More proper fix #
Total comments: 8
Patch Set 4 : address review comments. #Patch Set 5 : extract first/last function for SlotScopedTraversal #Patch Set 6 : Implement traversal methods for SlotScopedTraversal::previous() #Patch Set 7 : Minor cleanups #
Total comments: 8
Patch Set 8 : fix for hayato's review comments #Patch Set 9 : Fix bugs, add unit tests #
Total comments: 15
Patch Set 10 : Fix for hayato's review comments. #
Total comments: 4
Patch Set 11 : More references #Messages
Total messages: 56 (40 generated)
Description was changed from ========== Fix focus navigation. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedNavigation was not handling elements that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return focusable element which is slotted. BUG=618587 ==========
kochi@chromium.org changed reviewers: + hayato@chromium.org
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Description was changed from ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedNavigation was not handling elements that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return focusable element which is slotted. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedNavigation was not handling elements that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return element in its light DOM assigned to a slot. BUG=618587 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedNavigation was not handling elements that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return element in its light DOM assigned to a slot. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not handling elements that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return element in its light DOM assigned to a slot. BUG=618587 ==========
Description was changed from ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not handling elements that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return element in its light DOM assigned to a slot. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not handling an element that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return an element in its light DOM assigned to a slot. BUG=618587 ==========
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not handling an element that is assigned to another slot. For backward navigation, the original logic failed to detect a nested shadow host and can return an element in its light DOM assigned to a slot. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not excluding an element that is assigned to another slot. For backward navigation, the original logic in FocusController/SlotScopedTraversal failed to detect case when shadow hosts exist in a slot scope. Elements that are slotted to slots in their shadow roots should be skipped in traversal. BUG=618587 ==========
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:35: Element* nearestAncestorAssignedToSlot = Could we rename |nearestAncestorAssignedToSlot| to |nearestInclusiveAncestorAssignedToSlot|? The current name might be confusing. https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:37: // Search within children of an element which is directly assigned to a slot. Could you remove |directly| from the comments in this file? It is redundant. We do not want to use |indirectly assigned| in any case, which is confusing. https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:43: if (current == nearestAncestorAssignedToSlot) When does it happen? It looks that current.assignedSlot() is true if current == nearestAncestorAssignedToSlot. https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:81: while (previous) { Could you extract L81-L88 as a separate function? It looks there are similar patterns in other places. In addition to that, I do not understand why we need this check. Could you explain briefly what this check achieves?
https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:35: Element* nearestAncestorAssignedToSlot = On 2016/10/24 06:43:26, hayato wrote: > Could we rename |nearestAncestorAssignedToSlot| to > |nearestInclusiveAncestorAssignedToSlot|? > > The current name might be confusing. I agree the renaming, and adding "Inclusive" clarifies what it does more. With that, the comment in the function (L23-24) become redundant as the function name almost says the same thing. And as you said in the other comment, 'directly assigned' is not preferable. Also I renamed variables that hold return value from the function to 'slottedAncestor', which would be enough to describe what it is in a function. https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:37: // Search within children of an element which is directly assigned to a slot. On 2016/10/24 06:43:26, hayato wrote: > Could you remove |directly| from the comments in this file? > It is redundant. We do not want to use |indirectly assigned| in any case, which > is confusing. Done. https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:43: if (current == nearestAncestorAssignedToSlot) On 2016/10/24 06:43:26, hayato wrote: > When does it happen? > It looks that current.assignedSlot() is true if current == > nearestAncestorAssignedToSlot. Good catch. Looks like an artifact while I was debugging. Remved. https://codereview.chromium.org/2432293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:81: while (previous) { On 2016/10/24 06:43:26, hayato wrote: > Could you extract L81-L88 as a separate function? > It looks there are similar patterns in other places. > > In addition to that, I do not understand why we need this check. > > Could you explain briefly what this check achieves? Separated as a function previousWithinSameSlotScopeOrSelf(). In the original code, it returned element from ElementTraversal::previous() directly, but that element could be in a different slot scope (i.e. assigned to another slot). As this SlotScopedTraversal has to guarantee that the traversal happens within the same slot scope, we have to try to find a previous element that is in the same slot scope. For SlotScoepdTraversal::previous(current), |current|'s scope is |nearestAncestorAssignedToSlot|'s slot. If |current|'s previous or any of its ancestor is assigned to another slot, the scopes are different. So we check the equality of the scope with nearest assigned slots, and if they are same, return the previous. Otherwise, such previous element is in a different slot scope, and in the light DOM of a shadow host, so move up to the shadow host to continue searching. (|ancestor->parentElement()| on line 87 should be a shadow host.)
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As we discussed offline, I improved previous() logic so that it doesn't need any nearest ancestor slotted element comparison. For finding previous element, this CL uses normal ElementTraversal::previous() and then traverses the path again with NodeTraversal::previousPostOrder() to detect a shadow host in between. This is a bit compromised version and the above could be done in one-pass, but would require own version of internal previous(), previousSibling() and parent(), which would need more unit-tests. What do you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> For finding previous element, this CL uses normal > ElementTraversal::previous() and then traverses the path > again with NodeTraversal::previousPostOrder() to detect > a shadow host in between. > > This is a bit compromised version and the above could be > done in one-pass, but would require own version of internal > previous(), previousSibling() and parent(), which would > need more unit-tests. > > What do you think? That sounds a bad idea. The code is getting much complicated, and it would be hard for me to understand what is happening. I can not verify the correctness easily. Is there any other reason to avoid the idea which we have concluded yesterday? If the reason is that we have to write a unit test for that, the current approach also lacks a unit test for newly introduced functions. :) https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:33: Element* slottedAncestor = Could you avoid to use "Ancestor" here? It is confusing because we are likely to think that this is exclusive of |current|. https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:36: if (current.assignedSlot()) { This "if .. else .." looks redundant. We can use ElementTraversal::next(current, slottedAncestor) in either case. Also, we can merge this into the bellow "while" loop as "for" loop. e.g. for (next = ElementTraversal::next(..., ...); next; next = ElementTraversal::nextSkippingChildren(...)) { if (!next->assignedSlot()) return next; } https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:40: DCHECK(slottedAncestor); We should DCHECK immediately after L34. https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:49: DCHECK(!next->assignedSlot()); Nit: I would remove this DCHECK because it is just repeating the condition of the above "while" loop.
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/26 04:24:12, hayato wrote: > > For finding previous element, this CL uses normal > > ElementTraversal::previous() and then traverses the path > > again with NodeTraversal::previousPostOrder() to detect > > a shadow host in between. > > > > This is a bit compromised version and the above could be > > done in one-pass, but would require own version of internal > > previous(), previousSibling() and parent(), which would > > need more unit-tests. > > > > What do you think? > > That sounds a bad idea. The code is getting much complicated, and it would be > hard for me to understand what is happening. I can not verify the correctness > easily. > > Is there any other reason to avoid the idea which we have concluded yesterday? > If the reason is that we have to write a unit test for that, the current > approach also lacks a unit test for newly introduced functions. :) I had expected the reaction... I wrote the previous() function for this. I'm writing a unittest for it now, but if you have time, the code is up for review now.
https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:33: Element* slottedAncestor = On 2016/10/26 04:24:12, hayato wrote: > Could you avoid to use "Ancestor" here? > It is confusing because we are likely to think that this is exclusive of > |current|. Does |slottedInclusiveAncestor| sound okay then? I didn't come up with a better name - so reverted to |nearestInclusiveAncestorAssignedToSlot|. https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:36: if (current.assignedSlot()) { On 2016/10/26 04:24:12, hayato wrote: > This "if .. else .." looks redundant. > > We can use ElementTraversal::next(current, slottedAncestor) in either case. > > Also, we can merge this into the bellow "while" loop as "for" loop. > > e.g. > for (next = ElementTraversal::next(..., ...); next; next = > ElementTraversal::nextSkippingChildren(...)) { > if (!next->assignedSlot()) > return next; > } Done. Really good catch! https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:40: DCHECK(slottedAncestor); On 2016/10/26 04:24:12, hayato wrote: > We should DCHECK immediately after L34. Done. https://codereview.chromium.org/2432293002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:49: DCHECK(!next->assignedSlot()); On 2016/10/26 04:24:12, hayato wrote: > Nit: I would remove this DCHECK because it is just repeating the condition of > the above "while" loop. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> I had expected the reaction... I wrote the previous() function for this. > I'm writing a unittest for it now, but if you have time, the code is up for > review now. The logic seems broken. Please hold on any reviews.
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The new CL with unit tests uploaded. Could you review this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:14: Element* nextSkippingShadowHost(const Element& start, const Element& scope) { This name is confusing because it is inconsistent with the usage of ElementTraversal::nextSkippingChildren. It looks that this does not skip a shadow host. This could return a shadow host. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:16: const Element* current = &start; It does not need to declare |current| here. It is confusing. Declare |current| and assign |start| to it before L22, using |for| loop. e.g. for (const Element* current = &start; current != scope; current = current->parentElement() ) { ... } https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:32: while (!current->authorShadowRoot()) { You can rewrite this with for loop. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:122: HeapVector<Member<Node>> assignedNodes = slot.assignedNodes(); Use a reference. Unless that, vector is copied, I'm afraid. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:136: return lastWithinOrSelfSkippingShadowHost(*toElement(*assignedNode)); Hmm. This function name is confusing because it return an element whose assigned slot is null. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp (right): https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:129: const Element* expectedSequence[kLength] = {inner1, inner2}; Use vector, instead of an array. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:252: EXPECT_EQ(nullptr, SlotScopedTraversal::next(*child[0])); Is this an intentional behavior? Is there any use case for child[0] being called for SlotScopedTraversal::next()?
Patchset #10 (id:200001) has been deleted
https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:14: Element* nextSkippingShadowHost(const Element& start, const Element& scope) { On 2016/10/31 05:27:12, hayato wrote: > This name is confusing because it is inconsistent with the usage of > ElementTraversal::nextSkippingChildren. > It looks that this does not skip a shadow host. This could return a shadow host. All renamed to *SkippingChildrenOfShadowHost(). https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:16: const Element* current = &start; On 2016/10/31 05:27:12, hayato wrote: > It does not need to declare |current| here. It is confusing. > Declare |current| and assign |start| to it before L22, using |for| loop. > > e.g. > for (const Element* current = &start; current != scope; current = > current->parentElement() ) { > ... > } Done. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:32: while (!current->authorShadowRoot()) { On 2016/10/31 05:27:12, hayato wrote: > You can rewrite this with for loop. I'm not sure rewriting with for loop improves readability for this case: The shortest one (duplicate ElementTraversal::lastChild() calls): Element* lastWithinOrSelfSkippingChildrenOfShadowHost(const Element& scope) { Element* current = const_cast<Element*>(&scope); for (; !current->authorShadowRoot() && ElementTraversal::lastChild(*current); current = ElementTraversal::lastChild(*current)); return current; } Or (still redundant nullptr initialization for lastChild): Element* lastWithinOrSelfSkippingChildrenOfShadowHost(const Element& scope) { Element* current = const_cast<Element*>(&scope); for (Element* lastChild = nullptr; !current->authorShadowRoot(); current = lastChild) { lastChild = ElementTraversal::lastChild(*current); if (!lastChild) break; } return current; } ? Do you have any particular idea? https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:122: HeapVector<Member<Node>> assignedNodes = slot.assignedNodes(); On 2016/10/31 05:27:13, hayato wrote: > Use a reference. Unless that, vector is copied, I'm afraid. Done. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp (right): https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:129: const Element* expectedSequence[kLength] = {inner1, inner2}; On 2016/10/31 05:27:13, hayato wrote: > Use vector, instead of an array. Done. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:252: EXPECT_EQ(nullptr, SlotScopedTraversal::next(*child[0])); On 2016/10/31 05:27:13, hayato wrote: > Is this an intentional behavior? > > Is there any use case for child[0] being called for SlotScopedTraversal::next()? child[0] is slotted to another slot, and it is the only element that is assigned to the slot, so the next/previous for child[0] is nullptr. Another intention of having this test here is to make sure child[0] is in another slot scope, and not leak any node in the |slot|'s slot scope.
LGTM Could you file another bug to support nested slot fallback elements in focus navigation? https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:32: while (!current->authorShadowRoot()) { I see. Thanks. It is okay to leave this as is because it wouldn't get better. https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp (right): https://codereview.chromium.org/2432293002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:252: EXPECT_EQ(nullptr, SlotScopedTraversal::next(*child[0])); On 2016/10/31 at 07:30:25, kochi wrote: > On 2016/10/31 05:27:13, hayato wrote: > > Is this an intentional behavior? > > > > Is there any use case for child[0] being called for SlotScopedTraversal::next()? > > child[0] is slotted to another slot, and it is the only element that is > assigned to the slot, so the next/previous for child[0] is nullptr. Ah, I see. You are using the same "shadowHTML" both in L239 and L227. I missed it. Please ignore my comment. https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:82: HeapVector<Member<Node>> assignedNodes = slot->assignedNodes(); Could you update other places to use a reference? I think there are still several places where a copy happens. https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp (right): https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:152: Element* inner[6]; Could you replace all usage of arrays with vectors?
Filed a separate issue for fallback slots as crbug.com/660806. https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp (right): https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversal.cpp:82: HeapVector<Member<Node>> assignedNodes = slot->assignedNodes(); On 2016/10/31 08:12:01, hayato wrote: > Could you update other places to use a reference? I think there are still > several places where a copy happens. Done. https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp (right): https://codereview.chromium.org/2432293002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/shadow/SlotScopedTraversalTest.cpp:152: Element* inner[6]; On 2016/10/31 08:12:01, hayato wrote: > Could you replace all usage of arrays with vectors? I think current form is good enough, as this is in the unit test and explicit correspondence (e.g. inner[0] <=> #inner0) is clear, while if we use Vector<Element*> and .append() repeatedly, it may not be that clear. Also, using HeapVector<Member<Element*>> (as the return value of querySelector() is garbage-collected, oilpan checker complains to use HeapVector<Member> instead of Vector<rawptr>), made use of initializer list (e.g. L175) impossible (because wtf Vector has not supported initializer list from Member<> yet?).
The CQ bit was checked by kochi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org Link to the patchset: https://codereview.chromium.org/2432293002/#ps240001 (title: "More references")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not excluding an element that is assigned to another slot. For backward navigation, the original logic in FocusController/SlotScopedTraversal failed to detect case when shadow hosts exist in a slot scope. Elements that are slotted to slots in their shadow roots should be skipped in traversal. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not excluding an element that is assigned to another slot. For backward navigation, the original logic in FocusController/SlotScopedTraversal failed to detect case when shadow hosts exist in a slot scope. Elements that are slotted to slots in their shadow roots should be skipped in traversal. BUG=618587 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not excluding an element that is assigned to another slot. For backward navigation, the original logic in FocusController/SlotScopedTraversal failed to detect case when shadow hosts exist in a slot scope. Elements that are slotted to slots in their shadow roots should be skipped in traversal. BUG=618587 ========== to ========== Fix focus navigation for nested slot case For forward navigation, SlotScopedTraversal was not excluding an element that is assigned to another slot. For backward navigation, the original logic in FocusController/SlotScopedTraversal failed to detect case when shadow hosts exist in a slot scope. Elements that are slotted to slots in their shadow roots should be skipped in traversal. BUG=618587 Committed: https://crrev.com/5639462246b2b792a1d75ba418fce6c6edd76940 Cr-Commit-Position: refs/heads/master@{#428682} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5639462246b2b792a1d75ba418fce6c6edd76940 Cr-Commit-Position: refs/heads/master@{#428682} |
