Skip to content

Commit

Permalink
Revert "[SH] Allow highlighting text fragments on history navigations"
Browse files Browse the repository at this point in the history 
This reverts commit a49aad2.

Reason for revert: Breaks anchor links when opening a SH link
crbug.com/1233139

Original change's description:
> [SH] Allow highlighting text fragments on history navigations
>
> This behavior was disabled at the same time as disabling scrolling to
> the fragments for history use-cases. However, highlighting the fragment
> doesn't have the security implications that scrolling has. We can thus
> re-enable highlighting (but not scrolling) for these cases.
>
> Bug: 1202079
> Change-Id: Ic4f873832142260a746639ddf4b0db4a67414849
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2854279
> Reviewed-by: David Bokan <bokan@chromium.org>
> Commit-Queue: sebsg <sebsg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#877150}

(cherry picked from commit ceb59e3)

Bug: 1202079, 1233139
Change-Id: Ieb0c94df31c9a98b15772ca15654a7ad95c36833
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3059581
Commit-Queue: Guillaume Jenkins <gujen@google.com>
Auto-Submit: Guillaume Jenkins <gujen@google.com>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#906715}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3061060
Owners-Override: Srinivas Sista <srinivassista@chromium.org>
Auto-Submit: Srinivas Sista <srinivassista@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4590@{#3}
Cr-Branched-From: 1466af5-refs/heads/master@{#906442}
  • Loading branch information
@guillaumejenkins
guillaumejenkins authored and Chromium LUCI CQ committed Jul 29, 2021
1 parent 31b335d commit 7233cea
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 93 deletions.
29 changes: 14 additions & 15 deletions third_party/blink/renderer/core/page/scrolling/text_fragment_anchor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ bool ParseTextDirective(const String& fragment_directive,
return out_selectors->size() > 0;
}

// Determines whether the text fragment should be scrolled-to in the context of
// the current |frame|.
bool CheckSecurityRestrictionsForScrolling(LocalFrame& frame) {
bool CheckSecurityRestrictions(LocalFrame& frame) {
// This algorithm checks the security restrictions detailed in
// https://wicg.github.io/ScrollToTextFragment/#should-allow-a-text-fragment
// TODO(bokan): These are really only relevant for observable actions like
// scrolling. We should consider allowing highlighting regardless of these
// conditions. See the TODO in the relevant spec section:
// https://wicg.github.io/ScrollToTextFragment/#restricting-the-text-fragment

if (!frame.Loader().GetDocumentLoader()->ConsumeTextFragmentToken())
return false;

Expand Down Expand Up @@ -165,12 +168,8 @@ TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
return nullptr;
}

// The security checks only impact observable events like scrolling to the
// text fragment. Highlighting the text fragment is non-observable and thus
// can safely be done in those cases as well. More details available at
// https://wicg.github.io/ScrollToTextFragment/#restricting-the-text-fragment
if (!CheckSecurityRestrictionsForScrolling(frame)) {
should_scroll = false;
if (!CheckSecurityRestrictions(frame)) {
return nullptr;
} else if (!should_scroll) {
if (frame.Loader().GetDocumentLoader() &&
!frame.Loader().GetDocumentLoader()->NavigationScrollAllowed()) {
Expand Down Expand Up @@ -419,12 +418,6 @@ void TextFragmentAnchor::DidFindMatch(
if (AXObjectCache* cache = frame_->GetDocument()->ExistingAXObjectCache())
cache->HandleScrolledToAnchor(&node);

// Set the sequential focus navigation to the start of selection.
// Even if this element isn't focusable, "Tab" press will
// start the search to find the next focusable element from this element.
frame_->GetDocument()->SetSequentialFocusNavigationStartingPoint(
range.StartPosition().NodeAsRangeFirstNode());

metrics_->DidScroll();

// We scrolled the text into view if the main document scrolled or the text
Expand All @@ -444,6 +437,12 @@ void TextFragmentAnchor::DidFindMatch(
EphemeralRange(ToPositionInDOMTree(range.StartPosition()),
ToPositionInDOMTree(range.EndPosition()));
frame_->GetDocument()->Markers().AddTextFragmentMarker(dom_range);

// Set the sequential focus navigation to the start of selection.
// Even if this element isn't focusable, "Tab" press will
// start the search to find the next focusable element from this element.
frame_->GetDocument()->SetSequentialFocusNavigationStartingPoint(
range.StartPosition().NodeAsRangeFirstNode());
}

void TextFragmentAnchor::DidFinishSearch() {
Expand Down
89 changes: 14 additions & 75 deletions third_party/blink/renderer/core/page/scrolling/text_fragment_anchor_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "components/shared_highlighting/core/common/shared_highlighting_features.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/input/web_menu_source_type.h"
#include "third_party/blink/public/mojom/scroll/scroll_enums.mojom-blink.h"
#include "third_party/blink/public/platform/scheduler/test/renderer_scheduler_test_support.h"
#include "third_party/blink/public/platform/scheduler/web_thread_scheduler.h"
#include "third_party/blink/public/public_buildflags.h"
Expand All @@ -33,7 +32,6 @@
#include "third_party/blink/renderer/core/loader/document_loader.h"
#include "third_party/blink/renderer/core/loader/empty_clients.h"
#include "third_party/blink/renderer/core/page/context_menu_controller.h"
#include "third_party/blink/renderer/core/page/scrolling/fragment_anchor.h"
#include "third_party/blink/renderer/core/page/scrolling/text_fragment_finder.h"
#include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
#include "third_party/blink/renderer/core/scroll/scrollable_area.h"
Expand Down Expand Up @@ -2016,64 +2014,12 @@ TEST_F(TextFragmentAnchorTest, TextDirectiveInSvg) {
EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
}

// Ensure we restore the highlight on page reload, but that we do not scroll to
// the text fragment.
TEST_F(TextFragmentAnchorTest, Reload) {
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: 2200px;
}
#first {
position: absolute;
top: 1000px;
}
#second {
position: absolute;
top: 2000px;
}
</style>
<p id="first">This is a test page</p>
<p id="second">This is some more text</p>
)HTML";
request.Complete(html);
RunAsyncMatchingTasks();

// Render two frames to handle the async step added by the beforematch event.
Compositor().BeginFrame();
Compositor().BeginFrame();

EXPECT_EQ(1u, GetDocument().Markers().Markers().size());

// Scroll to the top before reloading.
GetDocument().View()->GetScrollableArea()->SetScrollOffset(
ScrollOffset(), mojom::blink::ScrollType::kProgrammatic);
Compositor().BeginFrame();

// Reload the page.
SimRequest reload_request("https://example.com/test.html#:~:text=test",
"text/html");
MainFrame().StartReload(WebFrameLoadType::kReload);
reload_request.Complete(html);

// Render two frames to handle the async step added by the beforematch event.
Compositor().BeginFrame();
Compositor().BeginFrame();

// Make sure the text fragment is highlighted.
EXPECT_EQ(*GetDocument().getElementById("first"), *GetDocument().CssTarget());
EXPECT_EQ(1u, GetDocument().Markers().Markers().size());

// Make sure the page is not scrolled to the highlight, but to the last place
// it was before the reload; in this case the top of the page.
EXPECT_EQ(ScrollOffset(), LayoutViewport()->GetScrollOffset());
}

// Ensure we don't restore a dismissed highlight on page reload.
TEST_F(TextFragmentAnchorTest, ReloadAfterDismissing) {
// Ensure we restore the text highlight on page reload
// TODO(bokan): This test is disabled as this functionality was suppressed in
// https://crrev.com/c/2135407; it would be better addressed by providing a
// highlight-only function. See the TODO in
// https://wicg.github.io/ScrollToTextFragment/#restricting-the-text-fragment
TEST_F(TextFragmentAnchorTest, DISABLED_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(
Expand All @@ -2098,29 +2044,22 @@ TEST_F(TextFragmentAnchorTest, ReloadAfterDismissing) {

EXPECT_EQ(1u, GetDocument().Markers().Markers().size());

// Dismiss the highlight.
EXPECT_TRUE(GetDocument().View()->GetFragmentAnchor()->Dismiss());

// Make sure the URL was updated.
KURL url = GetDocument()
.GetFrame()
->Loader()
.GetDocumentLoader()
->GetHistoryItem()
->Url();
EXPECT_EQ("https://example.com/test.html", url.GetString());
// Tap to dismiss the highlight.
SimulateClick(10, 10);
EXPECT_EQ(0u, GetDocument().Markers().Markers().size());

// Reload the page.
SimRequest reload_request(url.GetString(), "text/html");
// 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);

// Render two frames to handle the async step added by the beforematch event.
Compositor().BeginFrame();
Compositor().BeginFrame();

// Make sure the text fragment is not highlighted.
EXPECT_EQ(0u, GetDocument().Markers().Markers().size());
EXPECT_EQ(*GetDocument().getElementById("text"), *GetDocument().CssTarget());
EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
}

// Ensure that we can have text directives combined with non-text directives
Expand Down
8 changes: 5 additions & 3 deletions third_party/blink/renderer/core/page/scrolling/text_fragment_handler_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -774,12 +774,11 @@ TEST_P(TextFragmentHandlerTest,
feature_list_.InitAndEnableFeature(
shared_highlighting::kSharedHighlightingAmp);
SimRequest main_request("https://example.com/test.html", "text/html");
SimRequest child_request("https://example.com/child.html#:~:text=test",
"text/html");
SimRequest child_request("https://example.com/child.html", "text/html");
LoadURL("https://example.com/test.html");
main_request.Complete(R"HTML(
<!DOCTYPE html>
<iframe id="iframe" src="child.html#:~:text=test"></iframe>
<iframe id="iframe" src="child.html"></iframe>
)HTML");

child_request.Complete(R"HTML(
Expand All @@ -792,6 +791,9 @@ TEST_P(TextFragmentHandlerTest,
<p>
test
</p>
<script>
window.location.hash = ':~:text=test';
</script>
)HTML");
RunAsyncMatchingTasks();

Expand Down

0 comments on commit 7233cea

Please sign in to comment.