From 5df8a3ba954e7908a1a34b9383c776329fe75924 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Mon, 16 Dec 2019 12:11:39 -0800 Subject: [PATCH] fix(language-service): HTML path should include last node before cursor (#34440) Given the following HTML and cursor position: ```
^ 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 --- packages/language-service/src/completions.ts | 6 +-- .../src/expression_diagnostics.ts | 5 +- .../language-service/src/locate_symbol.ts | 6 +-- packages/language-service/src/utils.ts | 26 +++++++++- packages/language-service/test/utils_spec.ts | 50 ++++++++++++++++++- 5 files changed, 83 insertions(+), 10 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 19fe6c7a495fb..effa0a367c43b 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -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'; @@ -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 = new Set(['html', 'script', 'noscript', 'base', 'body', 'title', 'head', 'link']); @@ -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); diff --git a/packages/language-service/src/expression_diagnostics.ts b/packages/language-service/src/expression_diagnostics.ts index ccf825acf8eb7..2382fd85f06d0 100644 --- a/packages/language-service/src/expression_diagnostics.ts +++ b/packages/language-service/src/expression_diagnostics.ts @@ -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; @@ -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; diff --git a/packages/language-service/src/locate_symbol.ts b/packages/language-service/src/locate_symbol.ts index 499d3f95b1018..63086ed4ca8c8 100644 --- a/packages/language-service/src/locate_symbol.ts +++ b/packages/language-service/src/locate_symbol.ts @@ -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; @@ -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); } diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 65a187c7debf5..394fed7c37eac 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -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'; @@ -225,3 +225,27 @@ export function findPropertyValueOfType( } 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(path, position); +} diff --git a/packages/language-service/test/utils_spec.ts b/packages/language-service/test/utils_spec.ts index f032da97b529a..8419924c2b8dd 100644 --- a/packages/language-service/test/utils_spec.ts +++ b/packages/language-service/test/utils_spec.ts @@ -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', () => { @@ -32,3 +34,49 @@ describe('getDirectiveClassLike()', () => { expect(classDecl.name !.text).toBe('AppModule'); }); }); + +describe('getPathToNodeAtPosition', () => { + const html = '
'; + 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> + // ^ 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 + //
+ // ^ 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 + //
+ // ^ 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); + }); +});