[LayoutNG] Store NGPhysicalBoxFragment in HitTestResult.

It would be tricky to locate the right fragment in PositionForPoint(),
in cases where a node generates more than one fragment. A much easier
solution is to remember which fragment we hit.

Move fragment item specific code out of LayoutNGBlockFlowMixin's
PositionForPoint() into the new NGPhysicalBoxFragment method
PositionForPoint(). Also call this method from
HitTestResult::GetPosition() if we stored a fragment during hit testing.
For now we'll only do that for inline formatting contexts, since
NGPhysicalBoxFragment's PositionForPoint() doesn't support block
children yeyet.

Bug: 829028, 1043787
Change-Id: Ifc9f3245f832bdebd2a20dbd6ebe73ca3289d80b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485219Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819253}

third_party/blink/renderer/core/editing/editing_utilities.cc

@@ -1348,6 +1348,25 @@ PositionWithAffinity PositionRespectingEditingBoundary(
   return target_object->PositionForPoint(selection_end_point);
 }
 
+PositionWithAffinity AdjustForEditingBoundary(
+    const PositionWithAffinity& position_with_affinity) {
+  if (position_with_affinity.IsNull())
+    return position_with_affinity;
+  const Position& position = position_with_affinity.GetPosition();
+  const Node& node = *position.ComputeContainerNode();
+  if (HasEditableStyle(node))
+    return position_with_affinity;
+  const Position& forward =
+      MostForwardCaretPosition(position, kCanCrossEditingBoundary);
+  if (HasEditableStyle(*forward.ComputeContainerNode()))
+    return PositionWithAffinity(forward);
+  const Position& backward =
+      MostBackwardCaretPosition(position, kCanCrossEditingBoundary);
+  if (HasEditableStyle(*backward.ComputeContainerNode()))
+    return PositionWithAffinity(backward);
+  return position_with_affinity;
+}
+
 Position ComputePositionForNodeRemoval(const Position& position,
                                        const Node& node) {
   if (position.IsNull())

third_party/blink/renderer/core/editing/editing_utilities.h

@@ -261,6 +261,17 @@ PositionWithAffinity PositionRespectingEditingBoundary(
     const Position&,
     const PhysicalOffset& local_point,
     Node* target_node);
+
+// Move specified position to start/end of non-editable region.
+// If it can be found, we prefer a visually equivalent position that is
+// editable.
+// See also LayoutObject::CreatePositionWithAffinity()
+// Example:
+//  <editable><non-editable>|abc</non-editable></editable>
+//  =>
+//  <editable>|<non-editable>abc</non-editable></editable>
+PositionWithAffinity AdjustForEditingBoundary(const PositionWithAffinity&);
+
 Position ComputePositionForNodeRemoval(const Position&, const Node&);
 
 // TODO(editing-dev): These two functions should be eliminated.

third_party/blink/renderer/core/layout/hit_test_result.cc

@@ -41,6 +41,7 @@
 #include "third_party/blink/renderer/core/input_type_names.h"
 #include "third_party/blink/renderer/core/layout/layout_block.h"
 #include "third_party/blink/renderer/core/layout/layout_image.h"
+#include "third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.h"
 #include "third_party/blink/renderer/core/scroll/scrollbar.h"
@@ -73,6 +74,7 @@ HitTestResult::HitTestResult(const HitTestResult& other)
       local_point_(other.LocalPoint()),
       inner_url_element_(other.URLElement()),
       scrollbar_(other.GetScrollbar()),
+      box_fragment_(other.box_fragment_),
       is_over_embedded_content_view_(other.IsOverEmbeddedContentView()),
       canvas_region_id_(other.CanvasRegionId()) {
   // Only copy the NodeSet in case of list hit test.
@@ -100,6 +102,7 @@ bool HitTestResult::EqualForCacheability(const HitTestResult& other) const {
          local_point_ == other.LocalPoint() &&
          inner_url_element_ == other.URLElement() &&
          scrollbar_ == other.GetScrollbar() &&
+         box_fragment_ == other.box_fragment_ &&
          is_over_embedded_content_view_ == other.IsOverEmbeddedContentView();
 }
 
@@ -117,6 +120,8 @@ void HitTestResult::PopulateFromCachedResult(const HitTestResult& other) {
   local_point_ = other.LocalPoint();
   inner_url_element_ = other.URLElement();
   scrollbar_ = other.GetScrollbar();
+  box_fragment_ = other.box_fragment_;
+
   is_over_embedded_content_view_ = other.IsOverEmbeddedContentView();
   cacheable_ = other.cacheable_;
   canvas_region_id_ = other.CanvasRegionId();
@@ -138,6 +143,14 @@ void HitTestResult::Trace(Visitor* visitor) const {
   visitor->Trace(list_based_test_result_);
 }
 
+void HitTestResult::SetNodeAndPosition(
+    Node* node,
+    scoped_refptr<const NGPhysicalBoxFragment> box_fragment,
+    const PhysicalOffset& position) {
+  SetNodeAndPosition(node, position);
+  box_fragment_ = std::move(box_fragment);
+}
+
 PositionWithAffinity HitTestResult::GetPosition() const {
   if (!inner_possibly_pseudo_node_)
     return PositionWithAffinity();
@@ -149,6 +162,10 @@ PositionWithAffinity HitTestResult::GetPosition() const {
     return PositionWithAffinity(MostForwardCaretPosition(
         Position(inner_node_, PositionAnchorType::kBeforeChildren)));
   }
+  // TODO(mstensho): Remove check for Items() when the code is ready
+  // for it. Right now it doesn't support block children.
+  if (box_fragment_ && box_fragment_->Items())
+    return box_fragment_->PositionForPoint(LocalPoint());
   return layout_object->PositionForPoint(LocalPoint());
 }
 

third_party/blink/renderer/core/layout/hit_test_result.h

@@ -45,6 +45,7 @@ class HTMLMediaElement;
 class Image;
 class KURL;
 class MediaStreamDescriptor;
+class NGPhysicalBoxFragment;
 class Node;
 class LayoutObject;
 class Region;
@@ -119,6 +120,9 @@ class CORE_EXPORT HitTestResult {
     local_point_ = p;
     SetInnerNode(node);
   }
+  void SetNodeAndPosition(Node*,
+                          scoped_refptr<const NGPhysicalBoxFragment>,
+                          const PhysicalOffset&);
 
   PositionWithAffinity GetPosition() const;
   LayoutObject* GetLayoutObject() const;
@@ -206,6 +210,7 @@ class CORE_EXPORT HitTestResult {
   // For non-URL, this is the enclosing that triggers navigation.
   Member<Element> inner_url_element_;
   Member<Scrollbar> scrollbar_;
+  scoped_refptr<const NGPhysicalBoxFragment> box_fragment_;
   // Returns true if we are over a EmbeddedContentView (and not in the
   // border/padding area of a LayoutEmbeddedContent for example).
   bool is_over_embedded_content_view_;

third_party/blink/renderer/core/layout/ng/layout_ng_block_flow_mixin.cc

@@ -246,33 +246,6 @@ bool LayoutNGBlockFlowMixin<Base>::NodeAtPoint(
                                       accumulated_offset, action);
 }
 
-// Move specified position to start/end of non-editable region.
-// If it can be found, we prefer a visually equivalent position that is
-// editable.
-// See also LayoutObject::CreatePositionWithAffinity()
-// Example:
-//  <editable><non-editable>|abc</non-editable></editable>
-//  =>
-//  <editable>|<non-editable>abc</non-editable></editable>
-static PositionWithAffinity AdjustForEditingBoundary(
-    const PositionWithAffinity& position_with_affinity) {
-  if (position_with_affinity.IsNull())
-    return position_with_affinity;
-  const Position& position = position_with_affinity.GetPosition();
-  const Node& node = *position.ComputeContainerNode();
-  if (HasEditableStyle(node))
-    return position_with_affinity;
-  const Position& forward =
-      MostForwardCaretPosition(position, kCanCrossEditingBoundary);
-  if (HasEditableStyle(*forward.ComputeContainerNode()))
-    return PositionWithAffinity(forward);
-  const Position& backward =
-      MostBackwardCaretPosition(position, kCanCrossEditingBoundary);
-  if (HasEditableStyle(*backward.ComputeContainerNode()))
-    return PositionWithAffinity(backward);
-  return position_with_affinity;
-}
-
 template <typename Base>
 PositionWithAffinity LayoutNGBlockFlowMixin<Base>::PositionForPoint(
     const PhysicalOffset& point) const {
@@ -295,17 +268,7 @@ PositionWithAffinity LayoutNGBlockFlowMixin<Base>::PositionForPoint(
             paint_fragment->PositionForPoint(point_in_contents))
       return AdjustForEditingBoundary(position);
   } else if (const NGPhysicalBoxFragment* fragment = CurrentFragment()) {
-    if (const NGFragmentItems* items = fragment->Items()) {
-      // The given offset is relative to this |LayoutBlockFlow|. Convert to the
-      // contents offset.
-      PhysicalOffset point_in_contents = point;
-      Base::OffsetForContents(point_in_contents);
-      NGInlineCursor cursor(*items);
-      if (const PositionWithAffinity position =
-              cursor.PositionForPointInInlineFormattingContext(
-                  point_in_contents, *fragment))
-        return AdjustForEditingBoundary(position);
-    }
+    return fragment->PositionForPoint(point);
   }
 
   return Base::CreatePositionWithAffinity(0);

third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h"
 
+#include "third_party/blink/renderer/core/editing/editing_utilities.h"
 #include "third_party/blink/renderer/core/editing/position_with_affinity.h"
 #include "third_party/blink/renderer/core/layout/geometry/writing_mode_converter.h"
 #include "third_party/blink/renderer/core/layout/layout_block_flow.h"
@@ -728,6 +729,22 @@ void NGPhysicalBoxFragment::AddSelfOutlineRects(
   // LayoutBlockFlow::AddOutlineRects?
 }
 
+PositionWithAffinity NGPhysicalBoxFragment::PositionForPoint(
+    PhysicalOffset point) const {
+  if (IsScrollContainer())
+    point += PhysicalOffset(PixelSnappedScrolledContentOffset());
+  if (const NGFragmentItems* items = Items()) {
+    NGInlineCursor cursor(*items);
+    if (const PositionWithAffinity position =
+            cursor.PositionForPointInInlineFormattingContext(point, *this))
+      return AdjustForEditingBoundary(position);
+  }
+  // TODO(mstensho): Add support for block children.
+  if (layout_object_->GetNode())
+    return layout_object_->CreatePositionWithAffinity(0);
+  return PositionWithAffinity();
+}
+
 UBiDiLevel NGPhysicalBoxFragment::BidiLevel() const {
   // TODO(xiaochengh): Make the implementation more efficient.
   DCHECK(IsInline());

third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h

@@ -230,6 +230,8 @@ class CORE_EXPORT NGPhysicalBoxFragment final
                            NGOutlineType include_block_overflows,
                            Vector<PhysicalRect>* outline_rects) const;
 
+  PositionWithAffinity PositionForPoint(PhysicalOffset) const;
+
   UBiDiLevel BidiLevel() const;
 
   PhysicalBoxSides SidesToInclude() const {

third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc

@@ -1831,10 +1831,11 @@ BoxPainterBase::FillLayerInfo NGBoxFragmentPainter::GetFillLayerInfo(
 
 bool NGBoxFragmentPainter::HitTestContext::AddNodeToResult(
     Node* node,
+    const NGPhysicalBoxFragment* box_fragment,
     const PhysicalRect& bounds_rect,
     const PhysicalOffset& offset) const {
   if (node && !result->InnerNode())
-    result->SetNodeAndPosition(node, location.Point() - offset);
+    result->SetNodeAndPosition(node, box_fragment, location.Point() - offset);
   return result->AddNodeToListBasedTestResult(node, location, bounds_rect) ==
          kStopHitTesting;
 }
@@ -1931,18 +1932,19 @@ bool NGBoxFragmentPainter::NodeAtPoint(const HitTestContext& hit_test,
       // See http://crbug.com/1043471
       if (box_item_ && box_item_->IsInlineBox()) {
         if (hit_test.AddNodeToResult(
-                fragment.NodeForHitTest(), bounds_rect,
+                fragment.NodeForHitTest(), &box_fragment_, bounds_rect,
                 physical_offset - box_item_->OffsetInContainerBlock()))
           return true;
       } else if (paint_fragment_ &&
                  paint_fragment_->PhysicalFragment().IsInline()) {
         if (hit_test.AddNodeToResult(
-                fragment.NodeForHitTest(), bounds_rect,
+                fragment.NodeForHitTest(), /* box_fragment */ nullptr,
+                bounds_rect,
                 physical_offset - paint_fragment_->OffsetInContainerBlock()))
           return true;
       } else {
-        if (hit_test.AddNodeToResult(fragment.NodeForHitTest(), bounds_rect,
-                                     physical_offset))
+        if (hit_test.AddNodeToResult(fragment.NodeForHitTest(), &box_fragment_,
+                                     bounds_rect, physical_offset))
           return true;
       }
     }
@@ -2013,7 +2015,7 @@ bool NGBoxFragmentPainter::HitTestTextFragment(
     return false;
 
   return hit_test.AddNodeToResult(
-      text_fragment.NodeForHitTest(), rect,
+      text_fragment.NodeForHitTest(), /* box_fragment */ nullptr, rect,
       physical_offset - text_paint_fragment->OffsetInContainerBlock());
 }
 
@@ -2039,8 +2041,8 @@ bool NGBoxFragmentPainter::HitTestTextItem(const HitTestContext& hit_test,
   if (!hit_test.location.Intersects(rect))
     return false;
 
-  return hit_test.AddNodeToResult(text_item.NodeForHitTest(), rect,
-                                  hit_test.inline_root_offset);
+  return hit_test.AddNodeToResult(text_item.NodeForHitTest(), &box_fragment_,
+                                  rect, hit_test.inline_root_offset);
 }
 
 // Replicates logic in legacy InlineFlowBox::NodeAtPoint().
@@ -2097,7 +2099,7 @@ bool NGBoxFragmentPainter::HitTestLineBoxFragment(
   }
 
   return hit_test.AddNodeToResult(
-      fragment.NodeForHitTest(), bounds_rect,
+      fragment.NodeForHitTest(), &box_fragment_, bounds_rect,
       physical_offset - cursor.Current().OffsetInContainerBlock());
 }
 
@@ -2192,8 +2194,8 @@ bool NGBoxFragmentPainter::HitTestChildBoxItem(
     // snap, but matches to legacy and fixes crbug.com/976606.
     bounds_rect = PhysicalRect(PixelSnappedIntRect(bounds_rect));
     if (hit_test.location.Intersects(bounds_rect)) {
-      if (hit_test.AddNodeToResult(item.NodeForHitTest(), bounds_rect,
-                                   child_offset))
+      if (hit_test.AddNodeToResult(item.NodeForHitTest(), &box_fragment_,
+                                   bounds_rect, child_offset))
         return true;
     }
   }

third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.h

@@ -207,6 +207,7 @@ class NGBoxFragmentPainter : public BoxPainterBase {
     // Add |node| to |HitTestResult|. Returns true if the hit-testing should
     // stop.
     bool AddNodeToResult(Node* node,
+                         const NGPhysicalBoxFragment* box_fragment,
                          const PhysicalRect& bounds_rect,
                          const PhysicalOffset& offset) const;
 

third_party/blink/web_tests/TestExpectations

@@ -1100,8 +1100,6 @@ crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/float-margin-at-row-
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/forced-break-in-nested-columns.html [ Failure ]
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/forced-break-too-short-column.html [ Failure ]
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/hit-test-above-or-below.html [ Failure ]
-crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/hit-test-end-of-column.html [ Failure ]
-crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/hit-test-end-of-column-with-line-height.html [ Crash Failure ]
 crbug.com/1066629 virtual/layout_ng_block_frag/fast/multicol/hit-test-translate-z.html [ Failure ]
 crbug.com/829181 virtual/layout_ng_block_frag/fast/multicol/infinitely-tall-content-in-outer-crash.html [ Skip ]
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/inline-getclientrects.html [ Failure ]
@@ -1148,7 +1146,6 @@ crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/tall-float1.html [ F
 crbug.com/1079031 virtual/layout_ng_block_frag/fast/multicol/tall-line-in-short-block.html [ Failure ]
 crbug.com/1079031 virtual/layout_ng_block_frag/fast/multicol/transform-with-fixedpos.html [ Failure ]
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/caret-range-anonymous-block.html [ Failure ]
-crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/caret-range-anonymous-block-rtl.html [ Failure Crash ]
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/caret-range-outside-columns.html [ Timeout Failure ]
 crbug.com/829028 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/caret-range-outside-columns-rtl.html [ Failure ]
 crbug.com/1058792 virtual/layout_ng_block_frag/fast/multicol/vertical-lr/composited-relpos-overlapping-will-change.html [ Failure ]