[snippets] Fix incorrect range traversal when resolving variables

When we expand a snippet, we know where the variables are within the snippet; they're described using `Point`s, but the origin of the coordinate system is the beginning of the snippet. To translate them into actual buffer `Point`s during snippet expansion, we “add” each point to the `Point` that marks the insertion point of the snippet.

When doing so, we need to remember that, when a snippet contains a newline and content after that newline, that second line of content will be indented by the same amount as the initial line; we know how much leading whitespace (if any) there was before the snippet trigger text and apply it at the head of each line of the expansion.

For this reason, we were adding the two points incorrectly. Since the column position doesn't reset to `0` each time a row is advanced, we should've been using `Point#translate`, not `Point#traverse`.

I'd have caught this earlier if I had thought to test the combination of variable expansion and leading whitespace.

I see other usages of `Point#traverse` in different contexts in this file, but I'll leave them be until they can be proven to be the source of a bug.

packages/snippets/lib/snippet-expansion.js

@@ -215,10 +215,12 @@ module.exports = class SnippetExpansion {
     // markers are crucial for ensuring we adapt to those changes.
     for (const variable of this.snippet.variables) {
       const {point} = variable
-      const marker = this.getMarkerLayer(this.editor).markBufferRange([
-        startPosition.traverse(point),
-        startPosition.traverse(point)
-      ], {exclusive: false})
+      let markerRange = new Range(
+        startPosition.translate(point),
+        startPosition.translate(point)
+      )
+      const marker = this.getMarkerLayer(this.editor).markBufferRange(
+        markerRange, {exclusive: false})
       this.markersForVariables.set(variable, marker)
     }
   }

packages/snippets/spec/.eslintrc

@@ -1,5 +1,14 @@
 {
+  "env": { "jasmine": true },
   "rules": {
+    "node/no-unpublished-require": "off",
+    "node/no-extraneous-require": "off",
+    "no-unused-vars": "off",
+    "no-empty": "off",
+    "object-curly-spacing": ["error", "always"],
     "semi": ["error", "always"]
+  },
+  "globals": {
+    "waitsForPromise": true
   }
 }

packages/snippets/spec/snippets-spec.js

@@ -1,7 +1,7 @@
 const path = require('path');
 const temp = require('temp').track();
 const Snippets = require('../lib/snippets');
-const {TextEditor} = require('atom');
+const { TextEditor } = require('atom');
 const crypto = require('crypto');
 
 const SUPPORTS_UUID = ('randomUUID' in crypto) && (typeof crypto.randomUUID === 'function');
@@ -14,8 +14,8 @@ describe("Snippets extension", () => {
     if (param == null) {
       param = {};
     }
-    const {shift} = param;
-    const event = atom.keymaps.constructor.buildKeydownEvent('tab', {shift, target: editorElement});
+    const { shift } = param;
+    const event = atom.keymaps.constructor.buildKeydownEvent('tab', { shift, target: editorElement });
     atom.keymaps.handleKeyboardEvent(event);
   };
 
@@ -113,7 +113,7 @@ describe("Snippets extension", () => {
     invalidSnippets = 3;
     expect(snippets.getSnippets(editor)).toEqual({});
 
-    invalidSnippets = {a: null};
+    invalidSnippets = { a: null };
     expect(snippets.getSnippets(editor)).toEqual({});
   });
 
@@ -499,14 +499,14 @@ third tabstop $3\
           expect(editor.getSelectedBufferRange()).toEqual([[2, 40], [2, 40]]);
 
           // tab backwards
-          simulateTabKeyEvent({shift: true});
+          simulateTabKeyEvent({ shift: true });
           expect(editor.getSelectedBufferRange()).toEqual([[2, 14], [2, 17]]); // should highlight text typed at tab stop
 
-          simulateTabKeyEvent({shift: true});
+          simulateTabKeyEvent({ shift: true });
           expect(editor.getSelectedBufferRange()).toEqual([[3, 15], [3, 15]]);
 
           // shift-tab on first tab-stop does nothing
-          simulateTabKeyEvent({shift: true});
+          simulateTabKeyEvent({ shift: true });
           expect(editor.getCursorScreenPosition()).toEqual([3, 15]);
 
           // tab through all tab stops, then tab on last stop to terminate snippet
@@ -575,7 +575,7 @@ third tabstop $3\
             simulateTabKeyEvent();
 
             editor.moveRight();
-            simulateTabKeyEvent({shift: true});
+            simulateTabKeyEvent({ shift: true });
             expect(editor.getCursorBufferPosition()).toEqual([4, 15]);
           });
         });
@@ -672,7 +672,7 @@ third tabstop $3\
 
       describe("when the snippet spans multiple lines", () => {
         beforeEach(async () => {
-          editor.update({autoIndent: true});
+          editor.update({ autoIndent: true });
           // editor.update() returns a Promise that never gets resolved, so we
           // need to return undefined to avoid a timeout in the spec.
           // TODO: Figure out why `editor.update({autoIndent: true})` never gets resolved.
@@ -1055,6 +1055,17 @@ foo\
           editor.insertText('TEST');
           expect(editor.getText()).toBe("// TEST\n// ====");
         });
+
+        describe("and the editor is indented", () => {
+          it("traverses newlines properly", () => {
+            editor.setText('  bannerGeneric');
+            editor.setCursorScreenPosition([0, 15]);
+            simulateTabKeyEvent();
+            expect(editor.getText()).toBe("  // \n  // ");
+            editor.insertText('TEST');
+            expect(editor.getText()).toBe("  // TEST\n  // ====");
+          });
+        });
       });
 
       describe("and the document is HTML", () => {