Skip to content

Commit

Permalink
Fix FocusNavigation::FindOwner Parent Node is Slot case
Browse files Browse the repository at this point in the history 
FocusNavigation::FindOwner in focus_controller.cc has the
wrong logic that a node whose parent is a slot should always return
the ParentOrShadowHostElement. However, if that slot is in a slot scope and is a nested child, we don't want to return its parent.
Instead, we want to return the closest ancestor that is an assigned
slot.

This bug was causing the wrong owner to get found for nested slots
in slot scope, which then caused skipping elements within the same
slot when navigating.

Change-Id: I4e30a62b70d0cb977f9d21a0d7a40994f271473b
Fixed: 1327064, 1014868
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3784451
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028502}
  • Loading branch information
@dizhang168
dizhang168 authored and Chromium LUCI CQ committed Jul 26, 2022
1 parent 25ec23e commit 95348cc
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 8 deletions.
17 changes: 13 additions & 4 deletions third_party/blink/renderer/core/page/focus_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,13 @@ class FocusNavigation : public GarbageCollected<FocusNavigation> {
return nullptr;
}

// Owner is the slot node for slot scope and slot fallback contents scope, the
// shadow host for shadow tree scope and the iframe node for frame scope.
// Owner of a FocusNavigation:
// - If node in slot scope, is the assigned slot (found by traversing
// ancestors)
// - If node in slot fallback content scope, is the parent or shadowHost
// element
// - If node in shadow tree scope, is the parent or shadowHost element
// - If node in frame scope, is the iframe node
Element* FindOwner(ContainerNode& node) {
auto result = owner_map_.find(&node);
if (result != owner_map_.end())
Expand All @@ -142,8 +147,12 @@ class FocusNavigation : public GarbageCollected<FocusNavigation> {
// the slot node have assigned nodes.

Element* owner = nullptr;
if (node.AssignedSlot())
owner = node.AssignedSlot();
Element* owner_slot = nullptr;
if (Element* element = DynamicTo<Element>(node))
owner_slot = SlotScopedTraversal::FindScopeOwnerSlot(*element);

if (owner_slot)
owner = owner_slot;
else if (IsA<HTMLSlotElement>(node.parentNode()))
owner = node.ParentOrShadowHostElement();
else if (&node == node.ContainingTreeScope().RootNode())
Expand Down
4 changes: 0 additions & 4 deletions .../blink/web_tests/platform/generic/shadow-dom/focus-reverse-unassignable-slot-expected.txt
View file

This file was deleted.

68 changes: 68 additions & 0 deletions third_party/blink/web_tests/shadow-dom/focus-navigation-slots-in-slot.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<!DOCTYPE html>
<script src='../resources/testharness.js'></script>
<script src='../resources/testharnessreport.js'></script>
<script src='resources/shadow-dom.js'></script>
<script src='resources/focus-utils.js'></script>
<p>Tests for moving focus by pressing tab key across nodes in slot scope.<br>

<button id="b1">outside</button>
<div id='host'>
<template data-mode='open'>
<slot></slot>
</template>
<slot>
<button id="1A">single nested slot</button>
<button id="1B">single nested slot</button>
</slot>
<slot>
<button id="1C">single nested slot</button>
</slot>
<slot>
<slot>
<button id="2A">double nested slot</button>
<button id="2B">double nested slot</button>
</slot>
</slot>
<slot>
<button id="3A">single nested slot</button>
<slot>
<button id="3B">double nested slot</button>
<slot>
<button id="3C">Triple nested slot</button>
<button id="3D">Triple nested slot</button>
</slot>
<button id="3E">double nested slot</button>
</slot>
<button id="3F">single nested slot</button>
</slot>
</div>
<button id="b2">outside</button>

<script>
'use strict';

test(() => {
convertTemplatesToShadowRootsWithin(host);

let elements = [
'b1',
'1A',
'1B',
'1C',
'2A',
'2B',
'3A',
'3B',
'3C',
'3D',
'3E',
'3F',
'b2',
];

assert_focus_navigation_forward(elements);
elements.reverse();
assert_focus_navigation_backward(elements);
}, 'Focus should cover assigned nodes of slot, especially for nested slots in slot scope.');

</script>
47 changes: 47 additions & 0 deletions third_party/blink/web_tests/shadow-dom/focus-nested-slots.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<!DOCTYPE html>
<meta charset="utf-8">
<link rel="author" title="Joey Arhar" href="mailto:jarhar@chromium.org">
<link rel="help" href="https://bugs.chromium.org/p/chromium/issues/detail?id=1209217">
<script src='../resources/testharness.js'></script>
<script src='../resources/testharnessreport.js'></script>

<button id=button1>one</button>
<span>
<template shadowroot=open>
<slot name=myslot></slot>
</template>
<slot slot=myslot>
<button id=button2>two</button>
<button id=button3>three</button>
<button id=button4>four</button>
</slot>
</span>
<button id=button5>five</button>

<script>
test(() => {
button1.focus();
assert_equals(document.activeElement, button1);

// TODO(crbug.com/893480): Replace this with test_driver.Actions() and move
// this test to WPT when test_driver.Actions() is supported in content_shell
// or when all WPTs are run on chrome instead of content_shell.
eventSender.keyDown('\t');
assert_equals(document.activeElement, button2);
eventSender.keyDown('\t');
assert_equals(document.activeElement, button3);
eventSender.keyDown('\t');
assert_equals(document.activeElement, button4);
eventSender.keyDown('\t');
assert_equals(document.activeElement, button5);
eventSender.keyDown('\t', ['shiftKey']);
assert_equals(document.activeElement, button4);
eventSender.keyDown('\t', ['shiftKey']);
assert_equals(document.activeElement, button3);
eventSender.keyDown('\t', ['shiftKey']);
assert_equals(document.activeElement, button2);
eventSender.keyDown('\t', ['shiftKey']);
assert_equals(document.activeElement, button1);
}, `Verifies that focus order goes in flat tree order with buttons inside nested slots which have a mixture of assigned and unassigned states.`);

</script>
36 changes: 36 additions & 0 deletions third_party/blink/web_tests/shadow-dom/focus-reverse-unassigned-slot.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<!DOCTYPE html>
<script src='../resources/testharness.js'></script>
<script src='../resources/testharnessreport.js'></script>
<script src='resources/shadow-dom.js'></script>
<script src='resources/focus-utils.js'></script>
<div id="log"></div>

<input id=i0 value=i0>
<div id=outer>
<template data-mode=open>
<div id=inner>
<template data-mode=open>
<div>
<slot name=inside></slot>
</div>
</template>
<slot name=inside slot=inside>
<input id=i1 value=i1>
</slot>
</div>
</template>
</div>
<input id=i2 value=i2>

<script>
test(() => {
convertTemplatesToShadowRootsWithin(document.getElementById('outer'));

const elements = [
'i2',
'outer/i1',
'i0'
];
assert_focus_navigation_backward(elements);
}, `Verifies that focusing backwards from a button inside a slot which has no assigned nodes goes to the previous focusable element.`);
</script>

0 comments on commit 95348cc

Please sign in to comment.