corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1939065 - Optimize HTMLEditor::DoSplitNode and HTMLEditor::DoJoinNodes for IMEContentObserver r=m_kato

Browse Source 
`HTMLEditor::DoSplitNode` does:
1. Insert a new right node
2. Move content in left node into the right node

`HTMLEditor::DoJoinNodes` does:
1. Move content of the right node to the left node
2. Delete the left node

So, all things are done when connected. Therefore `IMEContentObserver` uses its
cache and it's harder to debug. Additionally, temporarily the touching range
becomes wider then the result at joining nodes. That could cause IME gets
received longer text change range. Therefore, they should do:

`HTMLEditor::DoSplitNode`:
1. Move content in left node to the orphan new right node (then, only removing
content is handled by `IMEContentObserver`)
2. Insert the right node (then, `IMEContentObserver` can compute the offset and
length only once)

`HTMLEditor::DoJoinNodes`:
1. Delete the right node (then, all moving content is treated as removed by
`IMEContentObserver`, i.e., it needs to compute offset/length once)
2. Move the non-connected right node children into the left node (then,
`IMEContentObserver` can cache all moving nodes and compute offset/length once)

Differential Revision: https://phabricator.services.mozilla.com/D232882
This commit is contained in:
Masayuki Nakano
2025-01-16 02:13:32 +00:00
parent 4aaf965b0d
commit b3e620e413
4 changed files with 108 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
dom/base/test/test_text_change_notifications_when_inserting_text_containing_line_breaks.html
View File

@@ -62,14 +62,14 @@ SimpleTest.waitForFocus(async () => {
for (const whiteSpace of ["normal", "pre"]) {
editingHost.style.whiteSpace = whiteSpace;
for (const collapseAt of [0, 1, 2, 3]) {
for (const insertionOffset of [0, 1, 2, 3]) {
for (const insertingText of ["\nabc\ndef",
"abc\ndef",
"abc\ndef\n",
"\n\nabc",
"abc\n\ndef"]) {
editingHost.textContent = "XYZ";
getSelection().collapse(editingHost.firstChild, collapseAt);
getSelection().collapse(editingHost.firstChild, insertionOffset);
await new Promise(resolve => {
requestAnimationFrame(() => requestAnimationFrame(resolve));
});
@@ -78,7 +78,7 @@ SimpleTest.waitForFocus(async () => {
await new Promise(resolve => {
requestAnimationFrame(() => requestAnimationFrame(resolve));
});
const middleOfText = collapseAt > 0 && collapseAt < "XYZ".length;
const middleOfText = insertionOffset > 0 && insertionOffset < "XYZ".length;
const expectSplit = middleOfText && whiteSpace == "normal";
const firstLineLength = insertingText.indexOf("\n");
const rightTextLength = expectSplit
@@ -86,7 +86,7 @@ SimpleTest.waitForFocus(async () => {
// At this time, the right half is removed from the `Text` and then,
// added it with new `Text`. Therefore, there is non-zero removed
// length.
? "XYZ".length - collapseAt
? "XYZ".length - insertionOffset
// However, if the linefeeds are preformatted or
// collapsed at start or end of a `Text`, we don't split the `Text`.
: 0;
@@ -94,41 +94,38 @@ SimpleTest.waitForFocus(async () => {
stringifyTextChanges(textChanges),
stringifyTextChanges([
{
offset: collapseAt,
offset: insertionOffset,
removedLength: rightTextLength,
addedLength: insertingText.length + rightTextLength,
},
]),
`white-space:${whiteSpace}: inserting text is "${
insertingText.replaceAll("\n", "\\n")
}" to offset ${collapseAt} in the Text`
}" to offset ${insertionOffset} in the Text`
);
textChanges = [];
document.execCommand("undo");
await new Promise(resolve => {
requestAnimationFrame(() => requestAnimationFrame(resolve));
});
// Finally, removing the inserted `Text` nodes and <br> elements causes joining the
// `Text` nodes around the inserted text. So, if the new text is just inserted, the
// text change data is same as above. On the other hand, if we did split, the offset
// is always 0, the removed length is the inserted text length and right text length
// at first split. Then, added text length is the right text length.
const joinText = expectSplit &&
(middleOfText || (firstLineLength > 0 && collapseAt == "XYZ".length));
const removeDataFromText =
expectSplit && collapseAt == "XYZ".length && firstLineLength > 0;
const expectJoin = expectSplit;
is(
stringifyTextChanges(textChanges),
stringifyTextChanges([
{
offset: !joinText || collapseAt == "XYZ".length ? collapseAt : 0,
removedLength: insertingText.length + (joinText && !removeDataFromText ? "XYZ".length : 0),
addedLength: joinText && !removeDataFromText ? "XYZ".length : 0,
// At undoing, the offset is always same as the inserted offset because we keep the
// left node at joining nodes so that simply removing the inserted data.
offset: insertionOffset,
// Removed length should be the length of inserted text and the text length of the
// right node at split because its data is once removed to move into the left `Text`.
removedLength: insertingText.length + rightTextLength,
// Added length should be the moved data length from the right node at split.
addedLength: rightTextLength,
},
]),
`white-space:${whiteSpace}: undoing inserted text is "${
insertingText.replaceAll("\n", "\\n")
}" to offset ${collapseAt} in the Text`
}" to offset ${insertionOffset} in the Text`
);
}
}

137
editor/libeditor/HTMLEditor.cpp
View File

@@ -5643,7 +5643,7 @@ Result<SplitNodeResult, nsresult> HTMLEditor::DoSplitNode(
if (NS_WARN_IF(!aStartOfRightNode.IsInContentNode())) {
return Err(NS_ERROR_INVALID_ARG);
}
MOZ_ASSERT(aStartOfRightNode.IsSetAndValid());
MOZ_DIAGNOSTIC_ASSERT(aStartOfRightNode.IsSetAndValid());
// Remember all selection points.
AutoTArray<SavedRange, 10> savedRanges;
@@ -5679,24 +5679,22 @@ Result<SplitNodeResult, nsresult> HTMLEditor::DoSplitNode(
return Err(NS_ERROR_FAILURE);
}
// Fix the child before mutation observer may touch the DOM tree.
nsIContent* firstChildOfRightNode = aStartOfRightNode.GetChild();
IgnoredErrorResult error;
parent->InsertBefore(
aNewNode, aStartOfRightNode.GetContainer()->GetNextSibling(), error);
if (MOZ_UNLIKELY(error.Failed())) {
NS_WARNING("nsINode::InsertBefore() failed");
return Err(error.StealNSResult());
}
MOZ_DIAGNOSTIC_ASSERT_IF(aStartOfRightNode.IsInTextNode(), aNewNode.IsText());
MOZ_DIAGNOSTIC_ASSERT_IF(!aStartOfRightNode.IsInTextNode(),
!aNewNode.IsText());
// For the performance of IMEContentObserver, we should move all data into
// aNewNode first because IMEContentObserver needs to compute moved content
// length only once when aNewNode is connected.
// If we are splitting a text node, we need to move its some data to the
// new text node.
if (aStartOfRightNode.IsInTextNode()) {
if (!aStartOfRightNode.IsEndOfContainer()) {
MOZ_DIAGNOSTIC_ASSERT_IF(aStartOfRightNode.IsInTextNode(), aNewNode.IsText());
MOZ_DIAGNOSTIC_ASSERT_IF(!aStartOfRightNode.IsInTextNode(),
!aNewNode.IsText());
const nsCOMPtr<nsIContent> firstChildOfRightNode =
aStartOfRightNode.GetChild();
nsresult rv = [&]() MOZ_NEVER_INLINE_DEBUG MOZ_CAN_RUN_SCRIPT {
if (aStartOfRightNode.IsEndOfContainer()) {
return NS_OK; // No content which should be moved into aNewNode.
}
if (aStartOfRightNode.IsInTextNode()) {
Text* originalTextNode = aStartOfRightNode.ContainerAs<Text>();
Text* newTextNode = aNewNode.AsText();
nsAutoString movingText;
@@ -5721,49 +5719,54 @@ Result<SplitNodeResult, nsresult> HTMLEditor::DoSplitNode(
DoSetText(MOZ_KnownLive(*newTextNode), movingText, error);
NS_WARNING_ASSERTION(!error.Failed(),
"EditorBase::DoSetText() failed, but ignored");
return NS_OK;
}
}
// If the node has been moved to different parent, we should do nothing
// since web apps should handle everything in such case.
else if (firstChildOfRightNode &&
aStartOfRightNode.GetContainer() !=
firstChildOfRightNode->GetParentNode()) {
NS_WARNING(
"The web app interrupted us and touched the DOM tree, we stopped "
"splitting anything");
} else {
// If the right node is new one and there is no children or splitting at
// end of the node, we need to do nothing.
if (!firstChildOfRightNode) {
// Do nothing.
}
// If the right node is new one and splitting at start of the container,
// we need to move all children to the new right node.
else if (!firstChildOfRightNode->GetPreviousSibling()) {
if (!firstChildOfRightNode->GetPreviousSibling()) {
// XXX Why do we ignore an error while moving nodes from the right
// node to the left node?
nsresult rv = MoveAllChildren(*aStartOfRightNode.GetContainer(),
EditorRawDOMPoint(&aNewNode, 0u));
if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) {
return Err(NS_ERROR_EDITOR_DESTROYED);
}
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
"HTMLEditor::MoveAllChildren() failed, but ignored");
"HTMLEditor::MoveAllChildren() failed");
return rv;
}
// If the right node is new one and splitting at middle of the node, we need
// to move inclusive next siblings of the split point to the new right node.
else {
// XXX Why do we ignore an error while moving nodes from the right node
// to the left node?
nsresult rv = MoveInclusiveNextSiblings(*firstChildOfRightNode,
EditorRawDOMPoint(&aNewNode, 0u));
if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) {
return Err(NS_ERROR_EDITOR_DESTROYED);
}
NS_WARNING_ASSERTION(
NS_SUCCEEDED(rv),
"HTMLEditor::MoveInclusiveNextSiblings() failed, but ignored");
}
// XXX Why do we ignore an error while moving nodes from the right node
// to the left node?
nsresult rv = MoveInclusiveNextSiblings(*firstChildOfRightNode,
EditorRawDOMPoint(&aNewNode, 0u));
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
"HTMLEditor::MoveInclusiveNextSiblings() failed");
return rv;
}();
// To avoid a dataloss bug, we should try to insert aNewNode even if we've
// already been destroyed.
if (NS_WARN_IF(!aStartOfRightNode.GetContainerParent())) {
return NS_WARN_IF(Destroyed()) ? Err(NS_ERROR_EDITOR_DESTROYED)
: Err(NS_ERROR_FAILURE);
}
// Finally, we should insert aNewNode which already has proper data or
// children.
IgnoredErrorResult error;
parent->InsertBefore(
aNewNode, aStartOfRightNode.GetContainer()->GetNextSibling(), error);
if (NS_WARN_IF(Destroyed())) {
return Err(NS_ERROR_EDITOR_DESTROYED);
}
if (MOZ_UNLIKELY(error.Failed())) {
NS_WARNING("nsINode::InsertBefore() failed");
return Err(error.StealNSResult());
}
if (NS_FAILED(rv)) {
NS_WARNING("Moving children from left node to right node failed");
return Err(rv);
}
// Handle selection
@@ -6040,17 +6043,20 @@ nsresult HTMLEditor::DoJoinNodes(nsIContent& aContentToKeep,
}
// OK, ready to do join now.
nsresult rv = [&]() MOZ_CAN_RUN_SCRIPT {
nsresult rv = [&]() MOZ_NEVER_INLINE_DEBUG MOZ_CAN_RUN_SCRIPT {
// If it's a text node, just shuffle around some text.
if (aContentToKeep.IsText() && aContentToRemove.IsText()) {
nsAutoString rightText;
nsAutoString leftText;
aContentToRemove.AsText()->GetData(rightText);
aContentToKeep.AsText()->GetData(leftText);
leftText += rightText;
// Delete the node first to minimize the text change range from
// IMEContentObserver of view.
aContentToRemove.Remove();
// Even if we've already destroyed, let's update aContentToKeep for
// avoiding a dataloss bug.
IgnoredErrorResult ignoredError;
DoSetText(MOZ_KnownLive(*aContentToKeep.AsText()), leftText,
ignoredError);
DoInsertText(MOZ_KnownLive(*aContentToKeep.AsText()),
aContentToKeep.AsText()->TextDataLength(), rightText,
ignoredError);
if (NS_WARN_IF(Destroyed())) {
return NS_ERROR_EDITOR_DESTROYED;
}
@@ -6061,28 +6067,25 @@ nsresult HTMLEditor::DoJoinNodes(nsIContent& aContentToKeep,
// Otherwise it's an interior node, so shuffle around the children.
AutoTArray<OwningNonNull<nsIContent>, 64> arrayOfChildContents;
HTMLEditUtils::CollectAllChildren(aContentToRemove, arrayOfChildContents);
// Delete the node first to minimize the text change range from
// IMEContentObserver of view.
aContentToRemove.Remove();
// Even if we've already destroyed, let's update aContentToKeep for avoiding
// a dataloss bug.
nsresult rv = NS_OK;
for (const OwningNonNull<nsIContent>& child : arrayOfChildContents) {
IgnoredErrorResult error;
aContentToKeep.AppendChild(child, error);
if (NS_WARN_IF(Destroyed())) {
return NS_ERROR_EDITOR_DESTROYED;
}
if (error.Failed()) {
if (MOZ_UNLIKELY(error.Failed())) {
NS_WARNING("nsINode::AppendChild() failed");
return error.StealNSResult();
rv = error.StealNSResult();
}
}
return NS_OK;
}();
// Delete the extra node.
if (NS_SUCCEEDED(rv)) {
aContentToRemove.Remove();
if (NS_WARN_IF(Destroyed())) {
return NS_ERROR_EDITOR_DESTROYED;
}
}
return rv;
}();
if (MOZ_LIKELY(oldPointAtRightContent.IsSet())) {
DebugOnly<nsresult> rvIgnored = RangeUpdaterRef().SelAdjJoinNodes(

1
editor/libeditor/HTMLEditorDeleteHandler.cpp
View File

@@ -7627,6 +7627,7 @@ nsresult HTMLEditor::MoveChildrenBetween(
}
}
if (NS_WARN_IF(
newContainer->IsInComposedDoc() &&
!EditorUtils::IsEditableContent(*newContainer, EditorType::HTML))) {
return NS_ERROR_EDITOR_UNEXPECTED_DOM_TREE;
}

34
widget/tests/window_composition_text_querycontent.xhtml
View File

@@ -9953,7 +9953,7 @@ async function runIMEContentObserverTest()
dumpUnexpectedNotifications(description, 1);
// "a[]d"
description = aDescription + "removing selected '\n' with pressing Delete";
description = aDescription + "removing selected '\\n' with pressing Delete";
waitNotifications = promiseReceiveNotifications();
synthesizeKey("KEY_Delete", {shiftKey: true}, win, callback);
await waitNotifications;
@@ -10238,18 +10238,18 @@ async function runIMEContentObserverTest()
checkPositionChangeNotification(notifications[2], description);
dumpUnexpectedNotifications(description, 3);
// "aB[]d"
// "aB[]d" or "<block>aB[]d</block>
description = aDescription + "removing a line break after 'B' with pressing Backspace";
waitNotifications = promiseReceiveNotifications();
synthesizeKey("KEY_Backspace", {}, win, callback);
await waitNotifications;
ensureToRemovePrecedingPositionChangeNotification();
if (isDefaultParagraphSeparatorBlock) {
// Joining two blocks causes removing "aB</block><block>d</block>" and inserting "aBd</block>"
// Joining two blocks causes removing "</block><block>d</block>" and inserting "d</block>"
checkTextChangeNotification(notifications[0], description, {
offset: offsetAtContainer,
removedLength: getNativeText("aB\nd").length,
addedLength: "aBd".length,
offset: offsetAtContainer + getNativeText("aB".length),
removedLength: getNativeText("\nd").length,
addedLength: "d".length,
});
checkSelectionChangeNotification(notifications[1], description, { offset: 2 + offsetAtContainer, text: "" });
checkPositionChangeNotification(notifications[2], description);
@@ -10307,18 +10307,18 @@ async function runIMEContentObserverTest()
checkSelectionChangeNotification(notifications[0], description, { offset: 1 + offsetAtContainer, text: kLF, reversed: true });
dumpUnexpectedNotifications(description, 1);
// "a[]d"
// "a[]d" or "<block>a[]d</block>"
description = aDescription + "removing selected '\\n' with pressing Delete";
waitNotifications = promiseReceiveNotifications();
synthesizeKey("KEY_Delete", {}, win, callback);
await waitNotifications;
ensureToRemovePrecedingPositionChangeNotification();
if (isDefaultParagraphSeparatorBlock) {
// Joining the blocks causes removing "a</block><block>d</block>" and inserting "<block>ad</block>".
// Joining the blocks causes removing "</block><block>d</block>" and inserting "d</block>".
checkTextChangeNotification(notifications[0], description, {
offset: offsetAtContainer,
removedLength: getNativeText("a\nd").length,
addedLength: getNativeText("ad").length,
offset: offsetAtContainer + "a".length,
removedLength: getNativeText("\nd").length,
addedLength: getNativeText("d").length,
});
} else {
checkTextChangeNotification(notifications[0], description, {
@@ -10387,8 +10387,16 @@ async function runIMEContentObserverTest()
await waitNotifications;
ensureToRemovePrecedingPositionChangeNotification();
is(aElement.innerHTML, "<div>13<div>4</div>5</div>", description + " should remove '<div>2</div>'");
// It causes removing '1<div>2<div>3</div></div>' and inserting '13<div>'.
checkTextChangeNotification(notifications[0], description, { offset: kLFLen + offsetAtStart, removedLength: getNativeText("1\n2\n3").length, addedLength: getNativeText("13\n").length });
checkTextChangeNotification(
notifications[0],
description,
{
offset: kLFLen + offsetAtStart + "1".length,
// It causes removing '<div>2<div>3</div></div>' and inserting '13<div>'.
removedLength: getNativeText("\n2\n3").length,
addedLength: getNativeText("3\n").length
}
);
checkSelectionChangeNotification(notifications[1], description, { offset: getNativeText("\n1").length + offsetAtStart, text: "" });
checkPositionChangeNotification(notifications[2], description);
dumpUnexpectedNotifications(description, 3);