Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler): properly associate source spans for implicitly closed elements #38126

Closed
wants to merge 3 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Jul 17, 2020

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:

  • <div><br></div>, the <br> is implicitly closed when parsing </div>,
      because <br> is a void element.
  • <div><p></div>, the <p> is implicitly closed when parsing </div>,
      as <p> is allowed to be closed by the closing of its parent element.
  • <ul><li>A <li>B</ul>, the first <li> is implicitly closed when parsing
      the second <li>, whereas the second <li> would be implicitly closed when
      parsing the </ul>.

In all the cases above the parsed structure would be correct, however the source
span of the closing </div> 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 <input/>
and implicitly closed elements such as <input>. The former does have an end
source span (identical to its start source span) whereas the latter does not.

Fixes #36118
Resolves FW-2004

These methods are no longer used so they can safely be removed.
…elements

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:

- `<div><br></div>`, the `<br>` is implicitly closed when parsing `</div>`,
  because `<br>` is a void element.
- `<div><p></div>`, the `<p>` is implicitly closed when parsing `</div>`,
  as `<p>` is allowed to be closed by the closing of its parent element.
- `<ul><li>A <li>B</ul>`, the first `<li>` is implicitly closed when parsing
  the second `<li>`, whereas the second `<li>` would be implicitly closed when
  parsing the `</ul>`.

In all the cases above the parsed structure would be correct, however the source
span of the closing `</div>` 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 `<input/>`
and implicitly closed elements such as `<input>`. The former does have an end
source span (identical to its start source span) whereas the latter does not.

Fixes angular#36118
Resolves FW-2004
@JoostK JoostK added type: bug/fix freq1: low severity3: broken target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Jul 17, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jul 17, 2020
@JoostK JoostK marked this pull request as ready for review July 17, 2020 22:38
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 17, 2020
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice and clean. Great commit message! And very nice way of testing.
A couple of nits, 'tis all! Thanks for taking this one.

packages/compiler/src/ml_parser/parser.ts Outdated Show resolved Hide resolved
packages/compiler/src/ml_parser/parser.ts Outdated Show resolved Hide resolved
packages/compiler/test/ml_parser/html_parser_spec.ts Outdated Show resolved Hide resolved
@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 18, 2020
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 18, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jul 20, 2020
@sonukapoor
Copy link
Contributor

Really nice and clean. Great commit message! And very nice way of testing.

Very much agreed 💯

AndrewKushnir pushed a commit that referenced this pull request Jul 20, 2020
These methods are no longer used so they can safely be removed.

PR Close #38126
AndrewKushnir pushed a commit that referenced this pull request Jul 20, 2020
…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:

- `<div><br></div>`, the `<br>` is implicitly closed when parsing `</div>`,
  because `<br>` is a void element.
- `<div><p></div>`, the `<p>` is implicitly closed when parsing `</div>`,
  as `<p>` is allowed to be closed by the closing of its parent element.
- `<ul><li>A <li>B</ul>`, the first `<li>` is implicitly closed when parsing
  the second `<li>`, whereas the second `<li>` would be implicitly closed when
  parsing the `</ul>`.

In all the cases above the parsed structure would be correct, however the source
span of the closing `</div>` 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 `<input/>`
and implicitly closed elements such as `<input>`. 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
AndrewKushnir pushed a commit that referenced this pull request Jul 20, 2020
…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:

- `<div><br></div>`, the `<br>` is implicitly closed when parsing `</div>`,
  because `<br>` is a void element.
- `<div><p></div>`, the `<p>` is implicitly closed when parsing `</div>`,
  as `<p>` is allowed to be closed by the closing of its parent element.
- `<ul><li>A <li>B</ul>`, the first `<li>` is implicitly closed when parsing
  the second `<li>`, whereas the second `<li>` would be implicitly closed when
  parsing the `</ul>`.

In all the cases above the parsed structure would be correct, however the source
span of the closing `</div>` 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 `<input/>`
and implicitly closed elements such as `<input>`. 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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 20, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
These methods are no longer used so they can safely be removed.

PR Close angular#38126
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…elements (angular#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:

- `<div><br></div>`, the `<br>` is implicitly closed when parsing `</div>`,
  because `<br>` is a void element.
- `<div><p></div>`, the `<p>` is implicitly closed when parsing `</div>`,
  as `<p>` is allowed to be closed by the closing of its parent element.
- `<ul><li>A <li>B</ul>`, the first `<li>` is implicitly closed when parsing
  the second `<li>`, whereas the second `<li>` would be implicitly closed when
  parsing the `</ul>`.

In all the cases above the parsed structure would be correct, however the source
span of the closing `</div>` 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 `<input/>`
and implicitly closed elements such as `<input>`. The former does have an end
source span (identical to its start source span) whereas the latter does not.

Fixes angular#36118
Resolves FW-2004

PR Close angular#38126
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes freq1: low target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HtmlParser incorrectly reports end tags (@angular/compiler)
5 participants