Closed
Bug 314980
Opened 18 years ago
Closed 18 years ago
DOM element.getAttribute() method error (CAttributeToken::SanitizeKey())
Categories
(Core :: DOM: HTML Parser, defect, P4)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: atec_post, Assigned: mrbkap)
References
Details
(Keywords: verified1.8.0.10, verified1.8.1.2)
Attachments
(3 files, 1 obsolete file)
1.30 KB,
text/html
|
Details | |
10.79 KB,
patch
|
bzbarsky
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
11.33 KB,
patch
|
jay
:
approval1.8.1.2+
jay
:
approval1.8.0.10+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050920 Firefox/1.0.7 SUSE/1.0.7-0.1 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050920 Firefox/1.0.7 SUSE/1.0.7-0.1 The call to getAttribute() method in Javascript doesn't work properly. Reproducible: Always Steps to Reproduce: 1. Make an HTML page with an element's name attribute set to "main_" (i.e. <td main_="100"> ) 2. Try to get the attribute with getAttribute("main_") Actual Results: The call returns NULL Expected Results: Following the example (steps to reproduce) the call should return "main_" This is a cut from a post on netscape.public.mozilla.dom news. Message subject: DOM element.getAttribute() error? The next code is a cut from an HTML page: <tr parent_type="head_blind" id="tr_1012" main_="110"> <td class="folder_content"><img onclick="../showResults.do" height="1" src="../skin/xp/img/pixel_empty.gif" width="10"><img style="cursor: pointer;" border="0" src="../skin/xp/img/folder_off.gif"><span type_action="folder">Chiquito</span></td><td class="folder_content" width="1"><img style="cursor: pointer;" border="0" src="../skin/xp/img/arrow_single_up.gif"></td> </tr> I wanted to find all the "TR" nodes with the "main_" attribute with: //// function getElmWithAtt(elm, a, v) { var nList = f.contentDocument.getElementsByTagName(elm); var i=0; for (; i<nList.length; i++) { var m = nList[i]; if (m.getAttribute(a) != null) { if (m.getAttribute(a) == v) { break; } } } return nList[i]; } getElmWithAtt("TR", "main_", 110); //// But it doesn't work. It worked with: getElmWithAtt("TR", "main", 110);
Updated•18 years ago
|
Assignee: nobody → mrbkap
Component: General → HTML: Parser
Product: Firefox → Core
QA Contact: general → parser
Version: unspecified → Trunk
Assignee | ||
Comment 1•18 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).
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.
Assignee | ||
Comment 3•18 years ago
|
||
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.
Assignee | ||
Comment 5•18 years ago
|
||
This needs to be pounded on, hard. I think it should do the right thing, though.
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Attachment #202114 -
Attachment is patch: false
Attachment #202114 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 7•18 years ago
|
||
This patch seems to provide the wanted compatibility.
Attachment #202115 -
Flags: review?(bzbarsky)
![]() |
||
Comment 8•18 years ago
|
||
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+
Assignee | ||
Updated•18 years ago
|
Attachment #202115 -
Flags: superreview?(jst)
Comment 9•18 years ago
|
||
Comment on attachment 202115 [details] [diff] [review] Fixed patch sr=jst
Attachment #202115 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 10•18 years ago
|
||
Fix checked into trunk. I'll file a new bug on our potentially allowing NULs in attribute keys in a second.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•17 years ago
|
||
This patch is simply the result of CVS joins. It applies cleanly to the 1.8 and 1.8.0 branches.
Attachment #250215 -
Flags: approval1.8.1.2?
Attachment #250215 -
Flags: approval1.8.0.10?
Comment 12•17 years ago
|
||
Wanted for compatibility with other browser's parsing of attributes; being non-standard here is complicating web apps trying to filter content.
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Updated•17 years ago
|
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Comment 13•17 years ago
|
||
Comment on attachment 250215 [details] [diff] [review] Patch for the 1.8 branches Approved for both branches, a=jay for drivers.
Attachment #250215 -
Flags: approval1.8.1.2?
Attachment #250215 -
Flags: approval1.8.1.2+
Attachment #250215 -
Flags: approval1.8.0.10?
Attachment #250215 -
Flags: approval1.8.0.10+
Comment 15•17 years ago
|
||
Using provided testcase confirmed existence of bug in 2.0.0.1. Verified for 2.0.0.2pre and 1.5.0.10pre. All PASS for testcase. Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/2007011103 BonEcho/2.0.0.2pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10pre) Gecko/20070111 Firefox/1.5.0.10pre
You need to log in
before you can comment on or make changes to this bug.
Description
•