13 years ago
Assignee: nobody → mrbkap
Component: General → HTML: Parser
Product: Firefox → Core
QA Contact: general → parser
Version: unspecified → Trunk
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Priority: -- → P4
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
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.
Status: NEW → ASSIGNED
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.
Attachment #201898 - Attachment is obsolete: true
Created attachment 202115 [details] [diff] [review] Fixed patch This patch seems to provide the wanted compatibility.
Attachment #202115 - Flags: review?(bzbarsky)
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
Attachment #202115 - Flags: review?(bzbarsky) → review+
Attachment #202115 - Flags: superreview?(jst)
Comment on attachment 202115 [details] [diff] [review] Fixed patch sr=jst
Attachment #202115 - Flags: superreview?(jst) → superreview+
Fix checked into trunk. I'll file a new bug on our potentially allowing NULs in attribute keys in a second.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Depends on: 315933
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.
Keywords: fixed18.104.22.168, fixed22.214.171.124
Using provided testcase confirmed existence of bug in 126.96.36.199. Verified for 188.8.131.52pre and 184.108.40.206pre. All PASS for testcase. Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:220.127.116.11pre) Gecko/2007011103 BonEcho/18.104.22.168pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:22.214.171.124pre) Gecko/20070111 Firefox/126.96.36.199pre
Keywords: fixed188.8.131.52, fixed184.108.40.206 → verified220.127.116.11, verified18.104.22.168
You need to log in before you can comment on or make changes to this bug.