Revert "Highlight text fragment directives on page reload"

This reverts commit 8e1e2bb6.

Reason for revert: <INSERT REASONING HERE>
../../third_party/blink/renderer/core/page/scrolling/text_fragment_anchor_test.cc(8,10): fatal error: 'third_party/blink/renderer/bindings/core/v8/usv_string_or_trusted_url.h' file not found
#include "third_party/blink/renderer/bindings/core/v8/usv_string_or_trusted_url.h"
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.


Original change's description:
> Highlight text fragment directives on page reload
> 
> Fix the behavior where fragment anchors were only kept alive if a scroll
> was needed, as is the behavior of element fragment anchors. This caused
> text fragment anchors not to be kept alive and we didn't highlight on
> reload.
> 
> Note that *sometimes* we would highlight on reload, due to a race
> condition with fragment anchors, where we process the fragment before
> restoring scroll position (which is where should_scroll becomes false),
> see crbug/839292. This change makes it so we highlight on reload
> independent of whether a scroll is needed.
> 
> Bug: 932551
> Change-Id: I38cc82c4920255d8bcae9f8b55ac80f24d1465da
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864714
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#706847}

TBR=bokan@chromium.org,nburris@chromium.org

Change-Id: Ifa4ea6a04c62662a1ec0a7b611247ed16d9d979b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 932551
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866623Reviewed-by: Lan Wei <lanwei@chromium.org>
Commit-Queue: Lan Wei <lanwei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706980}

third_party/blink/renderer/core/frame/local_frame_view.cc

@@ -1362,10 +1362,10 @@ void LocalFrameView::ProcessUrlFragment(const KURL& url,
                                         bool should_scroll) {
   // We want to create the anchor even if we don't need to scroll. This ensures
   // all the side effects like setting CSS :target are correctly set.
-  FragmentAnchor* anchor = FragmentAnchor::TryCreate(
-      url, *frame_, same_document_navigation, should_scroll);
+  FragmentAnchor* anchor =
+      FragmentAnchor::TryCreate(url, *frame_, same_document_navigation);
 
-  if (anchor) {
+  if (anchor && should_scroll) {
     fragment_anchor_ = anchor;
     fragment_anchor_->Installed();
 

third_party/blink/renderer/core/page/scrolling/element_fragment_anchor.cc

@@ -34,8 +34,7 @@ Node* FindAnchorFromFragment(const String& fragment, Document& doc) {
 }  // namespace
 
 ElementFragmentAnchor* ElementFragmentAnchor::TryCreate(const KURL& url,
-                                                        LocalFrame& frame,
-                                                        bool should_scroll) {
+                                                        LocalFrame& frame) {
   DCHECK(frame.GetDocument());
   Document& doc = *frame.GetDocument();
 
@@ -84,10 +83,6 @@ ElementFragmentAnchor* ElementFragmentAnchor::TryCreate(const KURL& url,
   if (!anchor_node)
     return nullptr;
 
-  // Element fragment anchors only need to be kept alive if they need scrolling.
-  if (!should_scroll)
-    return nullptr;
-
   return MakeGarbageCollected<ElementFragmentAnchor>(*anchor_node, frame);
 }
 

third_party/blink/renderer/core/page/scrolling/element_fragment_anchor.h

@@ -29,9 +29,7 @@ class CORE_EXPORT ElementFragmentAnchor final : public FragmentAnchor {
   // Parses the URL fragment and, if possible, creates and returns a fragment
   // based on an Element in the page. Returns nullptr otherwise. Produces side
   // effects related to fragment targeting in the page in either case.
-  static ElementFragmentAnchor* TryCreate(const KURL& url,
-                                          LocalFrame& frame,
-                                          bool should_scroll);
+  static ElementFragmentAnchor* TryCreate(const KURL& url, LocalFrame& frame);
 
   ElementFragmentAnchor(Node& anchor_node, LocalFrame& frame);
   ~ElementFragmentAnchor() override = default;

third_party/blink/renderer/core/page/scrolling/fragment_anchor.cc

@@ -15,8 +15,7 @@ namespace blink {
 
 FragmentAnchor* FragmentAnchor::TryCreate(const KURL& url,
                                           LocalFrame& frame,
-                                          bool same_document_navigation,
-                                          bool should_scroll) {
+                                          bool same_document_navigation) {
   DCHECK(frame.GetDocument());
 
   FragmentAnchor* anchor = nullptr;
@@ -29,7 +28,7 @@ FragmentAnchor* FragmentAnchor::TryCreate(const KURL& url,
   bool text_fragment_anchor_created = false;
   if (text_fragment_identifiers_enabled) {
     anchor = TextFragmentAnchor::TryCreateFragmentDirective(
-        url, frame, same_document_navigation, should_scroll);
+        url, frame, same_document_navigation);
     text_fragment_anchor_created = anchor;
   }
 
@@ -61,7 +60,7 @@ FragmentAnchor* FragmentAnchor::TryCreate(const KURL& url,
 
   bool element_id_anchor_found = false;
   if (!anchor) {
-    anchor = ElementFragmentAnchor::TryCreate(url, frame, should_scroll);
+    anchor = ElementFragmentAnchor::TryCreate(url, frame);
     element_id_anchor_found = anchor;
   }
 

third_party/blink/renderer/core/page/scrolling/fragment_anchor.h

@@ -32,8 +32,7 @@ class CORE_EXPORT FragmentAnchor : public GarbageCollected<FragmentAnchor> {
   // will be performed, for example, setting/clearing :target and svgView().
   static FragmentAnchor* TryCreate(const KURL& url,
                                    LocalFrame& frame,
-                                   bool same_document_navigation,
-                                   bool should_scroll);
+                                   bool same_document_navigation);
 
   FragmentAnchor() = default;
   virtual ~FragmentAnchor() = default;

third_party/blink/renderer/core/page/scrolling/text_fragment_anchor.cc

@@ -77,8 +77,7 @@ bool CheckSecurityRestrictions(LocalFrame& frame,
 TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
     const KURL& url,
     LocalFrame& frame,
-    bool same_document_navigation,
-    bool should_scroll) {
+    bool same_document_navigation) {
   DCHECK(RuntimeEnabledFeatures::TextFragmentIdentifiersEnabled(
       frame.GetDocument()));
 
@@ -97,16 +96,13 @@ TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
     return nullptr;
   }
 
-  return MakeGarbageCollected<TextFragmentAnchor>(selectors, frame,
-                                                  should_scroll);
+  return MakeGarbageCollected<TextFragmentAnchor>(selectors, frame);
 }
 
 TextFragmentAnchor::TextFragmentAnchor(
     const Vector<TextFragmentSelector>& text_fragment_selectors,
-    LocalFrame& frame,
-    bool should_scroll)
+    LocalFrame& frame)
     : frame_(&frame),
-      should_scroll_(should_scroll),
       metrics_(MakeGarbageCollected<TextFragmentAnchorMetrics>(
           frame_->GetDocument())) {
   DCHECK(!text_fragment_selectors.IsEmpty());
@@ -138,7 +134,7 @@ bool TextFragmentAnchor::Invoke() {
   if (user_scrolled_ && !did_scroll_into_view_)
     metrics_->ScrollCancelled();
 
-  first_match_needs_scroll_ = should_scroll_ && !user_scrolled_;
+  first_match_needs_scroll_ = !user_scrolled_;
 
   {
     // FindMatch might cause scrolling and set user_scrolled_ so reset it when
@@ -267,8 +263,8 @@ void TextFragmentAnchor::DidFinishSearch() {
     dismissed_ = true;
 
     DCHECK(!element_fragment_anchor_);
-    element_fragment_anchor_ = ElementFragmentAnchor::TryCreate(
-        frame_->GetDocument()->Url(), *frame_, should_scroll_);
+    element_fragment_anchor_ =
+        ElementFragmentAnchor::TryCreate(frame_->GetDocument()->Url(), *frame_);
     if (element_fragment_anchor_) {
       // Schedule a frame so we can invoke the element anchor in
       // PerformPreRafActions.
@@ -288,7 +284,7 @@ bool TextFragmentAnchor::Dismiss() {
   if (!did_find_match_ || dismissed_)
     return true;
 
-  DCHECK(!should_scroll_ || did_scroll_into_view_ || user_scrolled_);
+  DCHECK(did_scroll_into_view_ || user_scrolled_);
 
   frame_->GetDocument()->Markers().RemoveMarkersOfTypes(
       DocumentMarker::MarkerTypes::TextFragment());

third_party/blink/renderer/core/page/scrolling/text_fragment_anchor.h

@@ -36,13 +36,11 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
   static TextFragmentAnchor* TryCreateFragmentDirective(
       const KURL& url,
       LocalFrame& frame,
-      bool same_document_navigation,
-      bool should_scroll);
+      bool same_document_navigation);
 
   TextFragmentAnchor(
       const Vector<TextFragmentSelector>& text_fragment_selectors,
-      LocalFrame& frame,
-      bool should_scroll);
+      LocalFrame& frame);
   ~TextFragmentAnchor() override = default;
 
   bool Invoke() override;
@@ -91,10 +89,6 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
   // Whether the text fragment anchor has been dismissed yet. This should be
   // kept alive until dismissed so we can remove text highlighting.
   bool dismissed_ = false;
-  // Whether we should scroll the anchor into view. This will be false for
-  // history navigations and reloads, where we want to restore the highlight but
-  // not scroll into view again.
-  bool should_scroll_ = false;
 
   Member<TextFragmentAnchorMetrics> metrics_;
 

third_party/blink/renderer/core/page/scrolling/text_fragment_anchor_test.cc

@@ -11,7 +11,6 @@
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/core/frame/location.h"
-#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
 #include "third_party/blink/renderer/core/html/html_element.h"
 #include "third_party/blink/renderer/core/html/html_frame_owner_element.h"
 #include "third_party/blink/renderer/core/input/event_handler.h"
@@ -1545,46 +1544,6 @@ TEST_F(TextFragmentAnchorTest, TextDirectiveInSvg) {
   EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
 }
 
-// Ensure we restore the text highlight on page reload
-TEST_F(TextFragmentAnchorTest, HighlightOnReload) {
-  SimRequest request("https://example.com/test.html#:~:text=test", "text/html");
-  LoadURL("https://example.com/test.html#:~:text=test");
-  const String& html = R"HTML(
-    <!DOCTYPE html>
-    <style>
-      body {
-        height: 1200px;
-      }
-      p {
-        position: absolute;
-        top: 1000px;
-      }
-    </style>
-    <p id="text">This is a test page</p>
-  )HTML";
-  request.Complete(html);
-
-  Compositor().BeginFrame();
-  RunAsyncMatchingTasks();
-
-  EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
-
-  // Tap to dismiss the highlight.
-  SimulateClick(10, 10);
-  EXPECT_EQ(0u, GetDocument().Markers().Markers().size());
-
-  // Reload the page and expect the highlight to be restored.
-  SimRequest reload_request("https://example.com/test.html#:~:text=test",
-                            "text/html");
-  MainFrame().StartReload(WebFrameLoadType::kReload);
-  reload_request.Complete(html);
-
-  Compositor().BeginFrame();
-  RunAsyncMatchingTasks();
-
-  EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
-}
-
 }  // namespace
 
 }  // namespace blink