Skip to content

Commit

Permalink
fix(language-service): Use last child end span for parent without clo…
Browse files Browse the repository at this point in the history
…se tag (#42554)

Unclosed element tags are not assigned an `endSourceSpan` by the parser.
As a result, the visitor which determines the target node at a position
for the language service was unable to determine that a given position
was inside an unclosed parent. This happens because we update the
`endSourceSpan` of template/element nodes to be the end tag (and there
is not one for unclosed tags). Consequently, the visitor then cannot
match a position to any child node location.

This change updates the visitor logic to check if there are any
`children` of a template/element node and updates the end span to be the
end span of the last child. This allows our `isWithin` logic to identify
that a child position is within the unclosed parent.

Addresses one of the issues found during investigation of angular/vscode-ng-language-service#1399

PR Close #42554
  • Loading branch information
atscott authored and alxhub committed Jun 14, 2021
1 parent 8c1e0e6 commit 228beea
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 3 deletions.
14 changes: 11 additions & 3 deletions packages/language-service/ivy/template_target.ts
Expand Up @@ -286,7 +286,7 @@ class TemplateTargetVisitor implements t.Visitor {

visit(node: t.Node) {
const {start, end} = getSpanIncludingEndTag(node);
if (!isWithin(this.position, {start, end})) {
if (end !== null && !isWithin(this.position, {start, end})) {
return;
}

Expand Down Expand Up @@ -441,8 +441,16 @@ function getSpanIncludingEndTag(ast: t.Node) {
// the end of the closing tag. Otherwise, for situation like
// <my-component></my-comp¦onent> where the cursor is in the closing tag
// we will not be able to return any information.
if ((ast instanceof t.Element || ast instanceof t.Template) && ast.endSourceSpan) {
result.end = ast.endSourceSpan.end.offset;
if (ast instanceof t.Element || ast instanceof t.Template) {
if (ast.endSourceSpan) {
result.end = ast.endSourceSpan.end.offset;
} else if (ast.children.length > 0) {
// If the AST has children but no end source span, then it is an unclosed element with an end
// that should be the end of the last child.
result.end = getSpanIncludingEndTag(ast.children[ast.children.length - 1]).end;
} else {
// This is likely a self-closing tag with no children so the `sourceSpan.end` is correct.
}
}
return result;
}
36 changes: 36 additions & 0 deletions packages/language-service/ivy/test/legacy/template_target_spec.ts
Expand Up @@ -797,3 +797,39 @@ describe('findNodeAtPosition for microsyntax expression', () => {
expect((context as SingleNodeTarget).node).toBeInstanceOf(t.Element);
});
});

describe('unclosed elements', () => {
it('should locate children of unclosed elements', () => {
const {errors, nodes, position} = parse(`<div> {{b¦ar}}`);
expect(errors).toBe(null);
const {context} = getTargetAtPosition(nodes, position)!;
const {node} = context as SingleNodeTarget;
expect(isExpressionNode(node!)).toBe(true);
expect(node).toBeInstanceOf(e.PropertyRead);
});

it('should locate children of outside of unclosed when parent is closed elements', () => {
const {nodes, position} = parse(`<li><div></li> {{b¦ar}}`);
const {context} = getTargetAtPosition(nodes, position)!;
const {node} = context as SingleNodeTarget;
expect(isExpressionNode(node!)).toBe(true);
expect(node).toBeInstanceOf(e.PropertyRead);
});

it('should locate nodes before unclosed element', () => {
const {nodes, position} = parse(`<li>{{b¦ar}}<div></li>`);
const {context} = getTargetAtPosition(nodes, position)!;
const {node} = context as SingleNodeTarget;
expect(isExpressionNode(node!)).toBe(true);
expect(node).toBeInstanceOf(e.PropertyRead);
});

it('should be correct for end tag of parent node with unclosed child', () => {
const {nodes, position} = parse(`<li><div><div>{{bar}}</l¦i>`);
const {context} = getTargetAtPosition(nodes, position)!;
const {node} = context as SingleNodeTarget;
expect(isTemplateNode(node!)).toBe(true);
expect(node).toBeInstanceOf(t.Element);
expect((node as t.Element).name).toBe('li');
});
});

0 comments on commit 228beea

Please sign in to comment.