Skip to content

GitLab

T
tangled
Commit 42987aa5 authored by Nick Burris's avatar Nick Burris Committed by Commit Bot
Browse files
Options

[scroll-to-text-fragment] Fix TextFragmentAnchor CSS :target DCHECK 

In crrev.com/c/1965273 we started setting the CSS :target on the
document to the found match. However, this dirties style and may require
a lifecycle update, causing the computation of the highlight and scroll
rects to DCHECK. This patch adds a call to UpdateStyleAndLayout after
setting the target.

Bug: 1045117
Change-Id: Ic6648665ef7e7d1e9fba47a6bac0741803257269
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2017562Reviewed-by: default avatarDavid Bokan <bokan@chromium.org>
Commit-Queue: Nick Burris <nburris@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736621}
parent 16dc9c4a
Hide whitespace changes
Inline Side-by-side
Showing with 76 additions and 13 deletions
third_party/blink/renderer/core/page/scrolling/text_fragment_anchor.cc
View file @ 42987aa5
...@@ -222,9 +222,28 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) { ...@@ -222,9 +222,28 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) {
return; return;
} }
bool needs_style_and_layout = false;
// Apply :target to the first match // Apply :target to the first match
if (!did_find_match_) if (!did_find_match_) {
ApplyTargetToCommonAncestor(range); ApplyTargetToCommonAncestor(range);
needs_style_and_layout = true;
}
// Activate any find-in-page activatable display-locks in the ancestor
// chain.
if (DisplayLockUtilities::ActivateFindInPageMatchRangeIfNeeded(range)) {
// Since activating a lock dirties layout, we need to make sure it's clean
// before computing the text rect below.
needs_style_and_layout = true;
// TODO(crbug.com/1041942): It is possible and likely that activation
// signal causes script to resize something on the page. This code here
// should really yield until the next frame to give script an opportunity
// to run.
}
if (needs_style_and_layout)
frame_->GetDocument()->UpdateStyleAndLayout();
metrics_->DidFindMatch(PlainText(range)); metrics_->DidFindMatch(PlainText(range));
did_find_match_ = true; did_find_match_ = true;
...@@ -232,18 +251,6 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) { ...@@ -232,18 +251,6 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) {
if (first_match_needs_scroll_) { if (first_match_needs_scroll_) {
first_match_needs_scroll_ = false; first_match_needs_scroll_ = false;
// Activate any find-in-page activatable display-locks in the ancestor
// chain.
if (DisplayLockUtilities::ActivateFindInPageMatchRangeIfNeeded(range)) {
// Since activating a lock dirties layout, we need to make sure it's clean
// before computing the text rect below.
frame_->GetDocument()->UpdateStyleAndLayout();
// TODO(crbug.com/1041942): It is possible and likely that activation
// signal causes script to resize something on the page. This code here
// should really yield until the next frame to give script an opportunity
// to run.
}
PhysicalRect bounding_box(ComputeTextRect(range)); PhysicalRect bounding_box(ComputeTextRect(range));
// Set the bounding box height to zero because we want to center the top of // Set the bounding box height to zero because we want to center the top of
......
third_party/blink/renderer/core/page/scrolling/text_fragment_anchor_test.cc
View file @ 42987aa5
...@@ -1688,6 +1688,62 @@ TEST_F(TextFragmentAnchorTest, NonTextDirectives) { ...@@ -1688,6 +1688,62 @@ TEST_F(TextFragmentAnchorTest, NonTextDirectives) {
EXPECT_EQ(2u, GetDocument().Markers().Markers().size()); EXPECT_EQ(2u, GetDocument().Markers().Markers().size());
} }
// Test that the text directive applies :target styling
TEST_F(TextFragmentAnchorTest, CssTarget) {
SimRequest main_request("https://example.com/test.html#:~:text=test",
"text/html");
SimRequest css_request("https://example.com/test.css", "text/css");
LoadURL("https://example.com/test.html#:~:text=test");
main_request.Complete(R"HTML(
<!DOCTYPE html>
<style>
p {
margin-top: 1000px;
}
</style>
<link rel="stylesheet" href="test.css">
<p id="text">test</p>
)HTML");
// With BlockHTMLParserOnStyleSheetsEnabled, the text fragment anchor won't be
// invoked until the CSS is loaded. Otherwise, we test the behavior where the
// text fragment anchor is invoked before and after the stylesheet is applied.
if (!RuntimeEnabledFeatures::BlockHTMLParserOnStyleSheetsEnabled()) {
Compositor().PaintFrame();
ScrollOffset first_scroll_offset = LayoutViewport()->GetScrollOffset();
ASSERT_NE(ScrollOffset(), first_scroll_offset);
Element& p = *GetDocument().getElementById("text");
IntRect first_bounding_rect = BoundingRectInFrame(p);
EXPECT_TRUE(ViewportRect().Contains(first_bounding_rect));
// Load CSS that has target styling that moves the text out of view
css_request.Complete(R"CSS(
:target {
margin-top: 2000px;
}
)CSS");
RunPendingTasks();
Compositor().BeginFrame();
// Ensure the target text is still in view and stayed centered
ASSERT_NE(first_scroll_offset, LayoutViewport()->GetScrollOffset());
EXPECT_EQ(first_bounding_rect, BoundingRectInFrame(p));
} else {
css_request.Complete(R"CSS(
:target {
margin-top: 2000px;
}
)CSS");
RunPendingTasks();
Compositor().BeginFrame();
}
Element& p = *GetDocument().getElementById("text");
EXPECT_TRUE(ViewportRect().Contains(BoundingRectInFrame(p)));
EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
}
} // 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