Bug 1297414 - formatBlock should move non-editable nodes; r=masayuki
Before this change, if you did formatBlock on "a<span
contenteditable=false>b</span>c", it would become "acb", because the "a"
and "c" would be moved to the front of the node, and the "b" would be
left alone at the end because the editing code doesn't want to move it.
Now we will move the "b" as well, even though it's not editable, so that
the node remains "abc". The rule is that a non-editable element cannot
have its attributes or children changed, but it can have its parent
changed, so there's nothing wrong with moving it here.
On the way, I fixed an exception in insert*List if there was an
uneditable inline node around. I don't intend to fix all the todo's in
the test, but now it should have better coverage, at least.
MozReview-Commit-ID: 3okcGq4an3f
--- a/dom/html/test/mochitest.ini
+++ b/dom/html/test/mochitest.ini
@@ -257,17 +257,16 @@ skip-if = toolkit == 'android' #TIMED_OU
[test_bug401160.xhtml]
[test_bug405242.html]
[test_bug406596.html]
[test_bug417760.html]
[test_bug421640.html]
[test_bug424698.html]
[test_bug428135.xhtml]
[test_bug430351.html]
-[test_bug430392.html]
[test_bug441930.html]
[test_bug442801.html]
[test_bug448166.html]
[test_bug456229.html]
[test_bug458037.xhtml]
[test_bug460568.html]
[test_bug481335.xhtml]
skip-if = toolkit == 'android' #TIMED_OUT
deleted file mode 100644
--- a/dom/html/test/test_bug430392.html
+++ /dev/null
@@ -1,47 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<!--
-https://bugzilla.mozilla.org/show_bug.cgi?id=430392
--->
-<head>
- <title>Test for Bug 430392</title>
- <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
- <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
- <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
-</head>
-<body>
-<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=430392">Mozilla Bug 430392</a>
-<p id="display"></p>
-<div id="content">
- <div contenteditable="true" id="edit"> <span contenteditable="false">A</span> ; <span contenteditable="false">B</span> ; <span contenteditable="false">C</span> </div>
-</div>
-<pre id="test">
-<script class="testbody" type="text/javascript">
-
-/** Test for Bug 430392 **/
-
-function test() {
- var edit = document.getElementById("edit");
- var html = edit.innerHTML;
- document.getElementById("edit").focus();
-
- synthesizeKey("VK_RIGHT", {});
- synthesizeKey("VK_RIGHT", {});
- synthesizeKey("VK_RETURN", {});
- synthesizeKey("VK_RETURN", {});
- synthesizeKey("VK_BACK_SPACE", {});
- synthesizeKey("VK_BACK_SPACE", {});
-
- is(edit.innerHTML, html,
- "adding and then deleting returns should not change text");
-
- SimpleTest.finish();
-}
-
-SimpleTest.waitForExplicitFinish();
-addLoadEvent(test);
-
-</script>
-</pre>
-</body>
-</html>
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -3217,28 +3217,28 @@ HTMLEditRules::WillMakeList(Selection* a
curParent = EditorBase::GetNodeLocation(curNode, &offset);
// make sure we don't assemble content that is in different table cells
// into the same list. respect table cell boundaries when listifying.
if (curList && InDifferentTableElements(curList, curNode)) {
curList = nullptr;
}
- // if curNode is a Break, delete it, and quit remembering prev list item
- if (TextEditUtils::IsBreak(curNode)) {
+ // If curNode is a break, delete it, and quit remembering prev list item.
+ // If an empty inline container, delete it, but still remember the previous
+ // item.
+ NS_ENSURE_STATE(mHTMLEditor);
+ if (mHTMLEditor->IsEditable(curNode) && (TextEditUtils::IsBreak(curNode) ||
+ IsEmptyInline(curNode))) {
NS_ENSURE_STATE(mHTMLEditor);
rv = mHTMLEditor->DeleteNode(curNode);
NS_ENSURE_SUCCESS(rv, rv);
- prevListItem = nullptr;
- continue;
- } else if (IsEmptyInline(curNode)) {
- // if curNode is an empty inline container, delete it
- NS_ENSURE_STATE(mHTMLEditor);
- rv = mHTMLEditor->DeleteNode(curNode);
- NS_ENSURE_SUCCESS(rv, rv);
+ if (TextEditUtils::IsBreak(curNode)) {
+ prevListItem = nullptr;
+ }
continue;
}
if (HTMLEditUtils::IsList(curNode)) {
// do we have a curList already?
if (curList && !EditorUtils::IsDescendantOf(curNode, curList)) {
// move all of our children into curList. cheezy way to do it: move
// whole list and then RemoveContainer() on the list. ConvertListType
@@ -3493,23 +3493,16 @@ HTMLEditRules::WillMakeBasicBlock(Select
*aHandled = true;
// Contruct a list of nodes to act on.
nsTArray<OwningNonNull<nsINode>> arrayOfNodes;
rv = GetNodesFromSelection(aSelection, EditAction::makeBasicBlock,
arrayOfNodes);
NS_ENSURE_SUCCESS(rv, rv);
- // Remove all non-editable nodes. Leave them be.
- for (int32_t i = arrayOfNodes.Length() - 1; i >= 0; i--) {
- if (!htmlEditor->IsEditable(arrayOfNodes[i])) {
- arrayOfNodes.RemoveElementAt(i);
- }
- }
-
// If nothing visible in list, make an empty block
if (ListIsEmptyLine(arrayOfNodes)) {
// Get selection location
NS_ENSURE_STATE(aSelection.GetRangeAt(0) &&
aSelection.GetRangeAt(0)->GetStartParent());
OwningNonNull<nsINode> parent =
*aSelection.GetRangeAt(0)->GetStartParent();
int32_t offset = aSelection.GetRangeAt(0)->StartOffset();
@@ -6765,16 +6758,19 @@ HTMLEditRules::RemoveBlockStyle(nsTArray
// If curNode is a address, p, header, address, or pre, remove it
if (HTMLEditUtils::IsFormatNode(curNode)) {
// Process any partial progress saved
if (curBlock) {
nsresult rv = RemovePartOfBlock(*curBlock, *firstNode, *lastNode);
NS_ENSURE_SUCCESS(rv, rv);
firstNode = lastNode = curBlock = nullptr;
}
+ if (!mHTMLEditor->IsEditable(curNode)) {
+ continue;
+ }
// Remove current block
nsresult rv = htmlEditor->RemoveBlockContainer(*curNode->AsContent());
NS_ENSURE_SUCCESS(rv, rv);
} else if (curNode->IsAnyOfHTMLElements(nsGkAtoms::table,
nsGkAtoms::tr,
nsGkAtoms::tbody,
nsGkAtoms::td,
nsGkAtoms::li,
@@ -6782,16 +6778,19 @@ HTMLEditRules::RemoveBlockStyle(nsTArray
nsGkAtoms::div) ||
HTMLEditUtils::IsList(curNode)) {
// Process any partial progress saved
if (curBlock) {
nsresult rv = RemovePartOfBlock(*curBlock, *firstNode, *lastNode);
NS_ENSURE_SUCCESS(rv, rv);
firstNode = lastNode = curBlock = nullptr;
}
+ if (!mHTMLEditor->IsEditable(curNode)) {
+ continue;
+ }
// Recursion time
nsTArray<OwningNonNull<nsINode>> childArray;
GetChildNodesForOperation(*curNode, childArray);
nsresult rv = RemoveBlockStyle(childArray);
NS_ENSURE_SUCCESS(rv, rv);
} else if (IsInlineNode(curNode)) {
if (curBlock) {
// If so, is this node a descendant?
@@ -6804,21 +6803,22 @@ HTMLEditRules::RemoveBlockStyle(nsTArray
// handle it now. We need to remove the portion of curBlock that
// contains [firstNode - lastNode].
nsresult rv = RemovePartOfBlock(*curBlock, *firstNode, *lastNode);
NS_ENSURE_SUCCESS(rv, rv);
firstNode = lastNode = curBlock = nullptr;
// Fall out and handle curNode
}
curBlock = htmlEditor->GetBlockNodeParent(curNode);
- if (curBlock && HTMLEditUtils::IsFormatNode(curBlock)) {
- firstNode = lastNode = curNode->AsContent();
- } else {
+ if (!curBlock || !HTMLEditUtils::IsFormatNode(curBlock) ||
+ !mHTMLEditor->IsEditable(curBlock)) {
// Not a block kind that we care about.
curBlock = nullptr;
+ } else {
+ firstNode = lastNode = curNode->AsContent();
}
} else if (curBlock) {
// Some node that is already sans block style. Skip over it and process
// any partial progress saved.
nsresult rv = RemovePartOfBlock(*curBlock, *firstNode, *lastNode);
NS_ENSURE_SUCCESS(rv, rv);
firstNode = lastNode = curBlock = nullptr;
}
@@ -6841,32 +6841,26 @@ HTMLEditRules::ApplyBlockStyle(nsTArray<
nsIAtom& aBlockTag)
{
// Intent of this routine is to be used for converting to/from headers,
// paragraphs, pre, and address. Those blocks that pretty much just contain
// inline things...
NS_ENSURE_STATE(mHTMLEditor);
RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
- // Remove all non-editable nodes. Leave them be.
- for (int32_t i = aNodeArray.Length() - 1; i >= 0; i--) {
- if (!htmlEditor->IsEditable(aNodeArray[i])) {
- aNodeArray.RemoveElementAt(i);
- }
- }
-
nsCOMPtr<Element> newBlock;
nsCOMPtr<Element> curBlock;
for (auto& curNode : aNodeArray) {
nsCOMPtr<nsINode> curParent = curNode->GetParentNode();
int32_t offset = curParent ? curParent->IndexOf(curNode) : -1;
- // Is it already the right kind of block?
- if (curNode->IsHTMLElement(&aBlockTag)) {
+ // Is it already the right kind of block, or an uneditable block?
+ if (curNode->IsHTMLElement(&aBlockTag) ||
+ (!mHTMLEditor->IsEditable(curNode) && IsBlockNode(curNode))) {
// Forget any previous block used for previous inline nodes
curBlock = nullptr;
// Do nothing to this block
continue;
}
// If curNode is a address, p, header, address, or pre, replace it with a
// new block of correct type.
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -3164,17 +3164,17 @@ HTMLEditor::DeleteNode(nsINode* aNode)
return DeleteNode(node);
}
NS_IMETHODIMP
HTMLEditor::DeleteNode(nsIDOMNode* aNode)
{
// do nothing if the node is read-only
nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
- if (!IsModifiableNode(aNode) && !IsMozEditorBogusNode(content)) {
+ if (NS_WARN_IF(!IsModifiableNode(aNode) && !IsMozEditorBogusNode(content))) {
return NS_ERROR_FAILURE;
}
return EditorBase::DeleteNode(aNode);
}
nsresult
HTMLEditor::DeleteText(nsGenericDOMDataNode& aCharData,
--- a/editor/libeditor/tests/mochitest.ini
+++ b/editor/libeditor/tests/mochitest.ini
@@ -32,16 +32,17 @@ skip-if = toolkit == 'android'
[test_bug408231.html]
skip-if = toolkit == 'android'
[test_bug410986.html]
subsuite = clipboard
skip-if = toolkit == 'android'
[test_bug414526.html]
[test_bug417418.html]
skip-if = android_version == '18' # bug 1147989
+[test_bug430392.html]
[test_bug432225.html]
skip-if = toolkit == 'android'
[test_bug439808.html]
[test_bug442186.html]
[test_bug449243.html]
[test_bug455992.html]
[test_bug456244.html]
[test_bug460740.html]
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/tests/test_bug430392.html
@@ -0,0 +1,112 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=430392
+-->
+<head>
+ <title>Test for Bug 430392</title>
+ <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+ <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=430392">Mozilla Bug 430392</a>
+<p id="display"></p>
+<div id="content">
+ <div contenteditable="true" id="edit"> <span contenteditable="false">A</span> ; <span contenteditable="false">B</span> ; <span contenteditable="false">C</span> </div>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+/** Test for Bug 430392 **/
+
+function test() {
+ var edit = document.getElementById("edit");
+ var html = edit.innerHTML;
+ var expectedText = edit.textContent;
+ document.getElementById("edit").focus();
+
+ // Each test is [desc, callback]. callback() is called and we check that the
+ // textContent didn't change. For expected failures, the format is [desc,
+ // callback, expectedValue], and the test will be marked as an expected fail
+ // if the textContent changes to expectedValue, and an unexpected fail if
+ // it's neither the original value nor expectedValue.
+ var tests = [["adding returns", () => {
+ getSelection().collapse(edit.firstChild, 0);
+ synthesizeKey("VK_RIGHT", {});
+ synthesizeKey("VK_RIGHT", {});
+ synthesizeKey("VK_RETURN", {});
+ synthesizeKey("VK_RETURN", {});
+ synthesizeKey("VK_BACK_SPACE", {});
+ synthesizeKey("VK_BACK_SPACE", {});
+ }], ["adding shift-returns", () => {
+ getSelection().collapse(edit.firstChild, 0);
+ synthesizeKey("VK_RIGHT", {});
+ synthesizeKey("VK_RIGHT", {});
+ synthesizeKey("VK_RETURN", {shiftKey: true});
+ synthesizeKey("VK_RETURN", {shiftKey: true});
+ synthesizeKey("VK_BACK_SPACE", {});
+ synthesizeKey("VK_BACK_SPACE", {});
+ }, "A ; B ; C "]];
+ [
+ "insertorderedlist",
+ "insertunorderedlist",
+ ["formatblock", "p"],
+ ]
+ .forEach(item => {
+ var cmd = Array.isArray(item) ? item[0] : item;
+ var param = Array.isArray(item) ? item[1] : "";
+ tests.push([cmd, () => { document.execCommand(cmd, false, param) }]);
+ });
+ // These are all TODO -- they don't move the non-editable elements
+ [
+ "bold",
+ "italic",
+ "underline",
+ "strikethrough",
+ "subscript",
+ "superscript",
+ ["forecolor", "blue"],
+ ["backcolor", "blue"],
+ ["hilitecolor", "blue"],
+ ["fontname", "monospace"],
+ ["fontsize", "1"],
+ "justifyright",
+ "justifycenter",
+ "justifyfull"
+ ]
+ .forEach(item => {
+ var cmd = Array.isArray(item) ? item[0] : item;
+ var param = Array.isArray(item) ? item[1] : "";
+ tests.push([cmd, () => { document.execCommand(cmd, false, param) },
+ " A ; ; BC "]);
+ });
+ tests.push(["indent", () => { document.execCommand("indent") },
+ " ; ; ABC"]);
+ tests.forEach(arr => {
+ edit.innerHTML = html;
+ edit.focus();
+ getSelection().selectAllChildren(edit);
+ arr[1]();
+ if (2 < arr.length) {
+ todo_is(edit.textContent, expectedText,
+ arr[0] + " should not change text");
+ if (edit.textContent !== expectedText && edit.textContent !== arr[2]) {
+ is(edit.textContent, arr[2],
+ arr[0] + " changed to different failure");
+ }
+ } else {
+ is(edit.textContent, expectedText, arr[0] + " should not change text");
+ }
+ });
+
+ SimpleTest.finish();
+}
+
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(test);
+
+</script>
+</pre>
+</body>
+</html>