Tweak postEditor#addMarkupToRange to consider existing markups

Fixes #360

We insert the new markup at a consistent index across the range.
If we just push on the end of the list, it can end up in different positions
of the markup stack. This results in unnecessary closing and re-opening of
the markup each time it changes position.
If we just push it at the beginning of the list, this causes unnecessary closing
and re-opening of surrounding tags.
So, we look for any tags open across the whole range, and push into the stack
at the end of those.

src/js/editor/post.js

@@ -1,5 +1,5 @@
 import Position from '../utils/cursor/position';
-import { forEach, filter, values } from '../utils/array-utils';
+import { forEach, reduce, filter, values, commonItems } from '../utils/array-utils';
 import { DIRECTION } from '../utils/key';
 import LifecycleCallbacks from '../models/lifecycle-callbacks';
 import assert from '../utils/assert';
@@ -847,10 +847,29 @@ class PostEditor {
     if (range.isCollapsed) {
       return;
     }
-    this.splitMarkers(range).forEach(marker => {
-      marker.addMarkup(markup);
-      this._markDirty(marker);
-    });
+
+    let markers = this.splitMarkers(range);
+    if (markers.length) {
+      // We insert the new markup at a consistent index across the range.
+      // If we just push on the end of the list, it can end up in different positions
+      // of the markup stack. This results in unnecessary closing and re-opening of
+      // the markup each time it changes position.
+      // If we just push it at the beginning of the list, this causes unnecessary closing
+      // and re-opening of surrounding tags.
+      // So, we look for any tags open across the whole range, and push into the stack
+      // at the end of those.
+      // Prompted by https://github.com/bustlelabs/mobiledoc-kit/issues/360
+
+      let markupsOpenAcrossRange = reduce(markers, function (soFar, marker) {
+        return commonItems(soFar, marker.markups);
+      }, markers[0].markups);
+      let indexToInsert = markupsOpenAcrossRange.length;
+
+      markers.forEach(marker => {
+        marker.addMarkupAtIndex(markup, indexToInsert);
+        this._markDirty(marker);
+      });
+    }
   }
 
   /**

src/js/utils/array-utils.js

@@ -75,6 +75,21 @@ function commonItemLength(listA, listB) {
   return offset;
 }
 
+/**
+ * @return {Array} the items that are the same, starting from the 0th index, in a and b
+ * @private
+ */
+function commonItems(listA, listB) {
+  let offset = 0;
+  while (offset < listA.length && offset < listB.length) {
+    if (listA[offset] !== listB[offset]) {
+      break;
+    }
+    offset++;
+  }
+  return listA.slice(0, offset);
+}
+
 // return new array without falsy items like ruby's `compact`
 function compact(enumerable) {
   return filter(enumerable, i => !!i);
@@ -148,6 +163,7 @@ export {
   every,
   filter,
   commonItemLength,
+  commonItems,
   compact,
   reduce,
   objectToSortedKVArray,

src/js/utils/markuperable.js

@@ -11,6 +11,10 @@ export default class Markerupable {
     this.markups.push(markup);
   }
 
+  addMarkupAtIndex(markup, index) {
+    this.markups.splice(index, 0, markup);
+  }
+
   removeMarkup(markupOrMarkupCallback) {
     let callback;
     if (typeof markupOrMarkupCallback === 'function') {

tests/unit/editor/post-test.js

@@ -12,7 +12,6 @@ import { clearSelection } from 'mobiledoc-kit/utils/selection-utils';
 const { FORWARD } = DIRECTION;
 
 const { module, test } = Helpers;
-const { skip } = QUnit;
 
 let editor, editorElement;
 
@@ -938,7 +937,7 @@ test('#addMarkupToRange silently does nothing when invoked with an empty range',
   assert.ok(!section.markers.head.hasMarkup(markup), 'marker has no markup');
 });
 
-skip("#addMarkupToRange around a markup pushes the new markup below existing ones", (assert) => {
+test("#addMarkupToRange around a markup pushes the new markup below existing ones", (assert) => {
   let em;
   const editor = buildEditorWithMobiledoc(({post, markupSection, marker, markup}) => {
     em = markup('em');
@@ -993,7 +992,7 @@ test("#addMarkupToRange within a markup puts the new markup on top of the stack"
       '<p><em>one <b>BOLD</b> two</em></p>');
 });
 
-skip("#addMarkupToRange straddling the open tag of an existing markup, closes and reopens the existing markup", (assert) => {
+test("#addMarkupToRange straddling the open tag of an existing markup, closes and reopens the existing markup", (assert) => {
   let em;
   const editor = buildEditorWithMobiledoc(({post, markupSection, marker, markup}) => {
     em = markup('em');
@@ -1016,7 +1015,7 @@ skip("#addMarkupToRange straddling the open tag of an existing markup, closes an
       '<p><em>_one <b>TWO_</b></em><b> THREE</b></p>');
 });
 
-skip("#addMarkupToRange straddling the closing tag of an existing markup, closes and reopens the existing markup", (assert) => {
+test("#addMarkupToRange straddling the closing tag of an existing markup, closes and reopens the existing markup", (assert) => {
   let em;
   const editor = buildEditorWithMobiledoc(({post, markupSection, marker, markup}) => {
     em = markup('em');