Skip to content

Commit

Permalink
Bug 1740416 - Make HTMLEditor::HandleInsertParagraphInParagraph() c…
Browse files Browse the repository at this point in the history 
…all `WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement()` before splitting a text node r=m_kato,smaug

It does the following things when caret is collapsed in a text node in a `<p>`
or `<div>` element.

1. Split the text node containing caret to insert `<br>` element
2. Insert `<br>` element after it
3. Split ancestor elements which inclusive descendants of the `<p>` or `<div>`
4. Delete the `<br>` element if unnecessary from the left paragraph

Floorp-Projects#3 and Floorp-Projects#4 are performed by `HTMLEditor::SplitParagraph()` and it calls
`WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement()` correctly before
splitting the block.  However, in the case (caret is at middle of a text node),
the text has already been split to 2 nodes because of Floorp-Projects#1.  Therefore, it fails
to handle to keep the white-space visibility.

So that I believe that the root cause of this bug is, the method does much
complicated things which are required, and doing the redundant things will
eat memory space due to undo transactions.  However, for now, I'd like to fix
this with a simple patch which just call the preparation method before splitting
the text node because I'd like to uplift this if it'd be approved (Note that
this is not a recent regression, the root cause was created by bug 92686 which
was fixed in 17 years ago:
<https://searchfox.org/mozilla-central/commit/2e66280faef73e9be218e00758d4eb738395ac83>,
but must be annoying bug for users who see this frequently).

The new WPTs are pass in Chrome.

Differential Revision: https://phabricator.services.mozilla.com/D130950
  • Loading branch information
@masayuki-nakano
masayuki-nakano committed Nov 12, 2021
1 parent b727f0d commit d5658de
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 11 deletions.
17 changes: 15 additions & 2 deletions dom/base/nsINode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,6 @@ std::ostream& operator<<(std::ostream& aStream, const nsINode& aNode) {
nsAutoString elemDesc;
const nsINode* curr = &aNode;
while (curr) {
const nsString& localName = curr->LocalName();
nsString id;
if (curr->IsElement()) {
curr->AsElement()->GetId(id);
Expand All @@ -735,12 +734,26 @@ std::ostream& operator<<(std::ostream& aStream, const nsINode& aNode) {
elemDesc = elemDesc + u"."_ns;
}

elemDesc = elemDesc + localName;
if (!curr->LocalName().IsEmpty()) {
elemDesc.Append(curr->LocalName());
} else {
elemDesc.Append(curr->NodeName());
}

if (!id.IsEmpty()) {
elemDesc = elemDesc + u"['"_ns + id + u"']"_ns;
}

if (curr->IsElement() &&
curr->AsElement()->HasAttr(nsGkAtoms::contenteditable)) {
nsAutoString val;
curr->AsElement()->GetAttr(nsGkAtoms::contenteditable, val);
elemDesc = elemDesc + u"[contenteditable=\""_ns + val + u"\"]"_ns;
}
if (curr->IsDocument() && curr->IsInDesignMode()) {
elemDesc.Append(u"[designMode=\"on\"]"_ns);
}

curr = curr->GetParentNode();
}

Expand Down
3 changes: 2 additions & 1 deletion editor/libeditor/EditorDOMPoint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,8 @@ class EditorDOMPointBase final {
const SelfType& aDOMPoint) {
aStream << "{ mParent=" << aDOMPoint.mParent.get();
if (aDOMPoint.mParent) {
aStream << " (" << *aDOMPoint.mParent << ")";
aStream << " (" << *aDOMPoint.mParent
<< ", Length()=" << aDOMPoint.mParent->Length() << ")";
}
aStream << ", mChild=" << aDOMPoint.mChild.get();
if (aDOMPoint.mChild) {
Expand Down
32 changes: 25 additions & 7 deletions editor/libeditor/HTMLEditSubActionHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7151,8 +7151,30 @@ EditActionResult HTMLEditor::HandleInsertParagraphInParagraph(
}
} else {
if (doesCRCreateNewP) {
// XXX We split a text node here if caret is middle of it to insert
// <br> element **before** splitting aParentDivOrP. Then, if
// the <br> element becomes unnecessary, it'll be removed again.
// So this does much more complicated things than what we want to
// do here. We should handle this case separately to make the code
// much simpler.
Result<EditorDOMPoint, nsresult> pointToSplitOrError =
WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement(
*this, pointToSplitParentDivOrP, aParentDivOrP);
if (MOZ_UNLIKELY(NS_WARN_IF(Destroyed()))) {
return EditActionResult(NS_ERROR_EDITOR_DESTROYED);
}
if (MOZ_UNLIKELY(pointToSplitOrError.isErr())) {
NS_WARNING(
"WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement() "
"failed");
return EditActionResult(pointToSplitOrError.unwrapErr());
}
MOZ_ASSERT(pointToSplitOrError.inspect().IsSetAndValid());
if (pointToSplitOrError.inspect().IsSet()) {
pointToSplitParentDivOrP = pointToSplitOrError.unwrap();
}
ErrorResult error;
nsCOMPtr<nsIContent> newLeftDivOrP =
nsCOMPtr<nsIContent> newLeftTextNode =
SplitNodeWithTransaction(pointToSplitParentDivOrP, error);
if (NS_WARN_IF(Destroyed())) {
error = NS_ERROR_EDITOR_DESTROYED;
Expand All @@ -7162,16 +7184,12 @@ EditActionResult HTMLEditor::HandleInsertParagraphInParagraph(
"HTMLEditor::SplitNodeWithTransaction() failed");
return EditActionResult(error.StealNSResult());
}
pointToSplitParentDivOrP.SetToEndOf(newLeftDivOrP);
pointToSplitParentDivOrP.SetToEndOf(newLeftTextNode);
}

// We need to put new <br> after the left node if given node was split
// above.
pointToInsertBR.Set(pointToSplitParentDivOrP.GetContainer());
DebugOnly<bool> advanced = pointToInsertBR.AdvanceOffset();
NS_WARNING_ASSERTION(advanced,
"Failed to advance offset to after the container "
"of selection start");
pointToInsertBR.SetAfter(pointToSplitParentDivOrP.GetContainer());
}
} else {
// not in a text node.
Expand Down
2 changes: 1 addition & 1 deletion editor/libeditor/tests/test_bug607584.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
var paragraphs = content.querySelectorAll("p");
is(paragraphs.length, 2, "The paragraph should be split in two");
is(paragraphs[0].textContent, "Hello", "The first paragraph should have the correct content");
is(paragraphs[1].textContent, " world", "The second paragraph should have the correct content");
is(paragraphs[1].textContent, "\u00A0world", "The second paragraph should have the correct content");
is(paragraphs[0].getAttribute("id"), "foo", "The id of the first paragraph should be retained");
is(paragraphs[1].hasAttribute("id"), false, "The second paragraph shouldn't have an ID");
SimpleTest.finish();
Expand Down
105 changes: 105 additions & 0 deletions testing/web-platform/tests/editing/data/insertparagraph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1990,4 +1990,109 @@ var browserTests = [
"<dl contenteditable=\"false\"><dd contenteditable=\"\"><div>ab</div><div>cd<br></div>/dd></dl>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],

// The first white-space of the second paragraph must be &nbsp; for making it
// visible, but the other things do not matter for the following tests.
["<div>a[] b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
"<div>a</div><div>&nbsp;b</div>",
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
// And if the first paragraph ends with white-space, the text node should be
// followed by <br> element.
["<div>a []b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a <br></div><div>b</div>",
"<div>a&nbsp;</div><div>b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a&nbsp;[] b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a <br></div><div>&nbsp;b</div>",
"<div>a&nbsp;</div><div>&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a []&nbsp;b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a <br></div><div>&nbsp;b</div>",
"<div>a&nbsp;</div><div>&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
// These tests do not mind about the white-space sequence because it's not
// important here. That's tested by
// editing/other/white-spaces-after-execCommand-*.tentative.html
["<div>a&nbsp;[]&nbsp; b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;</div><div>&nbsp; b</div>",
"<div>a <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a&nbsp;[] &nbsp;b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;</div><div>&nbsp; b</div>",
"<div>a <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a []&nbsp; b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;</div><div>&nbsp; b</div>",
"<div>a <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a&nbsp;&nbsp;[]&nbsp; b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a&nbsp; <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp; b</div>",
"<div>a &nbsp;</div><div>&nbsp; b</div>",
"<div>a&nbsp; <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp;&nbsp;b</div>",
"<div>a &nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a&nbsp; []&nbsp; b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a&nbsp; <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp; b</div>",
"<div>a &nbsp;</div><div>&nbsp; b</div>",
"<div>a&nbsp; <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp;&nbsp;b</div>",
"<div>a &nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a &nbsp;[]&nbsp; b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a&nbsp; <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp; b</div>",
"<div>a &nbsp;</div><div>&nbsp; b</div>",
"<div>a&nbsp; <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp;&nbsp;b</div>",
"<div>a &nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a&nbsp;&nbsp;[] &nbsp;b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a&nbsp; <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp; b</div>",
"<div>a &nbsp;</div><div>&nbsp; b</div>",
"<div>a&nbsp; <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp;&nbsp;b</div>",
"<div>a &nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
["<div>a &nbsp;[] &nbsp;b</div>",
[["defaultparagraphseparator","div"],["insertparagraph",""]],
["<div>a&nbsp; <br></div><div>&nbsp; b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp; b</div>",
"<div>a &nbsp;</div><div>&nbsp; b</div>",
"<div>a&nbsp; <br></div><div>&nbsp;&nbsp;b</div>",
"<div>a&nbsp;&nbsp;</div><div>&nbsp;&nbsp;b</div>",
"<div>a &nbsp;</div><div>&nbsp;&nbsp;b</div>"],
[true,true],
{"insertparagraph":[false,false,"",false,false,""]}],
]

0 comments on commit d5658de

Please sign in to comment.