From 1a7a7360b081aeb61af2c418ac5cb74f7cb4036e Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 18 Jul 2020 00:05:11 +0200 Subject: [PATCH] fix(compiler): properly associate source spans for implicitly closed elements (#38126) HTML is very lenient when it comes to closing elements, so Angular's parser has rules that specify which elements are implicitly closed when closing a tag. The parser keeps track of the nesting of tag names using a stack and parsing a closing tag will pop as many elements off the stack as possible, provided that the elements can be implicitly closed. For example, consider the following templates: - `

`, the `
` is implicitly closed when parsing ``, because `
` is a void element. - `

`, the `

` is implicitly closed when parsing ``, as `

` is allowed to be closed by the closing of its parent element. - `

`, the first `
  • ` is implicitly closed when parsing the second `
  • `, whereas the second `
  • ` would be implicitly closed when parsing the ``. In all the cases above the parsed structure would be correct, however the source span of the closing `` would incorrectly be assigned to the element that is implicitly closed. The problem was that closing an element would associate the source span with the element at the top of the stack, however this may not be the element that is actually being closed if some elements would be implicitly closed. This commit fixes the issue by assigning the end source span with the element on the stack that is actually being closed. Any implicitly closed elements that are popped off the stack will not be assigned an end source span, as the implicit closing implies that no ending element is present. Note that there is a difference between self-closed elements such as `` and implicitly closed elements such as ``. The former does have an end source span (identical to its start source span) whereas the latter does not. Fixes #36118 Resolves FW-2004 PR Close #38126 --- packages/compiler/src/ml_parser/parser.ts | 18 +++--- .../compiler/test/ml_parser/ast_spec_utils.ts | 4 ++ .../test/ml_parser/html_parser_spec.ts | 56 ++++++++++++++++++- 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/packages/compiler/src/ml_parser/parser.ts b/packages/compiler/src/ml_parser/parser.ts index 003404a06d..b31db0c25c 100644 --- a/packages/compiler/src/ml_parser/parser.ts +++ b/packages/compiler/src/ml_parser/parser.ts @@ -261,8 +261,9 @@ class _TreeBuilder { const el = new html.Element(fullName, attrs, [], span, span, undefined); this._pushElement(el); if (selfClosing) { - this._popElement(fullName); - el.endSourceSpan = span; + // Elements that are self-closed have their `endSourceSpan` set to the full span, as the + // element start tag also represents the end tag. + this._popElement(fullName, span); } } @@ -281,25 +282,26 @@ class _TreeBuilder { const fullName = this._getElementFullName( endTagToken.parts[0], endTagToken.parts[1], this._getParentElement()); - if (this._getParentElement()) { - this._getParentElement()!.endSourceSpan = endTagToken.sourceSpan; - } - if (this.getTagDefinition(fullName).isVoid) { this.errors.push(TreeError.create( fullName, endTagToken.sourceSpan, `Void elements do not have end tags "${endTagToken.parts[1]}"`)); - } else if (!this._popElement(fullName)) { + } else if (!this._popElement(fullName, endTagToken.sourceSpan)) { const errMsg = `Unexpected closing tag "${ fullName}". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags`; this.errors.push(TreeError.create(fullName, endTagToken.sourceSpan, errMsg)); } } - private _popElement(fullName: string): boolean { + private _popElement(fullName: string, endSourceSpan: ParseSourceSpan): boolean { for (let stackIndex = this._elementStack.length - 1; stackIndex >= 0; stackIndex--) { const el = this._elementStack[stackIndex]; if (el.name == fullName) { + // Record the parse span with the element that is being closed. Any elements that are + // removed from the element stack at this point are closed implicitly, so they won't get + // an end source span (as there is no explicit closing element). + el.endSourceSpan = endSourceSpan; + this._elementStack.splice(stackIndex, this._elementStack.length - stackIndex); return true; } diff --git a/packages/compiler/test/ml_parser/ast_spec_utils.ts b/packages/compiler/test/ml_parser/ast_spec_utils.ts index 1906591cda..10060905bb 100644 --- a/packages/compiler/test/ml_parser/ast_spec_utils.ts +++ b/packages/compiler/test/ml_parser/ast_spec_utils.ts @@ -41,6 +41,10 @@ class _Humanizer implements html.Visitor { visitElement(element: html.Element, context: any): any { const res = this._appendContext(element, [html.Element, element.name, this.elDepth++]); + if (this.includeSourceSpan) { + res.push(element.startSourceSpan?.toString() ?? null); + res.push(element.endSourceSpan?.toString() ?? null); + } this.result.push(res); html.visitAll(this, element.attrs); html.visitAll(this, element.children); diff --git a/packages/compiler/test/ml_parser/html_parser_spec.ts b/packages/compiler/test/ml_parser/html_parser_spec.ts index 4370a1118e..af167cba94 100644 --- a/packages/compiler/test/ml_parser/html_parser_spec.ts +++ b/packages/compiler/test/ml_parser/html_parser_spec.ts @@ -583,7 +583,10 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe expect(humanizeDomSourceSpans(parser.parse( '
    \na\n
    ', 'TestComp'))) .toEqual([ - [html.Element, 'div', 0, '
    '], + [ + html.Element, 'div', 0, '
    ', + '
    ', '
    ' + ], [html.Attribute, '[prop]', 'v1', '[prop]="v1"'], [html.Attribute, '(e)', 'do()', '(e)="do()"'], [html.Attribute, 'attr', 'v2', 'attr="v2"'], @@ -602,12 +605,61 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe expect(node.endSourceSpan!.end.offset).toEqual(12); }); + it('should not set the end source span for void elements', () => { + expect(humanizeDomSourceSpans(parser.parse('

    ', 'TestComp'))).toEqual([ + [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'br', 1, '
    ', '
    ', null], + ]); + }); + + it('should not set the end source span for multiple void elements', () => { + expect(humanizeDomSourceSpans(parser.parse('

    ', 'TestComp'))).toEqual([ + [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'br', 1, '
    ', '
    ', null], + [html.Element, 'hr', 1, '
    ', '
    ', null], + ]); + }); + + it('should not set the end source span for standalone void elements', () => { + expect(humanizeDomSourceSpans(parser.parse('
    ', 'TestComp'))).toEqual([ + [html.Element, 'br', 0, '
    ', '
    ', null], + ]); + }); + + it('should set the end source span for standalone self-closing elements', () => { + expect(humanizeDomSourceSpans(parser.parse('
    ', 'TestComp'))).toEqual([ + [html.Element, 'br', 0, '
    ', '
    ', '
    '], + ]); + }); + + it('should set the end source span for self-closing elements', () => { + expect(humanizeDomSourceSpans(parser.parse('

    ', 'TestComp'))).toEqual([ + [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'br', 1, '
    ', '
    ', '
    '], + ]); + }); + + it('should not set the end source span for elements that are implicitly closed', () => { + expect(humanizeDomSourceSpans(parser.parse('

    ', 'TestComp'))).toEqual([ + [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'p', 1, '

    ', '

    ', null], + ]); + expect(humanizeDomSourceSpans(parser.parse('

  • A
  • B', 'TestComp'))) + .toEqual([ + [html.Element, 'div', 0, '
    ', '
    ', '
    '], + [html.Element, 'li', 1, '
  • ', '
  • ', null], + [html.Text, 'A', 2, 'A'], + [html.Element, 'li', 1, '
  • ', '
  • ', null], + [html.Text, 'B', 2, 'B'], + ]); + }); + it('should support expansion form', () => { expect(humanizeDomSourceSpans(parser.parse( '
    {count, plural, =0 {msg}}
    ', 'TestComp', {tokenizeExpansionForms: true}))) .toEqual([ - [html.Element, 'div', 0, '
    '], + [html.Element, 'div', 0, '
    ', '
    ', '
    '], [html.Expansion, 'count', 'plural', 1, '{count, plural, =0 {msg}}'], [html.ExpansionCase, '=0', 2, '=0 {msg}'], ]);