Commit 0467a29d authored by Oriol Brufau's avatar Oriol Brufau Committed by Chromium LUCI CQ

[editing] Ranges with equivalent positions should be collapsed

Before this patch, for an ephemeral range created with different but
equivalent positions, IsCollapsed() would return false. Analogously,
for a selection created with an extent different than but equivalent to
the the base, IsCaret() would return false.

This could result in SelectionTypeAdjuster::AdjustSelection trying to
create an ephemeral range with an end position preceding the start one,
producing a DCHECK failure.

Therefore, this patch changes the EphemeralRangeTemplate constructor and
SelectionTemplate::Builder::Extend so that the end (or extent) position
is set to the start (or base) one if they are equivalent.

However, this is not done if the start (or base) position is a <br>,
since editing/deleting/delete_after_block_image.html would fail.

Bug: 1143292

TEST=EphemeralRangeTest.EquivalentPositions
TEST=SelectionTest.EquivalentPositions

Change-Id: I3e9194b09c4bda893c05a70fdae858aa6aa22f21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2550834
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: default avatarYoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832687}
parent 090a7e86
...@@ -139,9 +139,10 @@ TEST_F(ApplyBlockElementCommandTest, FormatBlockCrossingUserModifyBoundary) { ...@@ -139,9 +139,10 @@ TEST_F(ApplyBlockElementCommandTest, FormatBlockCrossingUserModifyBoundary) {
auto* command = MakeGarbageCollected<FormatBlockCommand>(GetDocument(), auto* command = MakeGarbageCollected<FormatBlockCommand>(GetDocument(),
html_names::kPreTag); html_names::kPreTag);
// Shouldn't crash here. // Shouldn't crash here.
EXPECT_FALSE(command->Apply()); EXPECT_TRUE(command->Apply());
EXPECT_EQ( EXPECT_EQ(
"^<b style=\"-webkit-user-modify:read-only\"><button>|</button></b>", "<pre>|<br></pre>"
"<b style=\"-webkit-user-modify:read-only\"><button></button></b>",
GetSelectionTextFromBody()); GetSelectionTextFromBody());
} }
......
...@@ -27,7 +27,7 @@ EphemeralRangeTemplate<Strategy>::EphemeralRangeTemplate( ...@@ -27,7 +27,7 @@ EphemeralRangeTemplate<Strategy>::EphemeralRangeTemplate(
const PositionTemplate<Strategy>& start, const PositionTemplate<Strategy>& start,
const PositionTemplate<Strategy>& end) const PositionTemplate<Strategy>& end)
: start_position_(start), : start_position_(start),
end_position_(end) end_position_(start.IsEquivalent(end) ? start : end)
#if DCHECK_IS_ON() #if DCHECK_IS_ON()
, ,
dom_tree_version_(start.IsNull() ? 0 dom_tree_version_(start.IsNull() ? 0
......
...@@ -226,4 +226,28 @@ TEST_F(EphemeralRangeTest, commonAncesstorFlatTree) { ...@@ -226,4 +226,28 @@ TEST_F(EphemeralRangeTest, commonAncesstorFlatTree) {
range.CommonAncestorContainer()); range.CommonAncestorContainer());
} }
TEST_F(EphemeralRangeTest, EquivalentPositions) {
SetBodyContent(
"<div id='first'></div>"
"<div id='last'></div>");
Element* first = GetDocument().getElementById("first");
Element* last = GetDocument().getElementById("last");
Position after_first = Position::AfterNode(*first);
Position before_last = Position::BeforeNode(*last);
// Test ranges created with different but equivalent positions.
EXPECT_NE(after_first, before_last);
EXPECT_TRUE(after_first.IsEquivalent(before_last));
EphemeralRange range1(after_first, before_last);
EXPECT_TRUE(range1.IsCollapsed());
EXPECT_EQ(after_first, range1.StartPosition());
EXPECT_EQ(after_first, range1.EndPosition());
EphemeralRange range2(before_last, after_first);
EXPECT_TRUE(range2.IsCollapsed());
EXPECT_EQ(before_last, range2.StartPosition());
EXPECT_EQ(before_last, range2.EndPosition());
}
} // namespace blink } // namespace blink
...@@ -273,6 +273,8 @@ SelectionTemplate<Strategy>::Builder::Extend( ...@@ -273,6 +273,8 @@ SelectionTemplate<Strategy>::Builder::Extend(
DCHECK_EQ(selection_.GetDocument(), position.GetDocument()); DCHECK_EQ(selection_.GetDocument(), position.GetDocument());
DCHECK(selection_.Base().IsConnected()) << selection_.Base(); DCHECK(selection_.Base().IsConnected()) << selection_.Base();
DCHECK(selection_.AssertValid()); DCHECK(selection_.AssertValid());
if (selection_.extent_.IsEquivalent(position))
return *this;
selection_.extent_ = position; selection_.extent_ = position;
selection_.direction_ = Direction::kNotComputed; selection_.direction_ = Direction::kNotComputed;
return *this; return *this;
......
...@@ -119,4 +119,35 @@ TEST_F(SelectionTest, SetAsBacwardAndForward) { ...@@ -119,4 +119,35 @@ TEST_F(SelectionTest, SetAsBacwardAndForward) {
EXPECT_EQ(EphemeralRange(start, start), collapsed_selection.ComputeRange()); EXPECT_EQ(EphemeralRange(start, start), collapsed_selection.ComputeRange());
} }
TEST_F(SelectionTest, EquivalentPositions) {
SetBodyContent(
"<div id='first'></div>"
"<div id='last'></div>");
Element* first = GetDocument().getElementById("first");
Element* last = GetDocument().getElementById("last");
Position after_first = Position::AfterNode(*first);
Position before_last = Position::BeforeNode(*last);
// Test selections created with different but equivalent positions.
EXPECT_NE(after_first, before_last);
EXPECT_TRUE(after_first.IsEquivalent(before_last));
for (bool reversed : {false, true}) {
const Position& start = reversed ? before_last : after_first;
const Position& end = reversed ? after_first : before_last;
EphemeralRange range(start, end);
const SelectionInDOMTree& selection =
SelectionInDOMTree::Builder().Collapse(start).Extend(end).Build();
EXPECT_EQ(
selection,
SelectionInDOMTree::Builder().SetAsForwardSelection(range).Build());
EXPECT_TRUE(selection.IsCaret());
EXPECT_EQ(range, selection.ComputeRange());
EXPECT_EQ(start, selection.Base());
EXPECT_EQ(start, selection.Extent());
}
}
} // 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