Fix HTML element close tag auto-insertion inside Liquid branches (#516)

There was a bug with our findCurrentNode algorithm that made it so we'd go in the wrong node when a branch had child nodes.

The fix was to have more information on the AST and flag LiquidBranch as "unclosed" when their blockEndPosition is -1, -1.

It's kind of a nasty bug tbh.

Fixes #515

Author: charlespwd

.changeset/green-otters-smoke.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-language-server-common': patch
+---
+
+Fix HTML element close tag auto-insertion inside Liquid branches

.changeset/moody-books-remember.md

@@ -0,0 +1,5 @@
+---
+'@shopify/liquid-html-parser': patch
+---
+
+LiquidBranch nodes should always report a blockEndPosition of 0 length

packages/liquid-html-parser/src/stage-2-ast.spec.ts

@@ -681,6 +681,12 @@ describe('Unit: Stage 2 (AST)', () => {
         expectPath(ast, 'children.0.children.0.blockStartPosition.end').to.equal(
           source.indexOf(branchA),
         );
+        expectPath(ast, 'children.0.children.0.blockEndPosition.start').to.equal(
+          source.indexOf('{% elsif b %}'),
+        );
+        expectPath(ast, 'children.0.children.0.blockEndPosition.end').to.equal(
+          source.indexOf('{% elsif b %}'),
+        );
 
         expectPath(ast, 'children.0.children.1.type').to.equal('LiquidBranch');
         expectPath(ast, 'children.0.children.1.position.start').to.equal(
@@ -695,6 +701,12 @@ describe('Unit: Stage 2 (AST)', () => {
         expectPath(ast, 'children.0.children.1.blockStartPosition.end').to.equal(
           source.indexOf('{% elsif b %}') + '{% elsif b %}'.length,
         );
+        expectPath(ast, 'children.0.children.1.blockEndPosition.start').to.equal(
+          source.indexOf('{% else %}'),
+        );
+        expectPath(ast, 'children.0.children.1.blockEndPosition.end').to.equal(
+          source.indexOf('{% else %}'),
+        );
 
         expectPath(ast, 'children.0.children.2.type').to.equal('LiquidBranch');
         expectPath(ast, 'children.0.children.2.position.start').to.equal(
@@ -709,6 +721,12 @@ describe('Unit: Stage 2 (AST)', () => {
         expectPath(ast, 'children.0.children.2.blockStartPosition.end').to.equal(
           source.indexOf('{% else %}') + '{% else %}'.length,
         );
+        expectPath(ast, 'children.0.children.2.blockEndPosition.start').to.equal(
+          source.indexOf('{% endif %}', source.indexOf(branchC) + branchC.length),
+        );
+        expectPath(ast, 'children.0.children.2.blockEndPosition.end').to.equal(
+          source.indexOf('{% endif %}', source.indexOf(branchC) + branchC.length),
+        );
       }
     });
 

packages/liquid-html-parser/src/stage-2-ast.ts

@@ -441,6 +441,8 @@ interface LiquidBranchNode<Name, Markup> extends ASTNode<NodeTypes.LiquidBranch>
   whitespaceEnd: '-' | '';
   /** Range of the LiquidTag that delimits the branch (does not include children) */
   blockStartPosition: Position;
+  /** 0-size range that delimits where the branch ends, (-1, -1) when unclosed */
+  blockEndPosition: Position;
 }
 
 /**
@@ -577,7 +579,7 @@ export interface HtmlElement extends HtmlNodeBase<NodeTypes.HtmlElement> {
  * The node used to represent close tags without its matching opening tag.
  *
  * Typically found inside if statements.
- *
+
  * ```
  * {% if cond %}
  *   </wrapper>
@@ -890,6 +892,7 @@ class ASTBuilder {
     if (node.type === NodeTypes.LiquidBranch) {
       const previousBranch = this.findCloseableParentBranch(node);
       if (previousBranch) {
+        previousBranch.blockEndPosition = { start: node.position.start, end: node.position.start };
         // close dangling open HTML nodes
         while (
           this.parent &&
@@ -911,6 +914,7 @@ class ASTBuilder {
 
   close(node: ConcreteCloseNode, nodeType: NodeTypes.LiquidTag | NodeTypes.HtmlElement) {
     if (isLiquidBranch(this.parent)) {
+      this.parent.blockEndPosition = { start: node.locStart, end: node.locStart };
       this.closeParentWith(node);
     }
 
@@ -1328,6 +1332,7 @@ function liquidBranchBaseAttributes(
     whitespaceStart: node.whitespaceStart ?? '',
     whitespaceEnd: node.whitespaceEnd ?? '',
     blockStartPosition: position(node),
+    blockEndPosition: { start: -1, end: -1 },
     source: node.source,
   };
 }
@@ -1514,6 +1519,7 @@ function toNamedLiquidBranchBaseCase(node: ConcreteLiquidTagBaseCase): LiquidBra
     position: { start: node.locStart, end: node.locEnd },
     children: [],
     blockStartPosition: { start: node.locStart, end: node.locEnd },
+    blockEndPosition: { start: -1, end: -1 },
     whitespaceStart: node.whitespaceStart ?? '',
     whitespaceEnd: node.whitespaceEnd ?? '',
     source: node.source,
@@ -1525,14 +1531,9 @@ function toUnnamedLiquidBranch(parentNode: LiquidHtmlNode): LiquidBranchUnnamed
     type: NodeTypes.LiquidBranch,
     name: null,
     markup: '',
-    position: {
-      start: parentNode.position.end,
-      end: parentNode.position.end, // tmp value
-    },
-    blockStartPosition: {
-      start: parentNode.position.end,
-      end: parentNode.position.end,
-    },
+    position: { start: parentNode.position.end, end: parentNode.position.end },
+    blockStartPosition: { start: parentNode.position.end, end: parentNode.position.end },
+    blockEndPosition: { start: -1, end: -1 },
     children: [],
     whitespaceStart: '',
     whitespaceEnd: '',

packages/theme-language-server-common/src/formatting/providers/HtmlElementAutoclosingOnTypeFormattingProvider.spec.ts

@@ -129,7 +129,11 @@ describe('Module: HtmlElementAutoclosingOnTypeFormattingProvider', () => {
     const scenarios = [
       '{% if cond %}<div>█{% endif %}',
       '{% if cond %}{% else %}<div>█{% endif %}',
+      '{% if cond %}<div></div>{% else %}<div>█{% endif %}',
+      '{% if cond %}<div></div>{% elsif cond %}<div>█{% else %}<div>{% endif %}',
+      '{% if cond %}<div></div>{% elsif cond %}<div>█ {% else %}<div>{% endif %}',
       '{% unless cond %}<div>█{% endunless %}',
+      '{% unless cond %}<div><div>█{% endunless %}',
       '{% case thing %}{% when thing %}<div>█{% endif %}',
     ];
     for (const source of scenarios) {
@@ -144,7 +148,7 @@ describe('Module: HtmlElementAutoclosingOnTypeFormattingProvider', () => {
       };
       assert(document);
       const result = await onTypeFormattingProvider.onTypeFormatting(params);
-      assert(result);
+      assert(result, 'expected results for source:\n' + source);
       expect(TextDocument.applyEdits(document, result)).to.equal(source.replace(CURSOR, '</div>'));
 
       vi.advanceTimersByTime(10);

packages/theme-language-server-common/src/hover/providers/LiquidTagHoverProvider.spec.ts

@@ -18,16 +18,17 @@ describe('Module: LiquidTagHoverProvider', async () => {
   });
 
   it('should return the hover description of the correct tag', async () => {
+    // cursor always points at character before █
     await expect(provider).to.hover(`{%█ if cond %}{% endif %}`, expect.stringContaining('if'));
     await expect(provider).to.hover(`{% i█f cond %}{% endif %}`, expect.stringContaining('if'));
     await expect(provider).to.hover(`{% if█ cond %}{% endif %}`, expect.stringContaining('if'));
+    await expect(provider).to.hover(`{% if █cond %}{% endif %}`, expect.stringContaining('if'));
     await expect(provider).to.hover(`{% if cond █%}{% endif %}`, expect.stringContaining('if'));
     await expect(provider).to.hover(`{% if cond %}{% █ endif %}`, expect.stringContaining('if'));
     await expect(provider).to.hover(`{% echo█ 'hi' %}`, expect.stringContaining('echo'));
   });
 
   it('should not return the tag hover description when hovering over anything else in the tag', async () => {
-    await expect(provider).to.not.hover(`{% if █cond %}{% endif %}`, expect.stringContaining('if'));
     await expect(provider).to.not.hover(`{% if c█ond %}{% endif %}`, expect.stringContaining('if'));
     await expect(provider).to.not.hover(
       `{% if cond %} █ {%  endif %}`,

packages/theme-language-server-common/src/linkedEditingRanges/providers/HtmlTagNameLinkedRangesProvider.spec.ts

@@ -29,7 +29,7 @@ describe('Module: HtmlTagNameLinkedRangesProvider', () => {
     it('should return linked editing ranges for HTML tag names', async () => {
       params = {
         textDocument: { uri },
-        position: document.positionAt(1), // position within the opening div#main tag name
+        position: document.positionAt(2), // position within the opening div#main tag name
       };
 
       const result = await provider.linkedEditingRanges(params);

packages/theme-language-server-common/src/rename/providers/HtmlTagNameRenameProvider.spec.ts

@@ -27,7 +27,7 @@ describe('HtmlTagNameRenameProvider', () => {
   it('returns the correct workspace edit when renaming an HTML tag name', async () => {
     const params = {
       textDocument: { uri: 'file:///path/to/document.liquid' },
-      position: Position.create(0, 1),
+      position: Position.create(0, 2),
       newName: 'new-name',
     };
     documentManager.open(params.textDocument.uri, '<old><old></old></old>', 1);
@@ -53,7 +53,7 @@ describe('HtmlTagNameRenameProvider', () => {
   it('also works on complext liquid + text html tag names', async () => {
     const params = {
       textDocument: { uri: 'file:///path/to/document.liquid' },
-      position: Position.create(0, 1),
+      position: Position.create(0, 2),
       newName: 'new-name',
     };
     documentManager.open(params.textDocument.uri, '<web--{{ comp }}>text</web--{{ comp }}>', 1);

packages/theme-language-server-common/src/visitor.ts

@@ -2,8 +2,8 @@ import {
   AST,
   LiquidHtmlNode,
   NodeOfType,
-  NodeTypes,
   SourceCodeType,
+  NodeTypes,
 } from '@shopify/theme-check-common';
 
 export type VisitorMethod<S extends SourceCodeType, T, R> = (
@@ -108,7 +108,7 @@ export function findCurrentNode(
 }
 
 function isCovered(node: LiquidHtmlNode, offset: number): boolean {
-  return node.position.start <= offset && offset <= node.position.end;
+  return node.position.start < offset && offset <= node.position.end;
 }
 
 function size(node: LiquidHtmlNode): number {