Skip to content

refactor(parser): Highlight special close-implies-open logic#1047

Merged
fb55 merged 1 commit intofb55:masterfrom
vassudanagunta:parser-p-br-special
Dec 17, 2021
Merged

refactor(parser): Highlight special close-implies-open logic#1047
fb55 merged 1 commit intofb55:masterfrom
vassudanagunta:parser-p-br-special

Conversation

@vassudanagunta
Copy link
Contributor

@vassudanagunta vassudanagunta commented Dec 14, 2021

Only </p> and </br> close tags result in implied open tags:

This commit clarifies this in the existing comments. It also
makes the special case nature of this more apparent by using
the literal values 'p' and 'br' instead of the variable name,
which is proven to have these values. This is less obfuscating.

Only </p> and </br> close tags result in implied open tags:
- [`p`](https://github.com/fb55/htmlparser2/blob/6445c32b05dc070049eeaaf201bfc123daca3d37/src/Parser.ts#L336)
- [`br`](https://github.com/fb55/htmlparser2/blob/6445c32b05dc070049eeaaf201bfc123daca3d37/src/Parser.ts#L340) tags.

This commit clarifies this in the existing comments. It also
makes the special case nature of this more apparent by using
the literal values 'p' and 'br' instead of the variable `name`,
which is proven to have these values. This is less obfuscating.
@vassudanagunta vassudanagunta changed the title Highlight special onclosetag logic for p and br Highlight special close-implies-open logic for p and br Dec 14, 2021
@vassudanagunta
Copy link
Contributor Author

Question: is there a reason that only p and br behave this way? Do HTML5 compliant parsers do this for and only for these two tags?

@fb55 fb55 changed the title Highlight special close-implies-open logic for p and br refactor(parser): Highlight special close-implies-open logic Dec 17, 2021
@fb55 fb55 enabled auto-merge (squash) December 17, 2021 14:54
@fb55 fb55 merged commit 99be0df into fb55:master Dec 17, 2021
@fb55
Copy link
Owner

fb55 commented Dec 17, 2021

Hi @vassudanagunta, thanks for this PR! This logic was changed in #52, which includes some explanations.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1580382724

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.46%

Totals Coverage Status
Change from base Build 1580203836: 0.0%
Covered Lines: 737
Relevant Lines: 741

💛 - Coveralls

@vassudanagunta vassudanagunta deleted the parser-p-br-special branch December 17, 2021 22:35
@vassudanagunta
Copy link
Contributor Author

Ok, cool, thanks. I'm actually looking at the same spec issue 52 refers to to figure out the proper behavior for something that i think htmlparser2 gets wrong... i'll share it with you as a WIP PR so that I can demonstrate with test cases and we can discuss whether you agree.

@fb55
Copy link
Owner

fb55 commented Dec 18, 2021

If you don't want to go through the entire spec, one good place to look is parse5's parser implementation. _insertFakeElement is only called for br and p end tags.

@vassudanagunta
Copy link
Contributor Author

I meant something else unrelated to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants