Skip to content

GitLab

T
tangled
Commit 990f31c3 authored by Jeonghee Ahn's avatar Jeonghee Ahn Committed by Commit Bot
Browse files
Options

[SpatNav] Ignore certain ancestor focusables 

Background:
In SpavNav mode, more elements have become focus candidates:
https://chromium-review.googlesource.com/c/chromium/src/+/1461355
https://chromium-review.googlesource.com/c/chromium/src/+/1632231
https://chromium-review.googlesource.com/c/chromium/src/+/1859535

These changes were the result of our efforts to avoid unreachable
elements. In other words, SpatNav started to consider more elements,
with tabIndex<0, as candidates.

Problem:
Sometimes this caused inconvenience because every "wrapping"
element got focused towards the innermost candidate: The user
had to press a direction key several times to reach the "real"
focusable element, see the bug for more details.

Solution:
This CL tries to ignore certain extra, inconvenient focus
candidates. If an element is recognized as focusable by SpatNav's
extended condition (SupportsSpatialNavigationFocus() in element.cc)
and it has one or several focusable descendant(s), then we ignore
it in favor for its focusable descendant(s).

We only ignore ancestor focusables with a non-negative tabIndex.

Bug: 982601
Change-Id: I8d98fb2261f205b000e2ab7bae9f801272649784
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900792
Commit-Queue: Jeonghee Ahn <jeonghee27.ahn@lge.com>
Reviewed-by: default avatarHugo Holgersson <hugoh@logikbyran.se>
Reviewed-by: default avatarFredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#724506}
parent f15bf434
Hide whitespace changes
Inline Side-by-side
Showing with 270 additions and 5 deletions
third_party/blink/renderer/core/page/spatial_navigation_controller.cc
View file @ 990f31c3
......@@ -69,6 +69,24 @@ void ClearFocusInExitedFrames(LocalFrame* old_frame,
}
}
bool IsSkippableCandidate(const Element* element) {
// SpatNav tries to ignore certain, inconvenient focus candidates.
// If an element is recognized as focusable by
// SupportsSpatialNavigationFocus() but has one or several focusable
// descendant(s), then we might ignore it in favor for its focusable
// descendant(s).
if (element->GetIntegralAttribute(html_names::kTabindexAttr, -1) >= 0) {
// non-negative tabindex was set explicitly.
return false;
}
if (IsRootEditableElement(*element))
return false;
return true;
}
// Determines whether the given candidate is closer to the current interested
// node (in the given direction) than the current best. If so, it'll replace
// the current best.
......@@ -76,7 +94,9 @@ static void ConsiderForBestCandidate(SpatialNavigationDirection direction,
const FocusCandidate& current_interest,
const FocusCandidate& candidate,
FocusCandidate* best_candidate,
double* best_distance) {
double& best_distance,
FocusCandidate* previous_best_candidate,
double& previous_best_distance) {
DCHECK(candidate.visible_node->IsElementNode());
DCHECK(candidate.visible_node->GetLayoutObject());
......@@ -96,9 +116,27 @@ static void ConsiderForBestCandidate(SpatialNavigationDirection direction,
if (distance == kMaxDistance)
return;
if (distance < *best_distance && IsUnobscured(candidate)) {
Element* candidate_element = To<Element>(candidate.visible_node.Get());
Element* best_candidate_element =
To<Element>(best_candidate->visible_node.Get());
if (candidate_element->IsDescendantOf(best_candidate_element) &&
IsSkippableCandidate(best_candidate_element) &&
best_candidate->rect_in_root_frame.Contains(
candidate.rect_in_root_frame)) {
// Revert to previous best_candidate because current best_candidate is
// a skippable candidate.
*best_candidate = *previous_best_candidate;
best_distance = previous_best_distance;
previous_best_distance = kMaxDistance;
}
if (distance < best_distance && IsUnobscured(candidate)) {
*previous_best_candidate = *best_candidate;
previous_best_distance = best_distance;
*best_candidate = candidate;
*best_distance = distance;
best_distance = distance;
}
}
......@@ -330,7 +368,8 @@ FocusCandidate SpatialNavigationController::FindNextCandidateInContainer(
current_interest.focusable_node = interest_child_in_container;
current_interest.visible_node = interest_child_in_container;
FocusCandidate best_candidate;
FocusCandidate best_candidate, previous_best_candidate;
double previous_best_distance = kMaxDistance;
double best_distance = kMaxDistance;
for (; element;
element =
......@@ -351,7 +390,8 @@ FocusCandidate SpatialNavigationController::FindNextCandidateInContainer(
continue;
ConsiderForBestCandidate(direction, current_interest, candidate,
&best_candidate, &best_distance);
&best_candidate, best_distance,
&previous_best_candidate, previous_best_distance);
}
return best_candidate;
......
third_party/blink/web_tests/fast/spatial-navigation/snav-skippable-candidate-unskippable.html 0 → 100644
View file @ 990f31c3
<!doctype html>
<style>
.cursor {
cursor: pointer;
}
.box {
display: inline-block;
position: absolute;
width: 100px;
}
div {
margin-top: 5px;
border: 1px solid black;
}
div div {
margin-top: 0;
width: 100px;
border: none;
}
* {
background: white;
}
.green {
/* To help debugging, every element in resultMap is green */
background: lightgreen;
}
.gray {
/* To help debugging, every skippable-element is gray */
background: lightgray;
}
</style>
<button class="green" id="start">Start</button>
<div class="clickable green" id="a">A
<div>A-child1</div>
<div>A-child2</div>
</div>
<div class="cursor green" id="b">B
<div>B-child1</div>
<div>B-child2</div>
</div>
<div class="cursor green" id="c" style="height: 20px; margin-bottom: 40px;">C
<!-- |c-child| is out side of |c|. -->
<div class="green" tabindex="0" id="c-child" style="margin-top: 20px;">C-child</div>
</div>
<div class="cursor green" id="d" style="height: 20px; margin-bottom: 20px;">D
<!-- |d-child| extends outside of the ancestor of |d|. -->
<div class="green" tabindex="0" id="d-child">D-child</div>
</div>
<div id="e" style="position: relative; height: 50px">E
<div class="clickable box" id="e-child1" style="left: 110px; top: 30px;">E-child1</div>
<div class="clickable box gray" id="e-child2" style="left: 0; top: 15px;">E-child2
<div class="clickable green" id="e-child2-child">E-child2-child</div>
</div>
</div>
<button class="green" style="margin-left: 90px;" id="button">Button</button>
<div id="f" style=" position: relative; height: 50px">F
<div class="clickable box green" id="f-child1" style="left: 110px; top: 30px;">F-child1</div>
<div class="clickable box gray" id="f-child2" style="left: 0; top: 15px;">F-child2
<div class="clickable" id="f-child2-child">F-child2-child</div>
</div>
</div>
<div class="cursor green" tabindex="0" id="g">G
<div class="green" tabindex="0" id="g-child">G-child</div>
</div>
<div class="editable green" id="h">H
<div class="clickable green uneditable" id="h-child">H-child</div>
</div>
<div class="green" id="i" style="height: 50px; overflow-y: scroll;">I
<div class="clickable green" id="i-child" style="margin: 20px;">I-child</div>
</div>
<p>This test ensures that an element that SupportsSpatialNavigationFocus()
becomes a candidate in some cases, even though it has an inner focusable descendant.
</p>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="resources/snav-testharness.js"></script>
<script>
const noop = () => { };
const clickableElements = document.getElementsByClassName("clickable");
for (let elem of clickableElements) {
elem.addEventListener('click', noop);
}
document.getElementsByClassName("editable")[0].contentEditable = true;
document.getElementsByClassName("uneditable")[0].contentEditable = false;
var resultMap = [
["Down", "a"],
["Down", "b"],
["Down", "c"],
["Down", "c-child"],
["Down", "d"],
["Down", "d-child"],
["Down", "e-child2-child"],
["Down", "button"],
["Down", "f-child1"],
["Down", "g"],
["Down", "g-child"],
["Down", "h"],
["Down", "h-child"],
["Down", "i"],
["Down", "i-child"]
];
// Start from a known place.
document.getElementById("start").focus();
snav.assertFocusMoves(resultMap);
</script>
third_party/blink/web_tests/fast/spatial-navigation/snav-skippable-candidate.html 0 → 100644
View file @ 990f31c3
<!doctype html>
<style>
.cursor {
cursor: pointer;
}
.box {
display: inline-block;
position: absolute;
width: 100px;
}
div {
margin-top: 5px;
border: 1px solid black;
}
div div {
margin-top: 0;
width: 100px;
border: none;
}
* {
background: white;
}
.green {
/* To help debugging, every element in resultMap is green */
background: lightgreen;
}
.gray {
/* To help debugging, every skippable-element is gray */
background: lightgray;
}
</style>
<button class="green" id="start">Start</button>
<div class="cursor gray" id="a">A
<div>A-child1</div>
<button class="green" id="a-child2">A-child2</button>
</div>
<div class="clickable gray" id="b">B
<div>B-child1</div>
<div class="green" tabindex="0" id="b-child2">B-child2</div>
</div>
<div class="cursor gray" id="c">C
<input type="text" id="c-child1">
<div>C-child2</div>
</div>
<div class="clickable gray" id="d">D
<div class="clickable gray" id="d-child">D-child
<div class="clickable green" id="d-child-child">D-child-child
</div>
</div>
</div>
<div class="clickable gray" id="e">E
<div>
<div class="green" tabindex="0" id="e-child-child">E-child-child
</div>
</div>
</div>
<div class="cursor gray" id="f" style="position: relative; margin-top: 60px; height: 30px;">F
<!-- |f-child1| is out side of |f|. -->
<div class="green" tabindex="0" id="f-child1" style="position: absolute; top: -25px;">F-child1</div>
<div class="green" tabindex="0" id="f-child2" style="position: absolute; top: 0;">F-child2</div>
</div>
<div class="clickable gray" id="g">G
<div class="cursor gray">
<a href="" class="green" id="g-child">G-child
<div class="clickable gray">
<div class="cursor gray">
<a href="" class="green" id="g-child-child">G-child-child</a>
</div>
</div>
</a>
</div>
</div>
<p>This test ensures that an element that SupportsSpatialNavigationFocus()
doesn't become a candidate if it has an inner focusable descendant.
</p>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="resources/snav-testharness.js"></script>
<script>
const noop = () => { };
const clickableElements = document.getElementsByClassName("clickable");
for (let elem of clickableElements) {
elem.addEventListener('click', noop);
}
var resultMap = [
["Down", "a-child2"],
["Down", "b-child2"],
["Down", "c-child1"],
["Down", "d-child-child"],
["Down", "e-child-child"],
["Down", "f-child1"],
["Down", "f-child2"],
["Down", "g-child"],
["Down", "g-child-child"]
];
// Start from a known place.
document.getElementById("start").focus();
snav.assertFocusMoves(resultMap);
</script>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment