Merge pull request #877 from kossebau/fixOpMergeParagraphAndEmptyAnnotations

Friedrich · Friedrich · commit ca7d2e68ce0e · 2015-03-25T11:18:10.000+01:00
Fix OpMergeParagraph merging paragraphs with empty annotations
diff --git a/ChangeLog.md b/ChangeLog.md
@@ -1,3 +1,12 @@
+# Changes between 0.5.6 and 0.5.7
+
+## WebODF
+
+### Fixes
+
+* Fix breaking all empty annotations on merging the paragraph they are contained in with the one before ([#877](https://github.com/kogmbh/WebODF/pull/877)))
+
+
 # Changes between 0.5.5 and 0.5.6
 
 ## WebODF
diff --git a/webodf/lib/core/DomUtils.js b/webodf/lib/core/DomUtils.js
@@ -579,20 +579,29 @@
 
         /**
          * Removes all unwanted nodes from targetNode includes itself.
+         * The nodeFilter defines which nodes should be removed (NodeFilter.FILTER_REJECT),
+         * should be skipped including the subtree (NodeFilter.FILTER_SKIP) or should be kept
+         * and their subtree checked further (NodeFilter.FILTER_ACCEPT).
          * @param {!Node} targetNode
-         * @param {function(!Node):!boolean} shouldRemove check whether a node should be removed or not
+         * @param {!function(!Node) : !number} nodeFilter
          * @return {?Node} parent of targetNode
          */
-        function removeUnwantedNodes(targetNode, shouldRemove) {
+        function removeUnwantedNodes(targetNode, nodeFilter) {
             var parent = targetNode.parentNode,
                 node = targetNode.firstChild,
+                filterResult = nodeFilter(targetNode),
                 next;
+
+            if (filterResult === NodeFilter.FILTER_SKIP) {
+                return parent;
+            }
+
             while (node) {
                 next = node.nextSibling;
-                removeUnwantedNodes(node, shouldRemove);
+                removeUnwantedNodes(node, nodeFilter);
                 node = next;
             }
-            if (parent && shouldRemove(targetNode)) {
+            if (parent && (filterResult === NodeFilter.FILTER_REJECT)) {
                 mergeIntoParent(targetNode);
             }
             return parent;
diff --git a/webodf/lib/odf/CollapsingRules.js b/webodf/lib/odf/CollapsingRules.js
@@ -22,7 +22,7 @@
  * @source: https://github.com/kogmbh/WebODF/
  */
 
-/*global odf, core, Node*/
+/*global odf, core, Node, NodeFilter*/
 
 /**
  * Defines a set of rules for how elements can be collapsed based on whether they contain ODT content (e.g.,
@@ -36,14 +36,15 @@ odf.CollapsingRules = function CollapsingRules(rootNode) {
         domUtils = core.DomUtils;
 
     /**
-     * Returns true if a given node is odf node or a text node that has a odf parent.
+     * Returns NodeFilter value if a given node is odf node or a text node that has a odf parent.
      * @param {!Node} node
-     * @return {!boolean}
+     * @return {!number}
      */
-    function shouldRemove(node) {
-        return odfUtils.isODFNode(node)
+    function filterOdfNodesToRemove(node) {
+        var isToRemove = odfUtils.isODFNode(node)
             || (node.localName === "br" && odfUtils.isLineBreak(node.parentNode))
             || (node.nodeType === Node.TEXT_NODE && odfUtils.isODFNode(/** @type {!Node}*/(node.parentNode)));
+        return isToRemove ? NodeFilter.FILTER_REJECT : NodeFilter.FILTER_ACCEPT;
     }
 
     /**
@@ -69,7 +70,7 @@ odf.CollapsingRules = function CollapsingRules(rootNode) {
             parent.removeChild(targetNode);
         } else {
             // removes all odf nodes
-            parent = domUtils.removeUnwantedNodes(targetNode, shouldRemove);
+            parent = domUtils.removeUnwantedNodes(targetNode, filterOdfNodesToRemove);
         }
         if (parent && isCollapsibleContainer(parent)) {
             return mergeChildrenIntoParent(parent);
diff --git a/webodf/lib/ops/OpMergeParagraph.js b/webodf/lib/ops/OpMergeParagraph.js
@@ -22,7 +22,7 @@
  * @source: https://github.com/kogmbh/WebODF/
  */
 
-/*global ops, runtime, odf, core, Node*/
+/*global ops, runtime, odf, core, Node, NodeFilter*/
 
 /**
  * Merges two adjacent paragraphs together into the first paragraph. The destination paragraph
@@ -68,10 +68,13 @@ ops.OpMergeParagraph = function OpMergeParagraph() {
     /**
      * Returns true if the supplied node is an ODF grouping element with no content
      * @param {!Node} element
-     * @return {!boolean}
+     * @return {!number}
      */
-    function isEmptyGroupingElement(element) {
-        return odfUtils.isGroupingElement(element) && odfUtils.hasNoODFContent(element);
+    function filterEmptyGroupingElementToRemove(element) {
+        if (odf.OdfUtils.isInlineRoot(element)) {
+            return NodeFilter.FILTER_SKIP;
+        }
+        return odfUtils.isGroupingElement(element) && odfUtils.hasNoODFContent(element) ? NodeFilter.FILTER_REJECT : NodeFilter.FILTER_ACCEPT;
     }
 
     /**
@@ -93,7 +96,7 @@ ops.OpMergeParagraph = function OpMergeParagraph() {
                 // Empty spans need to be cleaned up on merge, as remove text only removes things that contain text content
                 // Child is moved across before collapsing so any foreign sub-elements are collapsed up the chain next to
                 // the destination location
-                domUtils.removeUnwantedNodes(child, isEmptyGroupingElement);
+                domUtils.removeUnwantedNodes(child, filterEmptyGroupingElementToRemove);
             }
             child = source.firstChild;
         }
diff --git a/webodf/tests/core/DomUtilsTests.js b/webodf/tests/core/DomUtilsTests.js
@@ -515,7 +515,7 @@ core.DomUtilsTests = function DomUtilsTests(runner) {
         p.appendChild(span3);
         t.doc.appendChild(p);
         t.parent = t.utils.removeUnwantedNodes(p, function (node) {
-            return node !== null;
+            return (node !== null) ? NodeFilter.FILTER_REJECT : NodeFilter.FILTER_ACCEPT;
         });
         r.shouldBe(t, "t.parent", "t.doc");
         r.shouldBe(t, "t.parent.childNodes.length", "0");
@@ -536,7 +536,7 @@ core.DomUtilsTests = function DomUtilsTests(runner) {
         p.appendChild(span3);
         t.doc.appendChild(p);
         t.parent = t.utils.removeUnwantedNodes(p, function (node) {
-            return node.localName === 'span';
+            return node.localName === 'span' ? NodeFilter.FILTER_REJECT : NodeFilter.FILTER_ACCEPT;
         });
         r.shouldBe(t, "t.parent", "t.doc");
         r.shouldBe(t, "t.parent.firstChild.localName", "'p'");
@@ -546,6 +546,32 @@ core.DomUtilsTests = function DomUtilsTests(runner) {
         r.shouldBe(t, "t.parent.firstChild.childNodes[2].firstChild.textContent", "'test'");
     }
 
+    function removeUnwantedNodes_SkipDiv() {
+        var p = document.createElement("p"),
+            span1 = document.createElement("span"),
+            div = document.createElement("div"),
+            divspan = document.createElement("span"),
+            span3 = document.createElement("span"),
+            b = document.createElement("b");
+        b.textContent = "test";
+        span1.textContent = "hello";
+        divspan.textContent = "world";
+        span3.appendChild(b);
+        p.appendChild(span1);
+        p.appendChild(div);
+        div.appendChild(divspan);
+        p.appendChild(span3);
+        t.doc.appendChild(p);
+        t.parent = t.utils.removeUnwantedNodes(p, function (node) {
+            return (node.localName === "div") ? NodeFilter.FILTER_SKIP : NodeFilter.FILTER_REJECT;
+        });
+        r.shouldBe(t, "t.parent", "t.doc");
+        r.shouldBe(t, "t.parent.childNodes.length", "1");
+        r.shouldBe(t, "t.parent.firstChild.localName", "'div'");
+        r.shouldBe(t, "t.parent.firstChild.childNodes.length", "1");
+        r.shouldBe(t, "t.parent.firstChild.firstChild.localName", "'span'");
+    }
+
     function removeAllChildNodes_None() {
         var p = document.createElement("p");
         t.doc.appendChild(p);
@@ -818,6 +844,7 @@ core.DomUtilsTests = function DomUtilsTests(runner) {
 
             removeUnwantedNodes_DiscardAll,
             removeUnwantedNodes_DiscardSpanOnly,
+            removeUnwantedNodes_SkipDiv,
 
             removeAllChildNodes_None,
             removeAllChildNodes_ElementAndTextNodes,
diff --git a/webodf/tests/ops/operationtests.xml b/webodf/tests/ops/operationtests.xml
@@ -528,6 +528,29 @@
   </ops>
   <after><office:text><text:p text:style-name="A">abcdefgh</text:p></office:text></after>
  </test>
+ <test name="MergeParagraphWithEmptyAnnotationText inside">
+  <before><office:text><text:p text:style-name="A">ab</text:p><text:p text:style-name="B">c<office:annotation office:name="alice_1">
+    <dc:creator e:memberid="Alice">Alice</dc:creator>
+    <dc:date>2013-08-05T12:34:07.061Z</dc:date>
+    <text:list>
+     <text:list-item>
+      <text:p></text:p>
+     </text:list-item>
+    </text:list>
+   </office:annotation>d</text:p></office:text></before>
+  <ops>
+   <op optype="MergeParagraph" destinationStartPosition="0" sourceStartPosition="3" paragraphStyleName="A"/>
+  </ops>
+  <after><office:text><text:p text:style-name="A">abc<office:annotation office:name="alice_1">
+    <dc:creator e:memberid="Alice">Alice</dc:creator>
+    <dc:date>2013-08-05T12:34:07.061Z</dc:date>
+    <text:list>
+     <text:list-item>
+      <text:p></text:p>
+     </text:list-item>
+    </text:list>
+   </office:annotation>d</text:p></office:text></after>
+ </test>
  <test name="RemoveAndMerge">
   <before><office:text><text:p text:style-name="A">abc<text:span text:style-name="B">d</text:span></text:p><text:p text:style-name="C"><text:span text:style-name="D">efgh</text:span></text:p></office:text></before>
   <ops>
