Merge pull request #195 from guardian/re-list-bug-fix

Re list bug fix
guardian · Jun 17, 2014 · 1fb48f9 · 1fb48f9
2 parents dc08bef + a8195fa
commit 1fb48f9
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 33 deletions.
diff --git a/bower.json b/bower.json
@@ -1,7 +1,8 @@
 {
   "name": "scribe",
   "dependencies": {
-    "lodash-amd": "2.4.1"
+    "lodash-amd": "2.4.1",
+    "scribe-common": "0.0.4"
   },
   "devDependencies": {
     "requirejs": "~2.1.9",

diff --git a/examples/amd.html b/examples/amd.html
@@ -14,15 +14,15 @@
     border-style: inset;
   }
 
-  rte, rte-toolbar {
+  .rte, .rte-toolbar {
     display: block;
   }
 
   p {
     margin-top: 0;
   }
 
-  rte {
+  .rte {
     border: 1px solid gray;
     height: 300px;
     overflow: auto;
@@ -52,11 +52,11 @@
   scribePluginSanitizer,
   scribePluginInlineStyles
 ) {
-  var scribe = new Scribe(document.querySelector('rte'));
+  var scribe = new Scribe(document.querySelector('.rte'));
 
   scribe.setContent('<p>Hello, World!<\/p>');
 
-  scribe.use(scribePluginToolbar(document.querySelector('rte-toolbar')));
+  scribe.use(scribePluginToolbar(document.querySelector('.rte-toolbar')));
   scribe.use(scribePluginInlineStyles());
   scribe.use(scribePluginSanitizer({ tags: {
     p: {},
@@ -74,10 +74,10 @@
   updateHtml();
 });
 </script>
-<rte-toolbar>
+<div class="rte-toolbar">
   <button data-command-name="bold">Bold</button>
-</rte-toolbar>
-<rte></rte>
+</div>
+<div class="rte"></div>
 <section>
   <h1>Output</h1>
   <textarea class="rte-output" readonly></textarea>

diff --git a/package.json b/package.json
@@ -22,6 +22,7 @@
     "q": "~1.0.0",
     "request": "~2.33.0",
     "selenium-webdriver": "~2.41.0",
+    "scribe-common": "0.0.4",
     "scribe-test-harness": "~0.0.2"
   },
   "scripts": {

diff --git a/src/dom-observer.js b/src/dom-observer.js
@@ -1,20 +1,16 @@
 define([
   'lodash-amd/modern/arrays/flatten',
-  'lodash-amd/modern/collections/toArray'
+  'lodash-amd/modern/collections/toArray',
+  'scribe-common/element',
+  'scribe-common/node'
 ], function (
   flatten,
-  toArray
+  toArray,
+  elementHelpers,
+  nodeHelpers
 ) {
 
   function observeDomChanges(el, callback) {
-    function notEmptyTextNode(node) {
-      return ! (node.nodeType === Node.TEXT_NODE && node.textContent === '');
-    }
-
-    function notSelectionMarkerNode(node) {
-      return ! (node.nodeType === Node.ELEMENT_NODE && node.className === 'scribe-marker');
-    }
-
     function includeRealMutations(mutations) {
       var allChangedNodes = flatten(mutations.map(function(mutation) {
         var added   = toArray(mutation.addedNodes);
@@ -23,13 +19,12 @@ define([
       }));
 
       var realChangedNodes = allChangedNodes.
-        filter(notEmptyTextNode).
-        filter(notSelectionMarkerNode);
+        filter(function(n) { return ! nodeHelpers.isEmptyTextNode(n); }).
+        filter(function(n) { return ! elementHelpers.isSelectionMarkerNode(n); });
 
       return realChangedNodes.length > 0;
     }
 
-
     // Flag to avoid running recursively
     var runningPostMutation = false;
     var observer = new MutationObserver(function(mutations) {

diff --git a/src/plugins/core/patches/commands/insert-list.js b/src/plugins/core/patches/commands/insert-list.js
@@ -1,4 +1,5 @@
-define(['../../../../api/element'], function (element) {
+define(['../../../../api/element',
+        'scribe-common/node'], function (element, nodeHelpers) {
 
   'use strict';
 
@@ -16,34 +17,58 @@ define(['../../../../api/element'], function (element) {
           scribe.api.CommandPatch.prototype.execute.call(this, value);
 
           if (this.queryState()) {
-            /**
-             * Chrome: If we apply the insertOrderedList command on an empty block, the
-             * OL/UL will be nested inside the block.
-             * As per: http://jsbin.com/oDOriyU/1/edit?html,js,output
-             */
-
             var selection = new scribe.api.Selection();
 
             var listElement = selection.getContaining(function (node) {
               return node.nodeName === 'OL' || node.nodeName === 'UL';
             });
 
+
+            /**
+             * Firefox: If we apply the insertOrderedList or the insertUnorderedList
+             * command on an empty block, a P will be inserted after the OL/UL.
+             * As per: http://jsbin.com/cubacoli/3/edit?html,js,output
+             */
+
+            if (listElement.nextElementSibling &&
+                listElement.nextElementSibling.childNodes.length === 0) {
+              nodeHelpers.removeNode(listElement.nextElementSibling);
+            }
+
+            /**
+             * Chrome: If we apply the insertOrderedList or the insertUnorderedList
+             * command on an empty block, the OL/UL will be nested inside the block.
+             * As per: http://jsbin.com/eFiRedUc/1/edit?html,js,output
+             */
+
             if (listElement) {
               var listParentNode = listElement.parentNode;
-
               // If list is within a text block then split that block
               if (listParentNode && /^(H[1-6]|P)$/.test(listParentNode.nodeName)) {
                 selection.placeMarkers();
-                listParentNode.parentNode.insertBefore(listElement, listParentNode.nextElementSibling);
+                // Move listElement out of the block
+                nodeHelpers.insertAfter(listElement, listParentNode);
                 selection.selectMarkers();
-                listParentNode.parentNode.removeChild(listParentNode);
+
+                /**
+                 * Chrome 27-34: An empty text node is inserted.
+                 */
+                if (listParentNode.childNodes.length === 2 &&
+                    nodeHelpers.isEmptyTextNode(listParentNode.firstChild)) {
+                  nodeHelpers.removeNode(listParentNode);
+                }
+
+                // Remove the block if it's empty
+                if (listParentNode.childNodes.length === 0) {
+                  nodeHelpers.removeNode(listParentNode);
+                }
               }
             }
 
             /**
              * Chrome: If a parent node has a CSS `line-height` when we apply the
-             * insert(Un)OrderedList command, Chrome appends a SPAN to LIs with
-             * inline styling replicating that `line-height`.
+             * insertOrderedList or the insertUnorderedList command, Chrome appends
+             * a SPAN to LIs with inline styling replicating that `line-height`.
              * As per: http://jsbin.com/OtemujAY/7/edit?html,css,js,output
              *
              * FIXME: what if the user actually wants to use SPANs? This could

diff --git a/test/commands.spec.js b/test/commands.spec.js
@@ -102,6 +102,20 @@ describe('commands', function () {
       });
     });
 
+    givenContentOf('<p>1<br>2|</p>', function () {
+      when('the command is executed', function () {
+        beforeEach(function () {
+          return executeCommand('insertOrderedList');
+        });
+
+        it('should wrap the contents of the last line in an ordered list', function () {
+          return scribeNode.getInnerHTML().then(function (innerHTML) {
+            expect(innerHTML).to.have.html('<p>1<br></p><ol><li>2<chrome-bogus-br></li></ol>');
+          });
+        });
+      });
+    });
+
     /**
      * Unapplying
      */