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

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

Merged
merged 1 commit into from Dec 17, 2021

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.

None yet

3 participants