diff --git a/packages/ui/domrange.js b/packages/ui/domrange.js index 156332119d..f2d4cef524 100644 --- a/packages/ui/domrange.js +++ b/packages/ui/domrange.js @@ -109,7 +109,8 @@ _extend(DomRange.prototype, { this.members = {}; }, - add: function (id, newMemberOrArray, beforeId) { + // (_nextNode is internal) + add: function (id, newMemberOrArray, beforeId, _nextNode) { if (id && typeof id !== 'string') { beforeId = newMemberOrArray; newMemberOrArray = id; @@ -120,11 +121,23 @@ _extend(DomRange.prototype, { if (id != null) throw new Error("Can only add one node or one component if id is given"); var array = newMemberOrArray; + // calculate `nextNode` once in case it involves a refresh + _nextNode = this.getInsertionPoint(beforeId); for (var i = 0; i < array.length; i++) - this.add(array[i], beforeId); + this.add(null, array[i], beforeId, _nextNode); return; } + var parentNode = this.parentNode(); + // Consider ourselves removed (and don't mind) if + // start marker has no parent. + if (! parentNode) + return; + // because this may call `refresh`, it must be done + // early, before we add the new member. + var nextNode = (_nextNode || + this.getInsertionPoint(beforeId)); + var newMember = newMemberOrArray; if (id == null) id = this.nextMemberId++; @@ -136,13 +149,6 @@ _extend(DomRange.prototype, { throw new Error("Member already exists: " + id.slice(1)); members[id] = newMember; - var parentNode = this.parentNode(); - // Consider ourselves removed (and don't mind) if - // start marker has no parent. - if (! parentNode) - return; - var nextNode = this.getInsertionPoint(beforeId); - if ('dom' in newMember) { if (! newMember.dom) throw new Error("Component not built"); @@ -245,7 +251,6 @@ _extend(DomRange.prototype, { return this.start.parentNode; }, startNode: function () { - // does not refresh. return this.start; }, endNode: function () { @@ -274,6 +279,9 @@ _extend(DomRange.prototype, { } } }, + + ///////////// INTERNALS below this point, pretty much + // The purpose of "refreshing" a DomRange is to // take into account any element removals or moves // that may have occurred, and to "fix" the start @@ -285,6 +293,9 @@ _extend(DomRange.prototype, { // node, and this node is moved using jQuery, refreshing // the DomRange will look to the element as ground truth // and move the start/end markers around the element. + // A refreshed DomRange's nodes may surround nodes from + // sibling DomRanges (including their marker nodes) + // until the sibling DomRange is refreshed. // // Specifically, `refresh` moves the `start` // and `end` nodes to immediate before the first, @@ -300,58 +311,102 @@ _extend(DomRange.prototype, { // // Performing add/move/remove operations on an "each" // shouldn't require refreshing the entire each, just - // the member in question. + // the member in question. (However, adding to the + // end may require refreshing the whole "each"; + // see `getInsertionPoint`. Adding multiple members + // at once using `add(array)` is faster. refresh: function () { var color = nextColor++; this.color = color; - this.eachMember(null, function (range) { + var someNode = null; + var someRange = null; + var numMembers = 0; + // Assign a single unique "color" (an integer) to + // our members and us to recognize them easily. + this.eachMember(function (node) { + someNode = node; + numMembers++; + }, function (range) { range.refresh(); range.color = color; + someRange = range; + numMembers++; }); - // XXX optimize this so if significant - // members are consecutive (ignoring - // intervening insignificant nodes), - // it isn't O(childNodes.length). var parentNode = this.parentNode(); var firstNode = null; var lastNode = null; - var memberRangeIn = null; - for (var node = parentNode.firstChild; - node; node = node.nextSibling) { - if (memberRangeIn && node === memberRangeIn.end) { - memberRangeIn = null; - continue; - } - if ((memberRangeIn || ( - node.$ui.dom === this && - node !== this.start && - node !== this.end)) && - isSignificantNode(node)) { - if (firstNode) { - for (var n = firstNode.previousSibling; - n && ! n.$ui; - n = n.previousSibling) { - // adopt node - this.members[this.nextMemberId++] = n; - n.$ui = this.component; + if (numMembers === 0) { + // don't scan for members + } else if (numMembers === 1) { + if (someNode) { + firstNode = someNode; + lastNode = someNode; + } else if (someRange) { + firstNode = someRange.start; + lastNode = someRange.end; + } + } else { + // This loop is O(childNodes.length), even if our members + // are already consecutive. This means refreshing just one + // item in a list is technically order of the total number + // of siblings, including in other list items. + // + // The root cause is we intentionally don't track the + // DOM order of our members, so finding the first + // and last in sibling order either involves a scan + // or a bunch of calls to compareDocumentPosition. + // + // Fortunately, the common cases of zero and one members + // are optimized. Also, the scan is super-fast because + // no work is done for unknown nodes. It could be possible + // to optimize this code further if it becomes a problem. + for (var node = parentNode.firstChild; + node; node = node.nextSibling) { + + if (node.$ui && + ((node.$ui.dom === this && + node !== this.start && + node !== this.end && + isSignificantNode(node)) || + (node.$ui.dom !== this && + node.$ui.dom.color === color && + node.$ui.dom.start === node))) { + // found a member range or node + // (excluding "insignificant" empty text nodes, + // which won't be moved by, say, jQuery) + if (firstNode) { + // if we've already found a member in our + // scan, see if there are some easy ownerless + // nodes to "adopt" by scanning backwards. + for (var n = firstNode.previousSibling; + n && ! n.$ui; + n = n.previousSibling) { + this.members[this.nextMemberId++] = n; + n.$ui = this.component; + } + } + if (node.$ui.dom === this) { + // Node + firstNode = (firstNode || node); + lastNode = node; + } else { + // Range + // skip it and include its nodes in + // firstNode/lastNode. + firstNode = (firstNode || node); + node = node.$ui.dom.end; + lastNode = node; } } - firstNode = (firstNode || node); - lastNode = node; } - - if (! memberRangeIn && node.$ui && - node.$ui.dom !== this && - node.$ui.dom.color === color && - node.$ui.dom.start === node) - memberRangeIn = node.$ui.dom; } if (firstNode) { - // some significant node found. - // expand to include other nodes we recognize. + // some member or significant node was found. + // expand to include our insigificant member + // nodes as well. for (var n; (n = firstNode.previousSibling) && n.$ui && n.$ui.dom.color === color;) @@ -360,7 +415,7 @@ _extend(DomRange.prototype, { (n = lastNode.nextSibling) && n.$ui && n.$ui.dom.color === color;) lastNode = n; - + // adjust our start/end pointers if (firstNode !== this.start) insertNode(this.start, parentNode, firstNode); @@ -373,8 +428,19 @@ _extend(DomRange.prototype, { var members = this.members; var parentNode = this.parentNode(); - if (! beforeId) + if (! beforeId) { + // Refreshing here is necessary if we want to + // allow elements to move around arbitrarily. + // If jQuery is used to reorder elements, it could + // easily make our `end` pointer meaningless, + // even though all our members continue to make + // good reference points as long as they are refreshed. + // + // However, a refresh is expensive! Let's + // make the developer manually refresh if + // elements are being re-ordered externally. return this.end; + } beforeId = '_' + beforeId; var mem = members[beforeId]; @@ -396,6 +462,7 @@ _extend(DomRange.prototype, { // not there anymore delete members[beforeId]; + // no good position return this.end; } }); diff --git a/packages/ui/domrange_tests.js b/packages/ui/domrange_tests.js index b0f0fd08be..dfcadba032 100644 --- a/packages/ui/domrange_tests.js +++ b/packages/ui/domrange_tests.js @@ -228,4 +228,217 @@ Tinytest.add("ui - DomRange - nested", function (test) { test.equal(spellDom(), '(AaCcBFfDdb)'); r.remove('B'); test.equal(spellDom(), '(AaCc)'); +}); + +Tinytest.add("ui - DomRange - outside moves", function (test) { + // In this one, uppercase letters are div elements, + // lowercase letters are marker text nodes, as follows: + // + // a-X-b - c-d-Y-Z-e-f - g-h-i-W-j-k-l V + // + // In other words, one DomRange containing an element (X), + // then two nested DomRanges containing two elements (Y,Z), + // etc. + + var wsp = function () { + return document.createTextNode(' '); + }; + + var X = document.createElement("DIV"); + X.id = 'X'; + var Y = document.createElement("DIV"); + Y.id = 'Y'; + var Z = document.createElement("DIV"); + Z.id = 'Z'; + var W = document.createElement("DIV"); + W.id = 'W'; + var V = document.createElement("DIV"); + V.id = 'V'; + + var ab = new Comp('ab'); + ab.dom.add(wsp()); + ab.dom.add('X', X); + ab.dom.add(wsp()); + var cf = new Comp('cf'); + var de = new Comp('de'); + de.dom.add(wsp()); + de.dom.add('Y', Y); + de.dom.add(wsp()); + de.dom.add('Z', Z); + de.dom.add(wsp()); + cf.dom.add(wsp()); + cf.dom.add('de', de); + cf.dom.add(wsp()); + var gl = new Comp('gl'); + var hk = new Comp('hk'); + var ij = new Comp('ij'); + ij.dom.add(wsp()); + ij.dom.add('W', W); + ij.dom.add(wsp()); + // i-W-j + test.equal(ij.dom.getNodes().length, 5); + gl.dom.add(wsp()); + gl.dom.add('hk', hk); + gl.dom.add(wsp()); + // g-hk-l + test.equal(gl.dom.getNodes().length, 6); + hk.dom.add(wsp()); + hk.dom.add('ij', ij); + hk.dom.add(wsp()); + // h-i-W-j-k + test.equal(hk.dom.getNodes().length, 9); + // g-h-i-W-j-k-l + test.equal(gl.dom.getNodes().length, 13); + + var r = new DomRange; + r.dom.add('ab', ab); + r.dom.add(wsp()); + r.dom.add('cf', cf); + r.dom.add(wsp()); + r.dom.add('gl', gl); + r.dom.add('V', V); + + var spellDom = function () { + var frag = r.parentNode(); + var str = ''; + _.each(frag.childNodes, function (n) { + if (n.nodeType === 3 && n.$ui) { + var ui = n.$ui; + if (ui.dom.start === n) + str += (ui.which ? ui.which.charAt(0) : '('); + else if (n.$ui && n.$ui.dom.end === n) + str += (ui.which ? ui.which.charAt(1) : ')'); + else + str += '-'; + } else { + str += (n.id || '?'); + } + }); + return str; + }; + var strip = function (str) { + return str.replace(/[^-\w()]+/g, ''); + }; + + test.equal(spellDom(), + strip('(a-X-b - c-d-Y-Z-e-f - g-h-i-W-j-k-l V)')); + + // all right, now let's mess around with these elements! + + $([Y,Z]).insertBefore(X); + + // jQuery lifted Y,Z right out and stuck them before X + test.equal(spellDom(), + strip('(a-YZX-b - c-d---e-f - g-h-i-W-j-k-l V)')); + + r.moveBefore('cf', 'ab'); + + // the move causes a refresh of `ab` and `cf` and their + // descendent members, re-establishing proper organization + // (ignoring whitespace textnodes) + test.equal(spellDom(), + strip('(- cdYZef aX-b ------- g-h-i-W-j-k-l V)')); + + $(W).insertBefore(X); + + test.equal(spellDom(), + strip('(- cdYZef aWX-b ------- g-h-i--j-k-l V)')); + + $(Z).insertBefore(W); + + test.equal(spellDom(), + strip('(- cdYef aZWX-b ------- g-h-i--j-k-l V)')); + + r.moveBefore('ab', 'cf'); + + // WOW! `ab` and `cf` have been fixed. Here's what + // happened: + // - Because `cf` is serving as an insertion point, it + // is refreshed first, and it recursively refreshes + // `de`. This causes `e` and then `f` to move to the + // right of `Z`. There's still `a` floating in the middle. + // - Then `ab` is refreshed. This moves `a` to right before + // `X`. + // - Finally, `aX-b` is moved before `c`. + test.equal(spellDom(), + strip('(- aX-b cdYZef W ------- g-h-i--j-k-l V)')); + + r.moveBefore('ab', 'gl'); + + // Because `gl` is being used as a reference point, + // it is refreshed to contain `W`. + // Because the `-` that was initial came from `ab`, + // it is recaptured. + test.equal(spellDom(), + strip('(cdYZef a-X-b ghiWjkl ------------- V)')); + + $(Z).insertBefore(X); + + test.equal(spellDom(), + strip('(cdYef a-ZX-b ghiWjkl ------------- V)')); + + r.moveBefore('gl', 'cf'); + + // Note that the `a` is still misplaced here. + test.equal(spellDom(), + strip('(ghiWjkl cdY a-ZefX-b ------------- V)')); + + r.moveBefore('cf', 'V'); + + test.equal(spellDom(), + strip('(ghiWjkl X-b ------------- cdY a-Zef V)')); + + + $(X).insertBefore(Y); + + // holy crap, now `aXb` is a mess. Really `a` and `b` + // are in the completely wrong place. + test.equal(spellDom(), + strip('(ghiWjkl -b ------------- cdXY a-Zef V)')); + + r.moveBefore('gl', 'ab'); + + // Now `c` and `d` are wrong. It looks like `cdYZef` + // also includes `W` and `X`. + test.equal(spellDom(), + strip('(-------------- cd ghiWjkl aXbY-Zef V)')); + + // However, remove `cf` will do a refresh first. + r.remove('cf'); + + test.equal(spellDom(), + strip('(-------------- ghiWjkl aXb V)')); + + $(X).insertBefore(W); + r.parentNode().appendChild(W); + + test.equal(spellDom(), + strip('(-------------- ghiXjkl ab V) W')); + + r.moveBefore('ab', 'gl'); + + + test.equal(spellDom(), + strip('(-------------- V) aXb ghiWjkl')); + + r.remove('V'); + + test.equal(spellDom(), + strip('(--------------) aXb ghiWjkl')); + + + // Manual refresh is required for move-to-end + // (or add-at-end) if elements may have moved externally, + // because the `end` pointer could be totally wrong. + // Otherwise, the order of `ab` and `gl` would swap, + // meaning the DomRange operations would do something + // different from the jQuery operations. + // + // See `range.getInsertionPoint`. + + r.refresh(); + r.moveBefore('gl', null); + + test.equal(spellDom(), + strip('------------- (- aXb ghiWjkl)')); }); \ No newline at end of file