Skip to content

GitLab

T
tangled
Commit 9ca40f89 authored by David Bokan's avatar David Bokan Committed by Commit Bot
Browse files
Options

Avoid fragment activation while rendering blocked 

This bug was introduced by:
  https://crrev.com/dbd4eed1920a2aafbc87236db7efb98f108ba063

The previous behavior was to only ProcessUrlFragment if rendering was
ready. If it wasn't, we would mark it as needing processing and try
again when it becomes ready. My patch above separated ProcessUrlFragment
from ScrollAndFocusFragmentAnchor but omitted the deferral of the latter
in cases where rendering wasn't yet ready.

This patch simply early-outs in ScrollAndFocusFragmentAnchor when
rendering isn't yet ready to match the old behavior. It also adds back a
hook in ImplicitClose since a layout may not be needed at the time the
document was closed because it was done at a time when rendering was
still blocked.

Bug: 851338
Change-Id: I70d4ab233580f7c005fd56e805d5acd79cd4eee6
Reviewed-on: https://chromium-review.googlesource.com/1130958Reviewed-by: default avatarKent Tamura <tkent@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583146}
parent 1093cd5a
Hide whitespace changes
Inline Side-by-side
Showing with 69 additions and 1 deletion
third_party/WebKit/LayoutTests/http/tests/html/click-anchor-link-crash.html
View file @ 9ca40f89
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
var anchorLink = document.getElementById("anchorlink"); var anchorLink = document.getElementById("anchorlink");
anchorLink.click(); anchorLink.click();
anchorLink.focus(); anchorLink.focus();
assert_equals(document.activeElement, document.body); assert_equals(document.activeElement, anchorLink);
}, "should not crash on focus call"); }, "should not crash on focus call");
}); });
</script> </script>
......
third_party/blink/renderer/core/dom/document.cc
View file @ 9ca40f89
...@@ -3371,6 +3371,9 @@ void Document::ImplicitClose() { ...@@ -3371,6 +3371,9 @@ void Document::ImplicitClose() {
View()->UpdateLayout(); View()->UpdateLayout();
} }
if (View())
View()->ScrollAndFocusFragmentAnchor();
load_event_progress_ = kLoadEventCompleted; load_event_progress_ = kLoadEventCompleted;
if (GetFrame() && GetLayoutView()) { if (GetFrame() && GetLayoutView()) {
......
third_party/blink/renderer/core/frame/local_frame_view.cc
View file @ 9ca40f89
...@@ -1887,6 +1887,9 @@ void LocalFrameView::ScrollAndFocusFragmentAnchor() { ...@@ -1887,6 +1887,9 @@ void LocalFrameView::ScrollAndFocusFragmentAnchor() {
if (!anchor_node) if (!anchor_node)
return; return;
if (!frame_->GetDocument()->IsRenderingReady())
return;
if (anchor_node->GetLayoutObject()) { if (anchor_node->GetLayoutObject()) {
LayoutRect rect; LayoutRect rect;
if (anchor_node != frame_->GetDocument()) { if (anchor_node != frame_->GetDocument()) {
......
third_party/blink/renderer/core/frame/local_frame_view_test.cc
View file @ 9ca40f89
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
#include <memory> #include <memory>
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
#include "third_party/blink/renderer/core/html/html_anchor_element.h"
#include "third_party/blink/renderer/core/html/html_element.h" #include "third_party/blink/renderer/core/html/html_element.h"
#include "third_party/blink/renderer/core/layout/layout_view.h" #include "third_party/blink/renderer/core/layout/layout_view.h"
#include "third_party/blink/renderer/core/paint/paint_layer.h" #include "third_party/blink/renderer/core/paint/paint_layer.h"
...@@ -18,7 +19,9 @@ ...@@ -18,7 +19,9 @@
#include "third_party/blink/renderer/platform/geometry/int_size.h" #include "third_party/blink/renderer/platform/geometry/int_size.h"
#include "third_party/blink/renderer/platform/graphics/paint/paint_artifact.h" #include "third_party/blink/renderer/platform/graphics/paint/paint_artifact.h"
#include "third_party/blink/renderer/platform/runtime_enabled_features.h" #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
using blink::test::RunPendingTasks;
using testing::_; using testing::_;
using testing::AnyNumber; using testing::AnyNumber;
...@@ -265,5 +268,64 @@ TEST_F(LocalFrameViewSimTest, CSSFragmentIdentifierEmptySelector) { ...@@ -265,5 +268,64 @@ TEST_F(LocalFrameViewSimTest, CSSFragmentIdentifierEmptySelector) {
EXPECT_EQ(nullptr, GetDocument().CssTarget()); EXPECT_EQ(nullptr, GetDocument().CssTarget());
} }
// Ensure the fragment navigation "scroll into view and focus" behavior doesn't
// activate synchronously while rendering is blocked waiting on a stylesheet.
// See https://crbug.com/851338.
TEST_F(LocalFrameViewSimTest, FragmentNavChangesFocusWhileRenderingBlocked) {
SimRequest main_resource("https://example.com/test.html", "text/html");
SimRequest css_resource("https://example.com/sheet.css", "text/css");
LoadURL("https://example.com/test.html");
main_resource.Complete(R"HTML(
<!DOCTYPE html>
<link rel="stylesheet" type="text/css" href="sheet.css">
<a id="anchorlink" href="#bottom">Link to bottom of the page</a>
<div style="height: 1000px;"></div>
<input id="bottom">Bottom of the page</a>
)HTML");
ScrollableArea* viewport = GetDocument().View()->LayoutViewport();
ASSERT_EQ(ScrollOffset(), viewport->GetScrollOffset());
// We're still waiting on the stylesheet to load so the load event shouldn't
// yet dispatch and rendering is deferred.
ASSERT_FALSE(GetDocument().IsRenderingReady());
EXPECT_FALSE(GetDocument().IsLoadCompleted());
// Click on the anchor element. This will cause a synchronous same-document
// navigation.
HTMLAnchorElement* anchor =
ToHTMLAnchorElement(GetDocument().getElementById("anchorlink"));
anchor->click();
// Even though the navigation is synchronous, the active element shouldn't be
// changed.
EXPECT_EQ(GetDocument().body(), GetDocument().ActiveElement())
<< "Active element changed while rendering is blocked";
EXPECT_EQ(ScrollOffset(), viewport->GetScrollOffset())
<< "Scroll offset changed while rendering is blocked";
// Force a layout.
anchor->style()->setProperty(&GetDocument(), "display", "block", String(),
ASSERT_NO_EXCEPTION);
GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
EXPECT_EQ(GetDocument().body(), GetDocument().ActiveElement())
<< "Active element changed due to layout while rendering is blocked";
EXPECT_EQ(ScrollOffset(), viewport->GetScrollOffset())
<< "Scroll offset changed due to layout while rendering is blocked";
// Complete the CSS stylesheet load so the document can finish loading. The
// fragment should be activated at that point.
css_resource.Complete("");
RunPendingTasks();
ASSERT_TRUE(GetDocument().IsLoadCompleted());
EXPECT_EQ(GetDocument().getElementById("bottom"),
GetDocument().ActiveElement())
<< "Active element wasn't changed after load completed.";
EXPECT_NE(ScrollOffset(), viewport->GetScrollOffset())
<< "Scroll offset wasn't changed after load completed.";
}
} // namespace } // namespace
} // namespace blink } // namespace blink
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment