Commit a35bd30b authored by Nick Burris's avatar Nick Burris Committed by Commit Bot

[TextUrlFragments] Improve scroll into view and highlighting of target.

I previously changed the scroll into view behavior to edge-aligned, with
the idea that we don't want to scroll to the middle of a long text
range, but this results in the scroll position being at the bottom of
the page for shorter text ranges. This patch reverts to centering the
text range, but specifically the top of the text range to handle the
case of long ranges.

Also changes the text highlight color to light yellow instead of orange
as recommended.

Bug: 958545 958544
Change-Id: I8d35d916acec0199db2a33a6d05089311129d66d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1592399
Commit-Queue: Nick Burris <nburris@chromium.org>
Reviewed-by: default avatarDavid Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656404}
parent 6cb27ead
...@@ -127,6 +127,10 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) { ...@@ -127,6 +127,10 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) {
LayoutRect bounding_box = LayoutRect(ComputeTextRect(range)); LayoutRect bounding_box = LayoutRect(ComputeTextRect(range));
// Set the bounding box height to zero because we want to center the top of
// the text range.
bounding_box.SetHeight(LayoutUnit());
DCHECK(range.Nodes().begin() != range.Nodes().end()); DCHECK(range.Nodes().begin() != range.Nodes().end());
Node& node = *range.Nodes().begin(); Node& node = *range.Nodes().begin();
...@@ -136,7 +140,7 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) { ...@@ -136,7 +140,7 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) {
node.GetLayoutObject()->ScrollRectToVisible( node.GetLayoutObject()->ScrollRectToVisible(
bounding_box, bounding_box,
WebScrollIntoViewParams(ScrollAlignment::kAlignCenterIfNeeded, WebScrollIntoViewParams(ScrollAlignment::kAlignCenterIfNeeded,
ScrollAlignment::kAlignToEdgeIfNeeded, ScrollAlignment::kAlignCenterIfNeeded,
kProgrammaticScroll)); kProgrammaticScroll));
} }
EphemeralRange dom_range = EphemeralRange dom_range =
...@@ -145,7 +149,7 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) { ...@@ -145,7 +149,7 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) {
// TODO(nburris): Determine what we should do with overlapping text matches. // TODO(nburris): Determine what we should do with overlapping text matches.
// Currently, AddTextMatchMarker will crash when adding an overlapping marker. // Currently, AddTextMatchMarker will crash when adding an overlapping marker.
frame_->GetDocument()->Markers().AddTextMatchMarker( frame_->GetDocument()->Markers().AddTextMatchMarker(
dom_range, TextMatchMarker::MatchStatus::kActive); dom_range, TextMatchMarker::MatchStatus::kInactive);
frame_->GetEditor().SetMarkedTextMatchesAreHighlighted(true); frame_->GetEditor().SetMarkedTextMatchesAreHighlighted(true);
} }
......
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