[SpatNav] Account for clipping in z-ordering checks

The spatial navigation algorithm attempts to prioritize targets fully
contained within another target. However, the current implementation has
a bug because it uses unclipped rects to determine containment.

This CL fixes the issue by using MapToVisualRectInAncestorSpace which
does account for clipping.

Small cleanup: Remove the |ignore_border| option since it's always true.

Bug: 957240
Change-Id: Id032bb9d354ec06d075b2de6bd53a8799f7a8af1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1586660Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654878}

third_party/blink/renderer/core/page/spatial_navigation.cc

@@ -66,7 +66,7 @@ FocusCandidate::FocusCandidate(Node* node, SpatialNavigationDirection direction)
       return;
 
     visible_node = node;
-    rect_in_root_frame = NodeRectInRootFrame(node, true /* ignore border */);
+    rect_in_root_frame = NodeRectInRootFrame(node);
   }
 
   focusable_node = node;
@@ -377,13 +377,28 @@ bool CanScrollInDirection(const LocalFrame* frame,
   }
 }
 
-LayoutRect NodeRectInRootFrame(const Node* node, bool ignore_border) {
+LayoutRect NodeRectInRootFrame(const Node* node) {
   DCHECK(node);
   DCHECK(node->GetLayoutObject());
   DCHECK(!node->GetDocument().View()->NeedsLayout());
 
-  LayoutRect rect = node->GetDocument().GetFrame()->View()->ConvertToRootFrame(
-      node->BoundingBox());
+  LayoutObject* object = node->GetLayoutObject();
+
+  LayoutRect rect = LayoutRect(object->LocalBoundingBoxRectForAccessibility());
+
+  // Inset the bounding box by the border.
+  // TODO(bokan): As far as I can tell, this is to work around empty iframes
+  // that have a border. It's unclear if that's still useful.
+  rect.Move(node->GetLayoutObject()->Style()->BorderLeftWidth(),
+            node->GetLayoutObject()->Style()->BorderTopWidth());
+  rect.SetWidth(LayoutUnit(
+      rect.Width() - node->GetLayoutObject()->Style()->BorderLeftWidth() -
+      node->GetLayoutObject()->Style()->BorderRightWidth()));
+  rect.SetHeight(LayoutUnit(
+      rect.Height() - node->GetLayoutObject()->Style()->BorderTopWidth() -
+      node->GetLayoutObject()->Style()->BorderBottomWidth()));
+
+  object->MapToVisualRectInAncestorSpace(/*ancestor=*/nullptr, rect);
 
   // Ensure the rect isn't empty. This can happen in some cases as the bounding
   // box is made up of the corners of multiple child elements. If the first
@@ -391,19 +406,6 @@ LayoutRect NodeRectInRootFrame(const Node* node, bool ignore_border) {
   // be empty. Ensure its not empty so intersections with the root frame don't
   // lie about being off-screen.
   rect.UniteEvenIfEmpty(LayoutRect(rect.Location(), LayoutSize(1, 1)));
-
-  // For authors that use border instead of outline in their CSS, we compensate
-  // by ignoring the border when calculating the rect of the focused element.
-  if (ignore_border) {
-    rect.Move(node->GetLayoutObject()->Style()->BorderLeftWidth(),
-              node->GetLayoutObject()->Style()->BorderTopWidth());
-    rect.SetWidth(LayoutUnit(
-        rect.Width() - node->GetLayoutObject()->Style()->BorderLeftWidth() -
-        node->GetLayoutObject()->Style()->BorderRightWidth()));
-    rect.SetHeight(LayoutUnit(
-        rect.Height() - node->GetLayoutObject()->Style()->BorderTopWidth() -
-        node->GetLayoutObject()->Style()->BorderBottomWidth()));
-  }
   return rect;
 }
 
@@ -674,7 +676,7 @@ LayoutRect SearchOrigin(const LayoutRect viewport_rect_of_root_frame,
     if (area_element)
       return StartEdgeForAreaElement(*area_element, direction);
 
-    LayoutRect box_in_root_frame = NodeRectInRootFrame(focus_node, true);
+    LayoutRect box_in_root_frame = NodeRectInRootFrame(focus_node);
     return Intersection(box_in_root_frame, viewport_rect_of_root_frame);
   }
 
@@ -682,7 +684,7 @@ LayoutRect SearchOrigin(const LayoutRect viewport_rect_of_root_frame,
   while (container) {
     if (!IsOffscreen(container)) {
       // The first scroller that encloses focus and is [partially] visible.
-      LayoutRect box_in_root_frame = NodeRectInRootFrame(container, true);
+      LayoutRect box_in_root_frame = NodeRectInRootFrame(container);
       return OppositeEdge(direction, Intersection(box_in_root_frame,
                                                   viewport_rect_of_root_frame));
     }

third_party/blink/renderer/core/page/spatial_navigation.h

@@ -85,8 +85,7 @@ bool AreElementsOnSameLine(const FocusCandidate& first_candidate,
 double ComputeDistanceDataForNode(SpatialNavigationDirection,
                                   const FocusCandidate& current_interest,
                                   const FocusCandidate& candidate);
-CORE_EXPORT LayoutRect NodeRectInRootFrame(const Node*,
-                                           bool ignore_border = false);
+CORE_EXPORT LayoutRect NodeRectInRootFrame(const Node*);
 CORE_EXPORT LayoutRect OppositeEdge(SpatialNavigationDirection side,
                                     const LayoutRect& box,
                                     LayoutUnit thickness = LayoutUnit());

third_party/blink/renderer/core/page/spatial_navigation_controller.cc

@@ -82,8 +82,8 @@ static void ConsiderForBestCandidate(SpatialNavigationDirection direction,
        candidate.rect_in_root_frame.IsEmpty()))
     return;
 
-  // Ignore off-screen focusables that are not exposed after one "scroll step"
-  // in the direction.
+  // Ignore off-screen focusables, if there's nothing in the direction we'll
+  // scroll until they come on-screen.
   if (candidate.is_offscreen)
     return;
 

third_party/blink/renderer/core/page/spatial_navigation_test.cc

@@ -198,7 +198,7 @@ TEST_F(SpatialNavigationTest, StartAtVisibleFocusedElement) {
 
   EXPECT_EQ(SearchOrigin(RootViewport(&GetFrame()), b,
                          SpatialNavigationDirection::kDown),
-            NodeRectInRootFrame(b, true));
+            NodeRectInRootFrame(b));
 }
 
 TEST_F(SpatialNavigationTest, StartAtVisibleFocusedScroller) {
@@ -220,7 +220,7 @@ TEST_F(SpatialNavigationTest, StartAtVisibleFocusedScroller) {
   Element* scroller = GetDocument().getElementById("scroller");
   EXPECT_EQ(SearchOrigin(RootViewport(&GetFrame()), scroller,
                          SpatialNavigationDirection::kDown),
-            NodeRectInRootFrame(scroller, true));
+            NodeRectInRootFrame(scroller));
 }
 
 TEST_F(SpatialNavigationTest, StartAtVisibleFocusedIframe) {
@@ -241,7 +241,7 @@ TEST_F(SpatialNavigationTest, StartAtVisibleFocusedIframe) {
   Element* iframe = GetDocument().getElementById("iframe");
   EXPECT_EQ(SearchOrigin(RootViewport(&GetFrame()), iframe,
                          SpatialNavigationDirection::kDown),
-            NodeRectInRootFrame(iframe, true));
+            NodeRectInRootFrame(iframe));
 }
 
 TEST_F(SpatialNavigationTest, StartAtTopWhenGoingDownwardsWithoutFocus) {
@@ -317,7 +317,7 @@ TEST_F(SpatialNavigationTest, StartAtContainersEdge) {
 
   Element* b = GetDocument().getElementById("b");
   const Element* container = GetDocument().getElementById("container");
-  const LayoutRect container_box = NodeRectInRootFrame(container, true);
+  const LayoutRect container_box = NodeRectInRootFrame(container);
 
   // TODO(crbug.com/889840):
   // VisibleBoundsInVisualViewport does not (yet) take div-clipping into
@@ -433,7 +433,7 @@ TEST_F(SpatialNavigationTest, PartiallyVisible) {
 
   EXPECT_FALSE(IsOffscreen(b));  // <button> is not completely offscreen.
 
-  LayoutRect button_in_root_frame = NodeRectInRootFrame(b, true);
+  LayoutRect button_in_root_frame = NodeRectInRootFrame(b);
 
   EXPECT_EQ(SearchOrigin(RootViewport(&GetFrame()), b,
                          SpatialNavigationDirection::kUp),
@@ -442,7 +442,7 @@ TEST_F(SpatialNavigationTest, PartiallyVisible) {
   // Do some scrolling.
   ScrollableArea* root_scroller = GetDocument().View()->GetScrollableArea();
   root_scroller->SetScrollOffset(ScrollOffset(0, 600), kProgrammaticScroll);
-  LayoutRect button_after_scroll = NodeRectInRootFrame(b, true);
+  LayoutRect button_after_scroll = NodeRectInRootFrame(b);
   ASSERT_NE(button_in_root_frame,
             button_after_scroll);  // As we scrolled, the
                                    // <button>'s position in
@@ -559,7 +559,7 @@ TEST_F(SpatialNavigationTest, PartiallyVisibleIFrame) {
   EXPECT_TRUE(IsOffscreen(child_element));         // Completely offscreen.
   EXPECT_FALSE(IsOffscreen(enclosing_container));  // Partially visible.
 
-  LayoutRect iframe = NodeRectInRootFrame(enclosing_container, true);
+  LayoutRect iframe = NodeRectInRootFrame(enclosing_container);
 
   // When searching downwards we start at activeElement's
   // container's (here: the iframe's) topmost visible edge.

third_party/blink/web_tests/virtual/focusless-spat-nav/fast/spatial-navigation/focusless/snav-focusless-account-for-clip.html

+<!DOCTYPE html>
+<script src="../../../../../resources/testharness.js"></script>
+<script src="../../../../../resources/testharnessreport.js"></script>
+<script src="file:///gen/layout_test_data/mojo/public/js/mojo_bindings.js"></script>
+<script src="file:///gen/third_party/blink/public/mojom/page/spatial_navigation.mojom.js"></script>
+<script src="../../../../../fast/spatial-navigation/resources/mock-snav-service.js"></script>
+<script src="../../../../../fast/spatial-navigation/resources/snav-testharness.js"></script>
+
+<style>
+  div {
+    width: 130px;
+    height: 130px;
+    border: 1px solid black;
+  }
+
+  #clip {
+    margin: 5px;
+    width: 100px;
+    height: 100px;
+    overflow: hidden;
+    background-color: lightgrey;
+  }
+
+  #target {
+    margin: 20px;
+    height: 50px;
+  }
+
+  pre {
+    position: absolute;
+    top: 400px;
+  }
+
+</style>
+<div tabindex="0">
+  Outer Target
+  <div id="clip" tabindex="0">
+    Clip
+    <div id="target" tabindex="0">Inner Target</div>
+  </div>
+</div>
+
+<script>
+  const target = document.getElementById("target");
+
+  // This test checks that the spatial navigation overlap testing works
+  // correctly in the presence of clipping. The spatial navigation algorithm
+  // attempts to prioritize the inner element when one valid target is fully
+  // contained by another valid target. This test ensures the inner is
+  // considered fully contained even if it has clipped overflow that would
+  // spill outside the outer target.
+  test(() => {
+    assert_true(!!window.internals);
+    snav.triggerMove('Down');
+    assert_equals(window.internals.interestedElement,
+                  target,
+                  "Expected interest to move to |target| element.");
+  }, "Navigation to fully contained but clipped element.");
+</script>