[SpatNav] Search to and from inline links' line box(es)

Background (from the CSS2 spec):
 "Although margins, borders, and padding of non-replaced
  elements do not enter into the line box calculation, they are
  still rendered around inline boxes. This means that if the
  height specified by line-height is less than the content
  height of contained boxes, backgrounds and colors of padding
  and borders may "bleed" into adjoining line boxes". [1]

Background about SpatNav:
a. SpatNav's distance formula's input is only the origin's
   (current focus') rect and a candidate's rect.
b. If two rectangles intersect, they are considered to be at
   zero (or negative) distance.

Problem:
When a link's inline box "bleeds" onto links below, then links
on the same line (to the left or right) could get unreachable
when SpatNav's distance formula favors the intersecting rects
below, see snav-line-height_less_font-size.html.

Solution:
Shrink these bleeding inline boxes to their line box's [2]
height. If a link is line broken over 2 lines, use 2x the line
box height, and so on.

[1] https://drafts.csswg.org/css2/#leading
[2] https://drafts.csswg.org/css2/#line-box

Bug: None
Change-Id: I002da7af30721de45105f5ba1330f4679a5457af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410039
Commit-Queue: Hugo Holgersson <hholgersson@fb.com>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#810252}

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

@@ -78,6 +78,10 @@ FocusCandidate::FocusCandidate(Node* node, SpatialNavigationDirection direction)
 
     visible_node = node;
     rect_in_root_frame = NodeRectInRootFrame(node);
+
+    // Remove any overlap with line boxes *above* the search origin.
+    rect_in_root_frame =
+        ShrinkInlineBoxToLineBox(*node->GetLayoutObject(), rect_in_root_frame);
   }
 
   focusable_node = node;
@@ -139,21 +143,24 @@ static bool IsRectInDirection(SpatialNavigationDirection direction,
   }
 }
 
-bool IsFragmentedInline(Node& node) {
-  const LayoutObject* layout_object = node.GetLayoutObject();
-  if (!layout_object->IsInline() || layout_object->IsAtomicInlineLevel())
-    return false;
+int LineBoxes(const LayoutObject& layout_object) {
+  if (!layout_object.IsInline() || layout_object.IsAtomicInlineLevel())
+    return 1;
 
-  // If it has empty quads, it's most likely not a fragmented text.
-  // <a><div></div></a> has for example one empty rect.
+  // If it has empty quads, it's most likely not a line broken ("fragmented")
+  // text. <a><div></div></a> has for example one empty rect.
   Vector<FloatQuad> quads;
-  layout_object->AbsoluteQuads(quads);
+  layout_object.AbsoluteQuads(quads);
   for (const FloatQuad& quad : quads) {
     if (quad.IsEmpty())
-      return false;
+      return 1;
   }
 
-  return quads.size() > 1;
+  return quads.size();
+}
+
+bool IsFragmentedInline(const LayoutObject& layout_object) {
+  return LineBoxes(layout_object) > 1;
 }
 
 FloatRect RectInViewport(const Node& node) {
@@ -768,7 +775,8 @@ PhysicalRect FirstVisibleFragment(const PhysicalRect& visibility,
                                   Iterator fragment,
                                   Iterator end) {
   while (fragment != end) {
-    PhysicalRect physical_fragment(EnclosedIntRect(fragment->BoundingBox()));
+    PhysicalRect physical_fragment =
+        PhysicalRect::EnclosingRect(fragment->BoundingBox());
     physical_fragment.Intersect(visibility);
     if (!physical_fragment.IsEmpty())
       return physical_fragment;
@@ -777,6 +785,93 @@ PhysicalRect FirstVisibleFragment(const PhysicalRect& visibility,
   return visibility;
 }
 
+LayoutUnit GetLogicalHeight(const PhysicalRect& rect,
+                            const LayoutObject& layout_object) {
+  if (layout_object.IsHorizontalWritingMode())
+    return rect.Height();
+  else
+    return rect.Width();
+}
+
+void SetLogicalHeight(PhysicalRect& rect,
+                      const LayoutObject& layout_object,
+                      LayoutUnit height) {
+  if (layout_object.IsHorizontalWritingMode())
+    rect.SetHeight(height);
+  else
+    rect.SetWidth(height);
+}
+
+LayoutUnit TallestInlineAtomicChild(const LayoutObject& layout_object) {
+  LayoutUnit max_child_size(0);
+
+  if (!layout_object.IsLayoutInline())
+    return max_child_size;
+
+  for (LayoutObject* child = layout_object.SlowFirstChild(); child;
+       child = child->NextSibling()) {
+    if (child->IsOutOfFlowPositioned())
+      continue;
+
+    if (child->IsAtomicInlineLevel()) {
+      max_child_size =
+          std::max(ToLayoutBox(child)->LogicalHeight(), max_child_size);
+    }
+  }
+
+  return max_child_size;
+}
+
+//   "Although margins, borders, and padding of non-replaced elements do not
+//    enter into the line box calculation, they are still rendered around
+//    inline boxes. This means that if the height specified by line-height is
+//    less than the content height of contained boxes, backgrounds and colors
+//    of padding and borders may "bleed" into adjoining line boxes". [1]
+// [1] https://drafts.csswg.org/css2/#leading
+// [2] https://drafts.csswg.org/css2/#line-box
+// [3] https://drafts.csswg.org/css2/#atomic-inline-level-boxes
+//
+// If an inline box is "bleeding", ShrinkInlineBoxToLineBox shrinks its
+// rect to the size of of its "line box" [2]. We need to do so because
+// "bleeding" can make links intersect vertically. We need to avoid that
+// overlap because it could make links on the same line (to the left or right)
+// unreachable as SpatNav's distance formula favors intersecting rects (on the
+// line below or above).
+PhysicalRect ShrinkInlineBoxToLineBox(const LayoutObject& layout_object,
+                                      PhysicalRect node_rect,
+                                      int line_boxes) {
+  if (!layout_object.IsInline() || layout_object.IsLayoutReplaced() ||
+      layout_object.IsButtonOrNGButton())
+    return node_rect;
+
+  // If actual line-height is bigger than the inline box, we shouldn't change
+  // anything. This is, for example, needed to not break
+  // snav-stay-in-overflow-div.html where the link's inline box doesn't fill
+  // the entire line box vertically.
+  LayoutUnit line_height = layout_object.StyleRef().ComputedLineHeightAsFixed();
+  LayoutUnit current_height = GetLogicalHeight(node_rect, layout_object);
+  if (line_height >= current_height)
+    return node_rect;
+
+  // Handle focusables like <a><img><a> (a LayoutInline that carries atomic
+  // inline boxes [3]). Despite a small line-height on the <a>, <a>'s line box
+  // will still fit the <img>.
+  line_height = std::max(TallestInlineAtomicChild(layout_object), line_height);
+  if (line_height >= current_height)
+    return node_rect;
+
+  // Cap the box at its line height to avoid overlapping inline links.
+  // Links can overlap vertically when CSS line-height < font-size, see
+  // snav-line-height_less_font-size.html.
+  line_boxes = line_boxes == -1 ? LineBoxes(layout_object) : line_boxes;
+  line_height = line_height * line_boxes;
+  if (line_height >= current_height)
+    return node_rect;
+  SetLogicalHeight(node_rect, layout_object, line_height);
+  return node_rect;
+}
+
+// TODO(crbug.com/1131419): Add tests and support for other writing-modes.
 PhysicalRect SearchOriginFragment(const PhysicalRect& visible_part,
                                   const LayoutObject& fragmented,
                                   const SpatialNavigationDirection direction) {
@@ -832,10 +927,17 @@ PhysicalRect SearchOrigin(const PhysicalRect& viewport_rect_of_root_frame,
     PhysicalRect visible_part =
         Intersection(box_in_root_frame, viewport_rect_of_root_frame);
 
-    if (IsFragmentedInline(*focus_node)) {
-      return SearchOriginFragment(visible_part, *focus_node->GetLayoutObject(),
-                                  direction);
+    const LayoutObject* const layout_object = focus_node->GetLayoutObject();
+    if (IsFragmentedInline(*layout_object)) {
+      visible_part =
+          SearchOriginFragment(visible_part, *layout_object, direction);
     }
+
+    // Remove any overlap with line boxes *below* the search origin.
+    // The search origin is always only one line (because if |focus_node| is
+    // line broken, SearchOriginFragment picks the first or last line's box).
+    visible_part = ShrinkInlineBoxToLineBox(*layout_object, visible_part, 1);
+
     return visible_part;
   }
 

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

@@ -39,7 +39,7 @@ constexpr double kMaxDistance = std::numeric_limits<double>::max();
 
 CORE_EXPORT bool IsSpatialNavigationEnabled(const LocalFrame*);
 
-struct FocusCandidate {
+struct CORE_EXPORT FocusCandidate {
   STACK_ALLOCATED();
 
  public:
@@ -64,7 +64,9 @@ struct FocusCandidate {
 };
 
 CORE_EXPORT bool HasRemoteFrame(const Node*);
-CORE_EXPORT bool IsFragmentedInline(Node& node);
+CORE_EXPORT int LineBoxes(const LayoutObject& layout_object);
+CORE_EXPORT
+bool IsFragmentedInline(const LayoutObject& layout_object);
 CORE_EXPORT FloatRect RectInViewport(const Node&);
 CORE_EXPORT bool IsOffscreen(const Node*);
 CORE_EXPORT bool IsUnobscured(const FocusCandidate&);
@@ -86,6 +88,12 @@ CORE_EXPORT PhysicalRect RootViewport(const LocalFrame*);
 PhysicalRect StartEdgeForAreaElement(const HTMLAreaElement&,
                                      SpatialNavigationDirection);
 HTMLFrameOwnerElement* FrameOwnerElement(const FocusCandidate&);
+
+CORE_EXPORT PhysicalRect
+ShrinkInlineBoxToLineBox(const LayoutObject& layout_object,
+                         PhysicalRect visible_part,
+                         int line_boxes = -1);
+
 CORE_EXPORT PhysicalRect
 SearchOriginFragment(const PhysicalRect& visible_part,
                      const LayoutObject& fragmented,

third_party/blink/web_tests/fast/spatial-navigation/snav-line-height_less_font-size.html

+<!doctype html>
+<p>CSS's <span style="font-family: monospace">line-height</span> can make the links' line boxes overlap vertically.
+SpatNav needs to handle these overlapping focusables.</p>
+
+<p>This test looks a bit artificial because the Ahem font's "glyphs" are squares that fill the
+entire line height. Real-life fonts don't; they have varying white space above/under the glyphs.
+To reduce the white space, web developers may use <span style="font-family: monospace">line-height < font-size</span>
+(which causes overlapping focusables...).</p>
+
+<p>Test 1 (the red and blue links intersect the purple link below):</p>
+
+<div style="font: 16px Ahem; line-height: 14px;">
+<a href="www" id="a" style="color: green">Aaaaaaaaaa</a> <a href="www" id="b" style="color: red">unreachable</a>, <a href="www" id="c">cccccccc ccc ccccccc</a><br/>
+blabla, <a href="www" id="d" style="color: purple">ddd ddd dddddd dddddd ddd ddd ddd</a> bla.
+</div>
+
+<p>Test 2 (the red and blue links intersect the purple link above):</p>
+
+<div style="font: 16px Ahem; line-height: 14px;">
+blabla, <a href="www" id="d2" style="color: purple">ddd ddd dddddd dddddd ddd ddd ddd</a> bla.
+<a href="www" id="a2" style="color: green">Aaaaaaaaaa</a> <a href="www" id="b2" style="color: red">unreachable</a>, <a href="www" id="c2">cccccccc ccc ccccccc</a><br/>
+</div>
+
+<p><em>Manual test instruction: Ensure that all links are reachable.</em></p>
+
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="resources/snav-testharness.js"></script>
+<script>
+  var resultMap = [
+    ["Down", "c"],
+    ["Left", "b"],
+    ["Left", "a"],
+    ["Right", "b"],
+    ["Right", "c"],
+    ["Down", "d"],
+
+    ["Down", "d2"],
+    ["Down", "c2"],
+    ["Left", "b2"],
+    ["Left", "a2"],
+    ["Right", "b2"],
+    ["Right", "c2"],
+
+    ["Up", "d2"],
+    ["Up", "d"],
+    ["Up", "c"],
+  ];
+  snav.assertFocusMoves(resultMap);
+</script>