Commit d49e7b56 authored by Joey Arhar's avatar Joey Arhar Committed by Commit Bot

Add beforematch hidden-matchable revealing UKM

UKM collection review:
https://docs.google.com/document/d/1-J9xNy7KQ9AYpFRaGBZu3ZlzPgh8NHxDt1VNAfjvIN0/edit#

Bug: 1114171
Change-Id: Ie2391610603ea91ac7d7b9d229efb3b6af0b8610
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354658
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Reviewed-by: default avatarBrian White <bcwhite@chromium.org>
Reviewed-by: default avatarvmpstr <vmpstr@chromium.org>
Reviewed-by: default avatarChris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799770}
parent 57d4ff14
......@@ -2745,6 +2745,23 @@ void Document::MarkHasFindInPageContentVisibilityActiveMatch() {
had_find_in_page_render_subtree_active_match_ = true;
}
void Document::MarkHasFindInPageBeforematchExpandedHiddenMatchable() {
// Note that although find-in-page in content-visibility requests happen in
// non-main frames, we only record the main frame results (per UKM policy).
// Additionally, we only record the event once.
if (had_find_in_page_beforematch_expanded_hidden_matchable_ ||
!IsInMainFrame())
return;
auto* recorder = UkmRecorder();
DCHECK(recorder);
DCHECK(UkmSourceID() != ukm::kInvalidSourceId);
ukm::builders::Blink_FindInPage(UkmSourceID())
.SetBeforematchExpandedHiddenMatchable(true)
.Record(recorder);
had_find_in_page_beforematch_expanded_hidden_matchable_ = true;
}
void Document::UpdateStyleAndLayout(DocumentUpdateReason reason) {
DCHECK(IsMainThread());
LocalFrameView* frame_view = View();
......
......@@ -1611,6 +1611,7 @@ class CORE_EXPORT Document : public ContainerNode,
void MarkHasFindInPageRequest();
void MarkHasFindInPageContentVisibilityActiveMatch();
void MarkHasFindInPageBeforematchExpandedHiddenMatchable();
void CancelPendingJavaScriptUrls();
......@@ -1656,6 +1657,10 @@ class CORE_EXPORT Document : public ContainerNode,
FRIEND_TEST_ALL_PREFIXES(FrameFetchContextSubresourceFilterTest,
DuringOnFreeze);
FRIEND_TEST_ALL_PREFIXES(DocumentTest, FindInPageUkm);
FRIEND_TEST_ALL_PREFIXES(TextFinderSimTest,
BeforeMatchExpandedHiddenMatchableUkm);
FRIEND_TEST_ALL_PREFIXES(TextFinderSimTest,
BeforeMatchExpandedHiddenMatchableUkmNoHandler);
class NetworkStateObserver;
friend class AXContext;
......@@ -2153,6 +2158,7 @@ class CORE_EXPORT Document : public ContainerNode,
// Records find-in-page metrics, which are sent to UKM on shutdown.
bool had_find_in_page_request_ = false;
bool had_find_in_page_render_subtree_active_match_ = false;
bool had_find_in_page_beforematch_expanded_hidden_matchable_ = false;
// To reduce the API noisiness an explicit deny decision will set a
// flag that auto rejects the promise without the need for an IPC
......
......@@ -68,6 +68,21 @@
namespace blink {
namespace {
// Returns the element which the beforematch event should be fired on given a
// matching range.
Element* GetBeforematchElement(const Range& range) {
// Find-in-page matches can't span multiple block-level elements (because
// the text will be broken by newlines between blocks), so first we find the
// block-level element which contains the match.
// This means we only need to traverse up from one node in the range, in
// this case we are traversing from the start position of the range.
return EnclosingBlock(range.StartPosition(), kCannotCrossEditingBoundary);
}
} // namespace
TextFinder::FindMatch::FindMatch(Range* range, int ordinal)
: range_(range), ordinal_(ordinal) {}
......@@ -209,6 +224,11 @@ bool TextFinder::FindInternal(int identifier,
scroll_context->range = active_match_.Get();
scroll_context->first_match = first_match ? first_match : active_match_.Get();
scroll_context->wrapped_around = wrapped_around;
Element* beforematch_element = GetBeforematchElement(*active_match_);
scroll_context->was_match_hidden =
beforematch_element &&
DisplayLockUtilities::NearestHiddenMatchableInclusiveAncestor(
*beforematch_element);
if (options.run_synchronously_for_testing) {
FireBeforematchEvent(std::move(scroll_context));
} else {
......@@ -809,20 +829,14 @@ void TextFinder::FireBeforematchEvent(
}
if (RuntimeEnabledFeatures::BeforeMatchEventEnabled()) {
// Find-in-page matches can't span multiple block-level elements (because
// the text will be broken by newlines between blocks), so first we find the
// block-level element which contains the match.
// This means we only need to traverse up from one node in the range, in
// this case we are traversing from the start position of the range.
Element* enclosing_block = EnclosingBlock(context->range->StartPosition(),
kCannotCrossEditingBoundary);
Element* beforematch_element = GetBeforematchElement(*context->range);
// Note that we don't check the `range.EndPosition()` since we just activate
// the beginning of the range. In find-in-page cases, the end position is
// the same since the matches cannot cross block boundaries. However, in
// scroll-to-text, the range might be different, but we still just activate
// the beginning of the range. See
// https://github.com/WICG/display-locking/issues/125 for more details.
if (enclosing_block) {
if (beforematch_element) {
// If the beforematch event handler causes layout shift, then we should
// give it layout shift allowance because it is responding to the user
// initiated find-in-page.
......@@ -830,7 +844,7 @@ void TextFinder::FireBeforematchEvent(
.GetFrameView()
->GetLayoutShiftTracker()
.NotifyFindInPageInput();
enclosing_block->DispatchEvent(
beforematch_element->DispatchEvent(
*Event::CreateBubble(event_type_names::kBeforematch));
}
// TODO(jarhar): Consider what to do based on DOM/style modifications made
......@@ -858,12 +872,11 @@ void TextFinder::Scroll(std::unique_ptr<AsyncScrollContext> context) {
// case we shouldn't scroll to it.
// Likewise, if the target scroll element is display locked, then we shouldn't
// scroll to it.
Element* enclosing_block = EnclosingBlock(context->range->StartPosition(),
kCannotCrossEditingBoundary);
Element* beforematch_element = GetBeforematchElement(*context->range);
if (context->range->collapsed() ||
(enclosing_block &&
(beforematch_element &&
DisplayLockUtilities::NearestHiddenMatchableInclusiveAncestor(
*enclosing_block))) {
*beforematch_element))) {
// If the range we were going to scroll to was removed or display locked,
// then we should continue to search for the next match.
// We don't need to worry about the case where another Find has already been
......@@ -879,6 +892,12 @@ void TextFinder::Scroll(std::unique_ptr<AsyncScrollContext> context) {
return;
}
if (context->was_match_hidden) {
GetFrame()
->GetDocument()
->MarkHasFindInPageBeforematchExpandedHiddenMatchable();
}
ScrollToVisible(context->range);
// If the user is browsing a page with autosizing, adjust the zoom to the
......
......@@ -149,8 +149,13 @@ class CORE_EXPORT TextFinder final : public GarbageCollected<TextFinder> {
Persistent<Range> first_match;
bool wrapped_around;
// Range to fire beforematch on and scroll to.
// Range to fire beforematch on and scroll to. active_match_ may get
// unassigned during the async steps, so we need to save it here.
Persistent<Range> range;
// If the match had the content-visibility: hidden-matchable property in the
// ancestor chain at the time of finding the matching text.
bool was_match_hidden;
};
// Same as Find but with extra internal parameters used to track incremental
......
......@@ -4,6 +4,7 @@
#include "third_party/blink/renderer/core/editing/finder/text_finder.h"
#include "components/ukm/test_ukm_recorder.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/mojom/frame/find_in_page.mojom-blink.h"
#include "third_party/blink/public/platform/platform.h"
......@@ -767,6 +768,87 @@ TEST_F(TextFinderSimTest, BeforeMatchEventAsyncExpandHighlight) {
EXPECT_TRUE(marker->GetType() == DocumentMarker::kTextMatch);
}
TEST_F(TextFinderSimTest, BeforeMatchExpandedHiddenMatchableUkm) {
SimRequest request("https://example.com/test.html", "text/html");
LoadURL("https://example.com/test.html");
request.Complete(R"HTML(
<!DOCTYPE html>
<style>
.hidden {
content-visibility: hidden-matchable;
}
</style>
<div id=hiddenid class=hidden>hidden</div>
<script>
hiddenid.addEventListener('beforematch', () => {
requestAnimationFrame(() => {
hiddenid.classList.remove('hidden');
}, 0);
});
</script>
)HTML");
Compositor().BeginFrame();
GetDocument().ukm_recorder_ = std::make_unique<ukm::TestUkmRecorder>();
auto* recorder =
static_cast<ukm::TestUkmRecorder*>(GetDocument().UkmRecorder());
EXPECT_EQ(recorder->entries_count(), 0u);
auto forced_activatable_locks = GetDocument()
.GetDisplayLockDocumentState()
.GetScopedForceActivatableLocks();
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kFindInPage);
GetTextFinder().Find(/*identifier=*/0, WebString(String("hidden")),
*mojom::blink::FindOptions::New(),
/*wrap_within_frame=*/false);
Compositor().BeginFrame();
Compositor().BeginFrame();
auto entries = recorder->GetEntriesByName("Blink.FindInPage");
ASSERT_EQ(entries.size(), 1u);
EXPECT_TRUE(ukm::TestUkmRecorder::EntryHasMetric(
entries[0], "BeforematchExpandedHiddenMatchable"));
}
TEST_F(TextFinderSimTest, BeforeMatchExpandedHiddenMatchableUkmNoHandler) {
SimRequest request("https://example.com/test.html", "text/html");
LoadURL("https://example.com/test.html");
request.Complete(R"HTML(
<!DOCTYPE html>
<style>
.hidden {
content-visibility: hidden-matchable;
}
</style>
<div id=hiddenid class=hidden>hidden</div>
)HTML");
Compositor().BeginFrame();
GetDocument().ukm_recorder_ = std::make_unique<ukm::TestUkmRecorder>();
auto* recorder =
static_cast<ukm::TestUkmRecorder*>(GetDocument().UkmRecorder());
EXPECT_EQ(recorder->entries_count(), 0u);
auto forced_activatable_locks = GetDocument()
.GetDisplayLockDocumentState()
.GetScopedForceActivatableLocks();
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kFindInPage);
GetTextFinder().Find(/*identifier=*/0, WebString(String("hidden")),
*mojom::blink::FindOptions::New(),
/*wrap_within_frame=*/false);
Compositor().BeginFrame();
Compositor().BeginFrame();
auto entries = recorder->GetEntriesByName("Blink.FindInPage");
EXPECT_EQ(entries.size(), 0u);
}
TEST_F(TextFinderTest, FindTextAcrossCommentNode) {
GetDocument().body()->setInnerHTML(
"<span>abc</span><!--comment--><span>def</span>");
......
......@@ -1697,6 +1697,12 @@ be describing additional metrics about the same event.
Logged once per page; indicates the type of find-in-page behaviors users
utilizied.
</summary>
<metric name="BeforematchExpandedHiddenMatchable">
<summary>
Boolean indicating that a find-in-page match was hidden but became visible
after the beforematch event was fired.
</summary>
</metric>
<metric name="DidHaveRenderSubtreeMatch">
<summary>
Boolean indicating whether there was an active match in content-visibility
......
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