Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(language-service): HTML path should include last node before curs…
…or (#34440)

Given the following HTML and cursor position:
```
<div c|></div>
      ^ cursor is here
```

Note that the cursor is **after** the attribute `c`.

Under the current implementation, only `Element` is included in the
path. Instead, it should be `Element -> Attribute`.

This bug occurs only for cases where the cursor is right after the Node,
and it is because the `end` position of the span is excluded from the search.
Instead, the `end` position should be included.

PR Close #34440
  • Loading branch information
kyliau authored and kara committed Dec 16, 2019
1 parent 11257b8 commit 76e4870
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 10 deletions.
6 changes: 3 additions & 3 deletions packages/language-service/src/completions.ts
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, ImplicitReceiver, NAMED_ENTITIES, Node as HtmlAst, NullTemplateVisitor, ParseSpan, PropertyRead, TagContentType, TemplateBinding, Text, findNode, getHtmlTagDefinition} from '@angular/compiler';
import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, ImplicitReceiver, NAMED_ENTITIES, Node as HtmlAst, NullTemplateVisitor, ParseSpan, PropertyRead, TagContentType, TemplateBinding, Text, getHtmlTagDefinition} from '@angular/compiler';
import {$$, $_, isAsciiLetter, isDigit} from '@angular/compiler/src/chars';

import {AstResult} from './common';
Expand All @@ -15,7 +15,7 @@ import {getExpressionCompletions} from './expressions';
import {attributeNames, elementNames, eventNames, propertyNames} from './html_info';
import {InlineTemplate} from './template';
import * as ng from './types';
import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getSelectors, hasTemplateReference, inSpan, spanOf} from './utils';
import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getPathToNodeAtPosition, getSelectors, hasTemplateReference, inSpan, spanOf} from './utils';

const HIDDEN_HTML_ELEMENTS: ReadonlySet<string> =
new Set(['html', 'script', 'noscript', 'base', 'body', 'title', 'head', 'link']);
Expand Down Expand Up @@ -121,7 +121,7 @@ export function getTemplateCompletions(
const {htmlAst, template} = templateInfo;
// The templateNode starts at the delimiter character so we add 1 to skip it.
const templatePosition = position - template.span.start;
const path = findNode(htmlAst, templatePosition);
const path = getPathToNodeAtPosition(htmlAst, templatePosition);
const mostSpecific = path.tail;
if (path.empty || !mostSpecific) {
result = elementCompletions(templateInfo);
Expand Down
5 changes: 3 additions & 2 deletions packages/language-service/src/expression_diagnostics.ts
Expand Up @@ -6,12 +6,13 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, CompileDirectiveSummary, CompileTypeMetadata, DirectiveAst, ElementAst, EmbeddedTemplateAst, Node, ParseSourceSpan, RecursiveTemplateAstVisitor, ReferenceAst, TemplateAst, TemplateAstPath, VariableAst, findNode, identifierName, templateVisitAll, tokenReference} from '@angular/compiler';
import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, CompileDirectiveSummary, CompileTypeMetadata, DirectiveAst, ElementAst, EmbeddedTemplateAst, Node, ParseSourceSpan, RecursiveTemplateAstVisitor, ReferenceAst, TemplateAst, TemplateAstPath, VariableAst, identifierName, templateVisitAll, tokenReference} from '@angular/compiler';
import * as ts from 'typescript';

import {AstType, ExpressionDiagnosticsContext, TypeDiagnostic} from './expression_type';
import {BuiltinType, Definition, Span, Symbol, SymbolDeclaration, SymbolQuery, SymbolTable} from './symbols';
import {Diagnostic} from './types';
import {getPathToNodeAtPosition} from './utils';

export interface DiagnosticTemplateInfo {
fileName?: string;
Expand Down Expand Up @@ -304,7 +305,7 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor {
}

private attributeValueLocation(ast: TemplateAst) {
const path = findNode(this.info.htmlAst, ast.sourceSpan.start.offset);
const path = getPathToNodeAtPosition(this.info.htmlAst, ast.sourceSpan.start.offset);
const last = path.tail;
if (last instanceof Attribute && last.valueSpan) {
return last.valueSpan.start.offset;
Expand Down
6 changes: 3 additions & 3 deletions packages/language-service/src/locate_symbol.ts
Expand Up @@ -6,13 +6,13 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AST, Attribute, BoundDirectivePropertyAst, BoundEventAst, CompileTypeSummary, CssSelector, DirectiveAst, ElementAst, SelectorMatcher, TemplateAstPath, findNode, tokenReference} from '@angular/compiler';
import {AST, Attribute, BoundDirectivePropertyAst, BoundEventAst, CompileTypeSummary, CssSelector, DirectiveAst, ElementAst, SelectorMatcher, TemplateAstPath, tokenReference} from '@angular/compiler';

import {AstResult} from './common';
import {getExpressionScope} from './expression_diagnostics';
import {getExpressionSymbol} from './expressions';
import {Definition, DirectiveKind, Span, Symbol} from './types';
import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, inSpan, offsetSpan, spanOf} from './utils';
import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getPathToNodeAtPosition, inSpan, offsetSpan, spanOf} from './utils';

export interface SymbolInfo {
symbol: Symbol;
Expand Down Expand Up @@ -144,7 +144,7 @@ export function locateSymbol(info: AstResult, position: number): SymbolInfo|unde

function findAttribute(info: AstResult, position: number): Attribute|undefined {
const templatePosition = position - info.template.span.start;
const path = findNode(info.htmlAst, templatePosition);
const path = getPathToNodeAtPosition(info.htmlAst, templatePosition);
return path.first(Attribute);
}

Expand Down
26 changes: 25 additions & 1 deletion packages/language-service/src/utils.ts
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AstPath, CompileDirectiveSummary, CompileTypeMetadata, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, ParseSourceSpan, RecursiveTemplateAstVisitor, TemplateAst, TemplateAstPath, identifierName, templateVisitAll} from '@angular/compiler';
import {AstPath, CompileDirectiveSummary, CompileTypeMetadata, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, HtmlAstPath, Node, ParseSourceSpan, RecursiveTemplateAstVisitor, RecursiveVisitor, TemplateAst, TemplateAstPath, identifierName, templateVisitAll, visitAll} from '@angular/compiler';
import * as ts from 'typescript';

import {AstResult, SelectorInfo} from './common';
Expand Down Expand Up @@ -225,3 +225,27 @@ export function findPropertyValueOfType<T extends ts.Node>(
}
return startNode.forEachChild(c => findPropertyValueOfType(c, propName, predicate));
}

/**
* Find the tightest node at the specified `position` from the AST `nodes`, and
* return the path to the node.
* @param nodes HTML AST nodes
* @param position
*/
export function getPathToNodeAtPosition(nodes: Node[], position: number): HtmlAstPath {
const path: Node[] = [];
const visitor = new class extends RecursiveVisitor {
visit(ast: Node) {
const span = spanOf(ast);
if (inSpan(position, span)) {
path.push(ast);
} else {
// Returning a truthy value here will skip all children and terminate
// the visit.
return true;
}
}
};
visitAll(visitor, nodes);
return new AstPath<Node>(path, position);
}
50 changes: 49 additions & 1 deletion packages/language-service/test/utils_spec.ts
Expand Up @@ -6,8 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/

import * as ng from '@angular/compiler';
import * as ts from 'typescript';
import {getDirectiveClassLike} from '../src/utils';

import {getDirectiveClassLike, getPathToNodeAtPosition} from '../src/utils';

describe('getDirectiveClassLike()', () => {
it('should return a directive class', () => {
Expand All @@ -32,3 +34,49 @@ describe('getDirectiveClassLike()', () => {
expect(classDecl.name !.text).toBe('AppModule');
});
});

describe('getPathToNodeAtPosition', () => {
const html = '<div c></div>';
const nodes: ng.Node[] = [];

beforeAll(() => {
const parser = new ng.HtmlParser();
const {rootNodes, errors} = parser.parse(html, 'url');
expect(errors.length).toBe(0);
nodes.push(...rootNodes);
});

it('must capture element', () => {
// First, try to get a Path to the Element
// <|div c></div>
// ^ cursor is here
const position = html.indexOf('div');
const path = getPathToNodeAtPosition(nodes, position);
// There should be just 1 node in the path, the Element node
expect(path.empty).toBe(false);
expect(path.head instanceof ng.Element).toBe(true);
expect(path.head).toBe(path.tail);
});

it('must capture attribute', () => {
// Then, try to get a Path to the Attribute
// <div |c></div>
// ^ cusor is here, before the attribute
const position = html.indexOf('c');
const path = getPathToNodeAtPosition(nodes, position);
expect(path.empty).toBe(false);
expect(path.head instanceof ng.Element).toBe(true);
expect(path.tail instanceof ng.Attribute).toBe(true);
});

it('must capture attribute before cursor', () => {
// Finally, try to get a Path to the attribute after the 'c' text
// <div c|></div>
// ^ cursor is here, after the attribute
const position = html.indexOf('c') + 1;
const path = getPathToNodeAtPosition(nodes, position);
expect(path.empty).toBe(false);
expect(path.head instanceof ng.Element).toBe(true);
expect(path.tail instanceof ng.Attribute).toBe(true);
});
});

0 comments on commit 76e4870

Please sign in to comment.