12 years ago
Yeah, might as well sync up with IE. It might be worth it to figure out if IE doesn't allow various characters in attribute keys (note: this function was added for bug 7548).
IMHO we should do as little "fixup" as possible of the markup. Dropping ending '/' seems like a good idea to ease xhtml migration. Other then that I think we should not strip any more characters then what IE does.
I did some testing; IE treats / as a terminator for attributes (which means that in cases like <foo f/g>, it treats foo as having 2 attributes, f and g). Do we want to emulate that?
Ugh, our current behavour is less then ideal. For <div f/b> we create an attribute named 'f/b'. IMHO we should never create attributes or elements with names containing illigal characters. Not sure what qualifies as legal characters in HTML/SGML, but in XML it's pretty clearly defined http://www.w3.org/TR/2004/REC-xml11-20040204/#NT-Name Ideally we should drop other charaters, so parsing <div f/b> as <div f b> or something similar seems good to me. Very imporantly, we should never put an ascii-null inside a element or attribute name. I suspect that that could cause security issues if we compare strings and atoms.
Created attachment 201898 [details] [diff] [review] Potential patch This needs to be pounded on, hard. I think it should do the right thing, though.
Created attachment 202114 [details] testcases Running these testcases should be self-explanatory. Are there any cases that I'm missing? Obsoleting attachment 201898 [details] [diff] [review] in preparation for a better patch.
Created attachment 202115 [details] [diff] [review] Fixed patch This patch seems to provide the wanted compatibility.
Comment on attachment 202115 [details] [diff] [review] Fixed patch >Index: parser/htmlparser/src/nsHTMLTokens.cpp >+ NS_ASSERTION(aChar == kApostrophe|| aChar == kQuote || aChar == kForwardSlash, Space before first ||, please. >@@ -2029,39 +1994,41 @@ nsresult CAttributeToken::Consume(PRUnic >+ result = aScanner.SkipOver(aChar); //strip quote. quote or slash r=bzbarsky
Comment on attachment 202115 [details] [diff] [review] Fixed patch sr=jst
Fix checked into trunk. I'll file a new bug on our potentially allowing NULs in attribute keys in a second.
Created attachment 250215 [details] [diff] [review] Patch for the 1.8 branches This patch is simply the result of CVS joins. It applies cleanly to the 1.8 and 1.8.0 branches.
Wanted for compatibility with other browser's parsing of attributes; being non-standard here is complicating web apps trying to filter content.
Comment on attachment 250215 [details] [diff] [review] Patch for the 1.8 branches Approved for both branches, a=jay for drivers.
Fixed on the 1.8 branches.
Using provided testcase confirmed existence of bug in 22.214.171.124. Verified for 126.96.36.199pre and 188.8.131.52pre. All PASS for testcase. Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:184.108.40.206pre) Gecko/2007011103 BonEcho/220.127.116.11pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:18.104.22.168pre) Gecko/20070111 Firefox/22.214.171.124pre