Fix issues with ordering of tagged items (#3301)

* Added better handling for sortByList manual placements

If manual placement specifications show up in an inconvenient order,
sortByList, will go to the trouble of processing them in that order.

* Added tests to confirm solution to (#3296)

...That custom tag ordering will not choke when tiddlers get sorted after their dependencies have been placed around them

* Corrected list-after bug when referencing external titles

* Using more error-proof $tw.utils.hop in sortByList

* minor indentation correction in test-tags.js

core/modules/wiki.js

@@ -558,39 +558,49 @@ exports.sortByList = function(array,listTitle) {
 			}
 		}
 		// Finally obey the list-before and list-after fields of each tiddler in turn
-		var sortedTitles = titles.slice(0);
-		for(t=0; t<sortedTitles.length; t++) {
-			title = sortedTitles[t];
-			var currPos = titles.indexOf(title),
-				newPos = -1,
-				tiddler = this.getTiddler(title);
-			if(tiddler) {
-				var beforeTitle = tiddler.fields["list-before"],
-					afterTitle = tiddler.fields["list-after"];
-				if(beforeTitle === "") {
-					newPos = 0;
-				} else if(afterTitle === "") {
-					newPos = titles.length;
-				} else if(beforeTitle) {
-					newPos = titles.indexOf(beforeTitle);
-				} else if(afterTitle) {
-					newPos = titles.indexOf(afterTitle);
-					if(newPos >= 0) {
-						++newPos;
+		var sortedTitles = titles.slice(0),
+			replacedTitles = Object.create(null),
+			self = this;
+		function replaceItem(title) {
+			if(!$tw.utils.hop(replacedTitles, title)) {
+				replacedTitles[title] = true;
+				var newPos = -1,
+					tiddler = self.getTiddler(title);
+				if(tiddler) {
+					var beforeTitle = tiddler.fields["list-before"],
+						afterTitle = tiddler.fields["list-after"];
+					if(beforeTitle === "") {
+						newPos = 0;
+					} else if(afterTitle === "") {
+						newPos = titles.length;
+					} else if(beforeTitle) {
+						replaceItem(beforeTitle);
+						newPos = titles.indexOf(beforeTitle);
+					} else if(afterTitle) {
+						replaceItem(afterTitle);
+						newPos = titles.indexOf(afterTitle);
+						if(newPos >= 0) {
+							++newPos;
+						}
 					}
-				}
-				if(newPos === -1) {
-					newPos = currPos;
-				}
-				if(newPos !== currPos) {
-					titles.splice(currPos,1);
-					if(newPos >= currPos) {
-						newPos--;
+					// We get the currPos //after// figuring out the newPos, because recursive replaceItem calls might alter title's currPos
+					var currPos = titles.indexOf(title);
+					if(newPos === -1) {
+						newPos = currPos;
+					}
+					if(currPos >= 0 && newPos !== currPos) {
+						titles.splice(currPos,1);
+						if(newPos >= currPos) {
+							newPos--;
+						}
+						titles.splice(newPos,0,title);
 					}
-					titles.splice(newPos,0,title);
 				}
 			}
-
+		};
+		for(t=0; t<sortedTitles.length; t++) {
+			title = sortedTitles[t];
+			replaceItem(title);
 		}
 		return titles;
 	}

editions/test/tiddlers/tests/test-tags.js

@@ -86,6 +86,59 @@ describe("Tag tests", function() {
 		expect(wiki.filterTiddlers("[tag[TiddlerSeventh]]").join(",")).toBe("Tiddler10,TiddlerOne,Tiddler Three,Tiddler11,Tiddler9,a fourth tiddler");
 	});
 
+	// Tests for issue (#3296)
+	it("should apply tag ordering in order of dependency", function () {
+		var wiki = new $tw.Wiki();
+
+		wiki.addTiddler({ title: "A", text: "", tags: "sortTag", "list-after": "B"});
+		wiki.addTiddler({ title: "B", text: "", tags: "sortTag", "list-after": "C"});
+		wiki.addTiddler({ title: "C", text: "", tags: "sortTag"});
+
+		expect(wiki.filterTiddlers("[tag[sortTag]]").join(',')).toBe("C,B,A");
+	});
+
+	it("should handle self-referencing dependency without looping infinitely", function() {
+		var wiki = new $tw.Wiki();
+
+		wiki.addTiddler({ title: "A", text: "", tags: "sortTag"});
+		wiki.addTiddler({ title: "B", text: "", tags: "sortTag", "list-after": "B"});
+		wiki.addTiddler({ title: "C", text: "", tags: "sortTag"});
+
+		expect(wiki.filterTiddlers("[tag[sortTag]]").join(',')).toBe("A,B,C");
+	});
+
+	it("should handle empty list-after ordering", function() {
+		var wiki = new $tw.Wiki();
+
+		wiki.addTiddler({ title: "A", text: "", tags: "sortTag", "list-after": ""});
+		wiki.addTiddler({ title: "B", text: "", tags: "sortTag"});
+		wiki.addTiddler({ title: "C", text: "", tags: "sortTag"});
+
+		expect(wiki.filterTiddlers("[tag[sortTag]]").join(',')).toBe("B,C,A");
+	});
+
+	// If a tiddler in the tag references a tiddler OUTSIDE of the tag
+	// with list-after/before, we need to make sure we don't accidentally
+	// handle that external tiddler, or that reference.
+	it("should gracefully handle dependencies that aren't in the tag list", function() {
+		var wiki = new $tw.Wiki();
+
+		wiki.addTiddler({ title: "A", text: "", tags: "sortTag"});
+		wiki.addTiddler({ title: "B", text: "", tags: "sortTag", "list-after": "Z"});
+		wiki.addTiddler({ title: "C", text: "", tags: "sortTag"});
+		wiki.addTiddler({ title: "Z", text: "", tags: "EXCLUDED", "list-before": ""});
+
+		expect(wiki.filterTiddlers("[tag[sortTag]]").join(',')).toBe("A,B,C");
+	});
+
+	it("should handle javascript-specific titles", function() {
+		var wiki = new $tw.Wiki();
+
+		wiki.addTiddler({ title: "A", text: "", tags: "sortTag"});
+		wiki.addTiddler({ title: "__proto__", text: "", tags: "sortTag", "list-before": ""});
+
+		expect(wiki.filterTiddlers("[tag[sortTag]]").join(',')).toBe("__proto__,A");
+	});
 });
 
 })();