xml5ever: compare prefix and local name when looking for duplicate attributes - #780
Open
FadeHack wants to merge 1 commit into
Open
xml5ever: compare prefix and local name when looking for duplicate attributes#780FadeHack wants to merge 1 commit into
FadeHack wants to merge 1 commit into
Conversation
The duplicate check compared the raw attribute name we had just read against the local names of the attributes already on the tag. The stored ones have been through process_qname by then, so xml:lang is sitting there as prefix "xml" with local name "lang", and reading a plain lang right after it matched and got rejected as a duplicate. That is why it was order dependent. Writing lang first and xml:lang second was fine, because the raw name "xml:lang" never matches the local name "lang". Namespaces in XML section 6.3 says attributes are the same only when their expanded names are the same, and xml:lang and lang have different expanded names, so both orderings should parse cleanly. This also shows up on the WPT test table-align-float.xhtml, whose root element carries both. So process the name first and compare the prefix along with the local name. Real duplicates, including two attributes with the same prefix, are still reported. The tokenizer has no namespace bindings yet, they are resolved later in XmlTreeBuilder::bind_qname, so two different prefixes bound to the same namespace still slip through here. That is a separate thing and it needs to be handled in the tree builder. Fixes servo#775
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #775
<root xml:lang="en" lang="en"/>was reported asDuplicate attribute, but<root lang="en" xml:lang="en"/>parsed fine.Why
finish_attributecompared the raw attribute name it had just read against the local names of the attributes already on the tag:The stored attributes have been through
process_qnameby then, soxml:langis sitting there as prefixxmlwith local namelang. Reading a plainlangright after it matches that local name and gets rejected.That is also where the order dependence comes from. With
langfirst, the raw namexml:langis compared against the local namelang, they do not match, and both attributes survive.Namespaces in XML section 6.3 says two attributes are the same only when their expanded names are the same, and
xml:langandlangdo not have the same expanded name, so both orderings should parse cleanly. It also affects the WPT testhtml/rendering/non-replaced-elements/tables/table-align-float.xhtml, whose root element carries both.The fix
Run
process_qnamefirst, then compare the prefix along with the local name. Real duplicates still get reported, including two attributes that share a prefix.One thing worth calling out: the tokenizer has no namespace bindings at this point, they are resolved later in
XmlTreeBuilder::bind_qname, so two different prefixes bound to the same namespace URI with the same local name are still not caught. That is a genuine duplicate under section 6.3 but it cannot be detected here without the bindings, so it belongs in the tree builder. I left it alone rather than widening this PR, and I am happy to look at it separately if you want.Testing
Added unit tests in
xml5ever/src/tokenizer/mod.rsfor both orderings from the issue, for two distinct prefixes sharing a local name, and for real duplicates with and without a prefix. The first two fail without the change.cargo test --all,cargo fmt --all -- --checkandcargo clippy --all-features --all-targetsare clean.