Fix scrolling to anchor on load

It looks like between when I reverted my patch in https://crrev.com/777040 and when I originally landed it there's been some changes that meant my revert broke hash navigation. I'm not sure how this worked before but it looks like currently the navigation relies on the fact that at the end of a frame's layout, we called into RootScrollerController::DidUpdateLayout which would recalculate the root scroller and then synchronously recompute style and layout using Document::UpdateStyleAndLayout(). When a page is finished parsing it tries to scroll to the hash in FrameLoader::FinishedParsing. However, if the page is blocked from rendering this will be deferred since we can't layout yet. Normally, when the load event is fired, we'd call Document::ImplicitClose which calls LocalFrameView::UpdateLayout. This causes the scroll to the hash via the above indirect call to Document::UpdateStyleAndLayout but my revert above removed this call. This CL fixes the issue by directly trying to scroll to the hash in ImplicitClose. Bug: 788486 Change-Id: I67f78372380f043bf88fda036bf7c263b17fbca0 Reviewed-on: https://chromium-review.googlesource.com/809093 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#524483}

Fix scrolling to anchor on load
It looks like between when I reverted my patch in https://crrev.com/777040 and when I originally landed it there's been some changes that meant my revert broke hash navigation. I'm not sure how this worked before but it looks like currently the navigation relies on the fact that at the end of a frame's layout, we called into RootScrollerController::DidUpdateLayout which would recalculate the root scroller and then synchronously recompute style and layout using Document::UpdateStyleAndLayout(). When a page is finished parsing it tries to scroll to the hash in FrameLoader::FinishedParsing. However, if the page is blocked from rendering this will be deferred since we can't layout yet. Normally, when the load event is fired, we'd call Document::ImplicitClose which calls LocalFrameView::UpdateLayout. This causes the scroll to the hash via the above indirect call to Document::UpdateStyleAndLayout but my revert above removed this call. This CL fixes the issue by directly trying to scroll to the hash in ImplicitClose. Bug: 788486 Change-Id: I67f78372380f043bf88fda036bf7c263b17fbca0 Reviewed-on: https://chromium-review.googlesource.com/809093 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#524483}
08015810 · David Bokan · Commit Bot · 7eebce26 · 08015810 · 08015810
Commit 08015810 authored Dec 15, 2017 by David Bokan Committed by Commit Bot Dec 15, 2017
2 changed files
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -3269,6 +3269,12 @@ void Document::ImplicitClose() {
        (!GetLayoutViewItem().FirstChild() ||
         GetLayoutViewItem().NeedsLayout()))
      View()->UpdateLayout();
+
+    // TODO(bokan): This is a temporary fix to https://crbug.com/788486.
+    // There's some better cleanups that should be done to follow-up:
+    // https://crbug.com/795381.
+    if (View() && goto_anchor_needed_after_stylesheets_load_)
+      View()->ProcessUrlFragment(url_);
  }

  load_event_progress_ = kLoadEventCompleted;

--- a/third_party/WebKit/Source/core/loader/ProgrammaticScrollTest.cpp
+++ b/third_party/WebKit/Source/core/loader/ProgrammaticScrollTest.cpp
@@ -8,6 +8,9 @@
 #include "core/frame/WebLocalFrameImpl.h"
 #include "core/loader/DocumentLoader.h"
 #include "core/loader/FrameLoader.h"
+#include "core/testing/sim/SimDisplayItemList.h"
+#include "core/testing/sim/SimRequest.h"
+#include "core/testing/sim/SimTest.h"
 #include "platform/testing/URLTestHelpers.h"
 #include "platform/testing/UnitTestHelpers.h"
 #include "public/platform/Platform.h"
@@ -104,4 +107,52 @@ TEST_F(ProgrammaticScrollTest, RestoreScrollPositionAndViewStateWithoutScale) {
  EXPECT_EQ(400, web_view->MainFrameImpl()->GetScrollOffset().height);
 }

+class ProgrammaticScrollSimTest : public ::testing::WithParamInterface<bool>,
+                                  private ScopedRootLayerScrollingForTest,
+                                  public SimTest {
+ public:
+  ProgrammaticScrollSimTest() : ScopedRootLayerScrollingForTest(GetParam()) {}
+};
+
+INSTANTIATE_TEST_CASE_P(All, ProgrammaticScrollSimTest, ::testing::Bool());
+
+TEST_P(ProgrammaticScrollSimTest, NavigateToHash) {
+  WebView().Resize(WebSize(800, 600));
+  SimRequest main_resource("https://example.com/test.html#target", "text/html");
+  SimRequest css_resource("https://example.com/test.css", "text/css");
+
+  LoadURL("https://example.com/test.html#target");
+
+  // Finish loading the main document before the stylesheet is loaded so that
+  // rendering is blocked when parsing finishes. This will delay closing the
+  // document until the load event.
+  main_resource.Start();
+  main_resource.Write(
+      "<!DOCTYPE html><link id=link rel=stylesheet href=test.css>");
+  css_resource.Start();
+  main_resource.Write(R"HTML(
+    <style>
+      body {
+        height: 4000px;
+      }
+      h2 {
+        position: absolute;
+        top: 3000px;
+      }
+    </style>
+    <h2 id="target">Target</h2>
+  )HTML");
+  main_resource.Finish();
+  css_resource.Complete();
+  Compositor().BeginFrame();
+
+  // Run pending tasks to fire the load event and close the document. This
+  // should cause the document to scroll to the hash.
+  testing::RunPendingTasks();
+
+  ScrollableArea* layout_viewport =
+      GetDocument().View()->LayoutViewportScrollableArea();
+  EXPECT_EQ(3001, layout_viewport->GetScrollOffset().Height());
+}
+
 }  // namespace blink