Reland [SpatNav] Fix IsOffscreen computation

Reland Note: Original CL actually fixed the test on Mac which caused it
to mis-match the failing expectation. The test remained broken on other
platforms because it didn't account for scroll animations. This CL fixes
the test and updates the expectation to passing, original CL in PS#1.

The IsOffscreen computation was comparing the FrameView's
GetScrollableArea()->VisibleContentRect, which is be the content rect in
the _root scroller_, with the object's VisibleRectInDocument, which is
in document coordinates. This breaks when a non-default root scroller is
active.

I've rewritten this method to be cleaner. I've kept the separate mapping
up to the local frame first using VisualRectInDocument rather than
mapping directly from local to root frame since VisualRectInDocument has
some specific logic in subclasses that uses a different local rect
depending on the LayoutObject type; I couldn't find an equivalent way of
getting an equivalent rect but in local coordinates and several tests
depend on this (e.g. how line boxes are collected in a LayoutInline).

TBR=eae@chromium.org

Bug: 949406
Change-Id: Iec0f5a27bd242c537db231cfb44b39d535fc579f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1565724
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650477}

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

@@ -38,6 +38,7 @@
 #include "third_party/blink/renderer/core/html/html_image_element.h"
 #include "third_party/blink/renderer/core/html_names.h"
 #include "third_party/blink/renderer/core/layout/layout_box.h"
+#include "third_party/blink/renderer/core/layout/layout_inline.h"
 #include "third_party/blink/renderer/core/layout/layout_view.h"
 #include "third_party/blink/renderer/core/page/frame_tree.h"
 #include "third_party/blink/renderer/core/page/page.h"
@@ -145,30 +146,42 @@ bool IsOffscreen(const Node* node) {
 
   DCHECK(!frame_view->NeedsLayout());
 
-  LayoutRect frame_viewport(
-      frame_view->GetScrollableArea()->VisibleContentRect());
-
-  LayoutObject* layout_object = node->GetLayoutObject();
-  if (!layout_object)
-    return true;
-
-  LayoutRect rect(layout_object->VisualRectInDocument());
-  if (rect.IsEmpty())
+  LayoutObject* object = node->GetLayoutObject();
+  if (!object)
     return true;
 
-  // A document always intersects with its frame's viewport.
-  if (node != node->GetDocument() && !frame_viewport.Intersects(rect))
-    return true;
-
-  // Now we know that the node is visible in the its own frame's viewport (it is
-  // not clipped by a scrollable div). That is, we've taken "element-clipping"
-  // into account - now we only need to ensure that this node isn't clipped by
-  // a frame.
-  if (auto* document = DynamicTo<Document>(node))
-    node = document->body();
-  if (node && node->IsElementNode())
-    return ToElement(*node).VisibleBoundsInVisualViewport().IsEmpty();
-  return true;
+  // Get the rect in the object's own frame. We use VisualRectInDocument for
+  // legacy reasons, it has some special cases for inlines that we'd like to
+  // preserve (or at least break layout tests). Because of that, we have to
+  // manually convert into frame coordinates and then clip to the frame.
+  LayoutRect rect_in_frame =
+      object->IsLayoutView()
+          ? object->VisualRectInDocument()
+          : frame_view->DocumentToFrame(object->VisualRectInDocument());
+  LayoutRect frame_rect =
+      LayoutRect(LayoutPoint(), LayoutSize(frame_view->Size()));
+  rect_in_frame.Intersect(frame_rect);
+
+  // Now convert from the local frame to the root frame's coordinate space.
+  // This will already apply clipping along the way.
+  LayoutRect rect_in_root_frame = rect_in_frame;
+  const LayoutBoxModelObject* ancestor = nullptr;
+  frame_view->GetLayoutView()->MapToVisualRectInAncestorSpace(
+      ancestor, rect_in_root_frame,
+      kUseTransforms | kTraverseDocumentBoundaries, kDefaultVisualRectFlags);
+
+  // Now convert to the visual viewport which will account for pinch zoom.
+  VisualViewport& visual_viewport =
+      object->GetDocument().GetPage()->GetVisualViewport();
+  FloatRect rect_in_viewport =
+      visual_viewport.RootFrameToViewport(FloatRect(rect_in_root_frame));
+
+  // RootFrameToViewport doesn't clip so manually apply the viewport clip here.
+  FloatRect viewport_rect =
+      FloatRect(FloatPoint(), FloatSize(visual_viewport.Size()));
+  rect_in_viewport.Intersect(viewport_rect);
+
+  return rect_in_viewport.IsEmpty();
 }
 
 // As IsOffscreen() but returns visibility through the |node|'s frame's viewport

third_party/blink/web_tests/fast/spatial-navigation/snav-root-scroller.html

+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="resources/snav-testharness.js"></script>
+
+<style>
+  ::-webkit-scrollbar {
+    display:none;
+  }
+
+  .box {
+    width: 100px;
+    height: 100px;
+    background-color: green;
+    margin: 5px;
+    border: 1px solid black;
+  }
+
+  #scroller {
+    width: 100%;
+    height: 100%;
+    overflow: auto;
+  }
+
+  body,html {
+    width: 100%;
+    height: 100%;
+    margin: 0;
+  }
+
+  html {
+    overflow: hidden;
+  }
+
+  #spacer {
+    height: 2000px;
+  }
+</style>
+
+<div id="scroller">
+  <div id="spacer"></div>
+  <div id="A" class="box" tabindex="0"></div>
+  <div id="B" class="box" tabindex="0"></div>
+</div>
+
+<script>
+  // This test checks that focusless mode allows entering focus into an element
+  // with the enter key and exiting it with the escape key.
+  const scroller = document.getElementById("scroller");
+  const a = document.getElementById("A");
+  const b = document.getElementById("B");
+
+  snav.assertSnavEnabledAndTestable();
+
+  // Fully scroll the scroller.
+  scroller.scrollTop = 100000;
+
+  // Start with |B| interested.
+  b.focus();
+
+  t = async_test("Spatial Navigation within a root scroller");
+
+  // We need two frames before we can be sure the root scroller is activated
+  // (since script will run before the frame lifecycle is updated).
+  requestAnimationFrame( () => {
+    requestAnimationFrame( () => {
+      t.step(() => {
+          assert_equals(window.internals.effectiveRootScroller(document), scroller);
+          assert_equals(window.internals.interestedElement,
+                        b,
+                        "|B| starts off interested.");
+
+          // Down should scroll the visual viewport, since there's no targets
+          // availabe on screen.
+          snav.triggerMove('Up');
+
+          assert_equals(window.internals.interestedElement,
+                        a,
+                        "Navigation should move interest to |A|");
+          t.done();
+      });
+    });
+  });
+</script>

third_party/blink/web_tests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction-expected.txt

@@ -6,4 +6,4 @@ TEST COMPLETE
 
 PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
 PASS true is true
-FAIL gFocusedDocument.activeElement.getAttribute("id") should be end. Was start.
+PASS gFocusedDocument.activeElement.getAttribute("id") is "end"

third_party/blink/web_tests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction.html

@@ -43,6 +43,7 @@
       testRunner.dumpAsText();
       testRunner.overridePreference("WebKitTabToLinksPreferenceKey", 1);
       testRunner.overridePreference("WebKitSpatialNavigationEnabled", 1);
+      internals.settings.setScrollAnimatorEnabled(false);
       testRunner.waitUntilDone();
     }
 
@@ -91,7 +92,7 @@
   </head>
   <body id="some-content" xmlns="http://www.w3.org/1999/xhtml">
     <a id="start" href="a">Start</a>
-    <div style='margin-top:100000000px'>
+    <div style='margin-top:100000px'>
         <a id="end" href="a">End</a>
     </div>
     <div id="console"></div>