Commit b045e03b authored by David Bokan's avatar David Bokan Committed by Commit Bot

Fix text fragment's user scroll condition

Text fragments listen for scrolling so that they can stop trying to keep
a fragment in view during load and to record metrics about users'
scrolling behavior related to text fragments.

However, the condition used to filter non-user scrolls is
IsExplicitScrollType which includes programmtic scrolls. This is carried
over from regular fragments but means our metrics above will include
programmatic scrolls.

This CL fixes the condition and updates tests to check all scroll types.

Some tests are moved around, for ease of review PS1 makes changes to the
test, PS2 is just the code move.

Bug: 1097426
Change-Id: Ib86690d1af0303299edde8007786df2d81af6518
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2255779
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: default avatarNick Burris <nburris@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781074}
parent 0ee2213e
......@@ -226,8 +226,10 @@ bool TextFragmentAnchor::Invoke() {
void TextFragmentAnchor::Installed() {}
void TextFragmentAnchor::DidScroll(mojom::blink::ScrollType type) {
if (!IsExplicitScrollType(type))
if (type != mojom::blink::ScrollType::kUser &&
type != mojom::blink::ScrollType::kCompositor) {
return;
}
user_scrolled_ = true;
......
......@@ -873,8 +873,28 @@ TEST_F(TextFragmentAnchorTest, OneContextTerm) {
EXPECT_EQ(10u, markers.at(0)->EndOffset());
}
class TextFragmentAnchorScrollTest
: public TextFragmentAnchorTest,
public testing::WithParamInterface<mojom::blink::ScrollType> {
protected:
bool IsUserScrollType() {
return GetParam() == mojom::blink::ScrollType::kCompositor ||
GetParam() == mojom::blink::ScrollType::kUser;
}
};
INSTANTIATE_TEST_SUITE_P(
ScrollTypes,
TextFragmentAnchorScrollTest,
testing::Values(mojom::blink::ScrollType::kUser,
mojom::blink::ScrollType::kProgrammatic,
mojom::blink::ScrollType::kClamping,
mojom::blink::ScrollType::kCompositor,
mojom::blink::ScrollType::kAnchoring,
mojom::blink::ScrollType::kSequenced));
// Test that a user scroll cancels the scroll into view.
TEST_F(TextFragmentAnchorTest, ScrollCancelled) {
TEST_P(TextFragmentAnchorScrollTest, ScrollCancelled) {
SimRequest request("https://example.com/test.html#:~:text=test", "text/html");
SimSubresourceRequest css_request("https://example.com/test.css", "text/css");
SimSubresourceRequest img_request("https://example.com/test.png",
......@@ -898,9 +918,11 @@ TEST_F(TextFragmentAnchorTest, ScrollCancelled) {
)HTML");
Compositor().PaintFrame();
mojom::blink::ScrollType scroll_type = GetParam();
if (!RuntimeEnabledFeatures::BlockHTMLParserOnStyleSheetsEnabled()) {
GetDocument().View()->LayoutViewport()->ScrollBy(
ScrollOffset(0, 100), mojom::blink::ScrollType::kUser);
GetDocument().View()->LayoutViewport()->ScrollBy(ScrollOffset(0, 100),
scroll_type);
// Set the target text to visible and change its position to cause a layout
// and invoke the fragment anchor in the next begin frame.
css_request.Complete("p { visibility: visible; top: 1001px; }");
......@@ -920,8 +942,8 @@ TEST_F(TextFragmentAnchorTest, ScrollCancelled) {
// Before invoking again, perform a user scroll. This should abort future
// scrolls during fragment invocation.
GetDocument().View()->LayoutViewport()->SetScrollOffset(
ScrollOffset(0, 0), mojom::blink::ScrollType::kUser);
GetDocument().View()->LayoutViewport()->SetScrollOffset(ScrollOffset(0, 0),
scroll_type);
ASSERT_FALSE(ViewportRect().Contains(BoundingRectInFrame(p)));
img_request.Complete("");
......@@ -936,7 +958,14 @@ TEST_F(TextFragmentAnchorTest, ScrollCancelled) {
Compositor().BeginFrame();
Element& p = *GetDocument().getElementById("text");
EXPECT_FALSE(ViewportRect().Contains(BoundingRectInFrame(p)));
// If the scroll was a user scroll then we shouldn't try to keep the fragment
// in view. Otherwise, we should.
if (IsUserScrollType()) {
EXPECT_FALSE(ViewportRect().Contains(BoundingRectInFrame(p)));
} else {
EXPECT_TRUE(ViewportRect().Contains(BoundingRectInFrame(p)));
}
EXPECT_EQ(p, *GetDocument().CssTarget());
EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
......
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