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(tokenizer): Properly handle CR when peeking #715

Merged
merged 3 commits into from Oct 30, 2022
Merged

fix(tokenizer): Properly handle CR when peeking #715

merged 3 commits into from Oct 30, 2022

Conversation

fb55
Copy link
Collaborator

@fb55 fb55 commented Oct 27, 2022

Peeking didn't properly support carriage returns, which led the parser to hang in special tags.

Fixes #710

Peeking didn't properly support carriage returns, which led the parser to hang in special tags.

Fixes #710
Copy link
Collaborator

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Looks good, but the test seems a bit specific to how the tokenizer is implemented. Perhaps a reduced case from the original problem (#710 (comment)) can be added as a test for the parser as a whole?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0001%) to 99.251% when pulling 274f1a2 on fix-peek into bb0129f on master.

@fb55 fb55 merged commit e6f304c into master Oct 30, 2022
@fb55 fb55 deleted the fix-peek branch October 30, 2022 18:16
@fb55
Copy link
Collaborator Author

fb55 commented Oct 30, 2022

Thanks for the review! I've switched the test to a parser regression test, following the pattern we already had in place for similar issues.

@fb55 fb55 mentioned this pull request Nov 16, 2022
jmbpwtw pushed a commit to jmbpwtw/parse5 that referenced this pull request Feb 16, 2023
jmbpwtw pushed a commit to jmbpwtw/parse5 that referenced this pull request Feb 16, 2023
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.

CRLF inside </noscript> occurs eof-in-element-that-can-contain-only-text error
3 participants