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 document type detection (limited-quirks mode) #305

Merged
merged 3 commits into from
Nov 6, 2019
Merged

Fix document type detection (limited-quirks mode) #305

merged 3 commits into from
Nov 6, 2019

Conversation

squidfunk
Copy link
Contributor

This PR fixes a bug with DOCTYPE detection, where the lowercased publicId will never match the limited-quirks document types with mixed case.

@inikulin
Copy link
Owner

inikulin commented Nov 5, 2019

@squidfunk Good catch, thanks! Though, seems like linting in CI is unhappy with line endings.

@squidfunk
Copy link
Contributor Author

Fixed, sorry for that.

@squidfunk
Copy link
Contributor Author

squidfunk commented Nov 5, 2019

BTW, I don't know if the spec changed since you implemented quirks-mode detection, but I think the prefixes are not entirely correct as they specify //en at the end. See this section of the spec which doesn't specify them. Also my testing with document.compatMode denotes that the language must not necessarily be en for correct quirks-mode detection.

@inikulin
Copy link
Owner

inikulin commented Nov 5, 2019

@squidfunk Thanks for yet another great catch. I've just looked through the spec commit history and it seems like it always was this way and it's a mistake on my side. I'm surprised it wasn't captured by html5lib-tests.

May I ask you to fix that in context of this PR as well and open a ticket in https://github.com/html5lib/html5lib-tests to add test cases for that?

@squidfunk
Copy link
Contributor Author

May I ask you to fix that in context of this PR as well and open a ticket in https://github.com/html5lib/html5lib-tests to add test cases for that?

Will do.

@squidfunk
Copy link
Contributor Author

I could combine some of the prefixes and add two missing - should now be spec-compliant.

@squidfunk
Copy link
Contributor Author

It would be really awesome if you could issue a bugfix release, as my downstream project pretty much depends on this fix :-)

@inikulin
Copy link
Owner

inikulin commented Nov 6, 2019

@squidfunk Great work, thank you!

It would be really awesome if you could issue a bugfix release, as my downstream project pretty much depends on this fix :-)

Will do =)

@inikulin inikulin merged commit 4855022 into inikulin:master Nov 6, 2019
@inikulin
Copy link
Owner

inikulin commented Nov 6, 2019

@squidfunk Published as v5.1.1

@squidfunk
Copy link
Contributor Author

Perfect, thanks! Happy to contribute.

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.

2 participants