Closed Bug 1165851 Opened 10 years ago Closed 9 years ago

document.createAttribute can not get their own Added attributes

Categories

(Core :: DOM: Core & HTML, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 - wontfix
firefox38.0.5 --- wontfix
firefox39 + fixed
firefox40 + fixed
firefox41 + fixed
firefox-esr38 39+ fixed

People

(Reporter: gsato, Assigned: bzbarsky)

References

()

Details

(4 keywords)

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.152 Safari/537.36 Steps to reproduce: <html> <head> <script type="text/javascript" src="jquery-1.5.1.min.js"></script> <script type="text/javascript"> $(document).ready(function(){ $("#btn").unbind("click").click(hoge); var select = document.getElementById("cmb"); for(var i=0; i < 5; i++){ //this method not work(TEST attribute is undefined) opt = notWorkScript(i); select.appendChild(opt); } $("#cmb").unbind("change").change(function () { alert($("#cmb option:selected").attr("TEST")); }); }); /* * this method work firefox,ie,chrome */ function workScript(cnt){ var opt = document.createElement("OPTION"); opt.text = "text" + cnt; opt.value = "val" + cnt; opt.setAttribute("TEST", "TEST" + cnt); return opt; } /** * this method not work on firefox * chrome,ie work! why? */ function notWorkScript(cnt){ var opt = document.createElement("OPTION"); opt.text = "text" + cnt; opt.value = "val" + cnt; var attr_node = document.createAttribute("TEST"); attr_node.nodeValue = "TEST" + cnt; opt.attributes.setNamedItem(attr_node); return opt; } function hoge() { alert($("#cmb option:selected").attr("value")); } </script> </head> <body> <form name="mainform"> <select name="cmb" id="cmb" > <option name="op1" id="op1" value="1" />hoge1 </select> <br><br> <input type="button" name="btn" id="btn" value="Click!!"><br> </form> </body> </html>
Attached file 1165851.html (reporter's testcase) (deleted) —
Component: Untriaged → DOM
Keywords: testcase
Product: Firefox → Core
So, if the attribute node is created using lowercase "test", then it works. Per spec https://dom.spec.whatwg.org/#dom-document-createattributelocalname "Note: This method does not have its input converted to ASCII lowercase." and getAttribute does convert to lowercase. https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name It is not clear to me what Blink (or IE) does here. I think attribute node handling works per spec but getAttribute() finds also uppercase attributes or something.
IIRC, Ms2ger has played with DOM Attributes. Do you recall the reasoning for Blinks behavior? Reporter, as of now, this is a bug in IE and Chrome. If you want spec to be changed, please file a spec bug.
Flags: needinfo?(Ms2ger)
Not sure what's going on here, but $URL works in Gecko 34.
Flags: needinfo?(Ms2ger) → needinfo?(annevk)
Keywords: regression
good=2015-01-16 bad=2015-01-17 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cac6192956ab&tochange=369a8f14ccf8 Suspected bug: Anuj Agarwal — Bug 1075702 - Fixed implementation of Element.setAttributeNode(). r=bz
Blocks: 1075702
Component: DOM → DOM: Core & HTML
Flags: needinfo?(anujagarwal464)
It was confirmed that it does not work at Firefox 38.0.1. But, 37.0.2 did work. Latest version has been fixed in ?
It regressed in FF38, not FF37.
We aligned closer with the spec in Firefox 38. The old behavior had all sorts of problems that the change fixed. The old behavior, and the behavior in other browsers, does case-insensitive stuff in various ad-hoc ways that don't really make sense when you look at them as a whole... If there is a serious web compat issue here we should figure out what that is and get the spec changed, but if not it's better to just get the other browsers to also follow the spec.
FWIW, I agree with Boris.
[Tracking Requested - why for this release]: we should keep an eye out for serious web compat regressions and escalate Tech Evangelism issues accordingly (see above).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(annevk)
Flags: needinfo?(anujagarwal464)
Anne, I'm not sure why you cleared the needinfo. This is a case of the spec not being web-compatible, no?
Flags: needinfo?(annevk)
[Tracking Requested - why for this release]: Too late for 38. Request tracking for 39 to make sure Liz sees it. I guess 39 is affected.
smaug is this something you are taking on or can help find an owner for it? I'll track this for 39 but unless it is fixed soon this is more realistic to aim at 40+.
Flags: needinfo?(bugs)
This is a regression from bug 1075702, which made us to follow the spec. But perhaps anuj or bz have ideas for this (and anne for the spec). Do we need to back out Bug 1075702 and change the spec?
Flags: needinfo?(bugs) → needinfo?(anujagarwal464)
I think we need to change the spec. The question is to what.
@smaug: I don't have any ideas for this.
Flags: needinfo?(anujagarwal464)
Anuj, do we have clear picture of what other browsers do and how they are against the current spec? Was our behavior closer to other browsers before Bug 1075702? If so, I think we should back it out and get the spec fixed.
Flags: needinfo?(anujagarwal464)
For this particular case our behavior was closer to other browsers. As for what they do, when we tested there were all sorts of incompatibilities... But yes, it might be best to just back out bug 1075702 and the various patches we landed on top of it (bug 1060938 and bug 1061234 at least) for now and reevaluate once we figure out what the spec should say. I think we should do that for 39, in fact.
Might need to back out bug 1070015 too.
As far as I can tell https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name needs to be case-insensitive to match Chrome, Safari, and Internet Explorer, which seems really unfortunate and all kinds of wrong. Both getAttribute() and attributes.getNamedItem() follow that same code path and both exhibit that case-insensitive behavior in those browsers (matching attributes with uppercase names, even namespaced ones). In Chrome and Safari this does not happen for XML documents. Did not test Internet Explorer and XML. (I also found that document.createAttribute() can create attributes with a non-null prefix in Chrome and Safari, but since Internet Explorer does not do that and matches Firefox we can leave that alone I think.) Based on the above my suggested fix would be that in DOM we modify https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name by removing step 1 and performing an ASCII case-insensitive match for attributes on HTML elements in HTML documents. This would fix both getAttribute() and attributes.getNamedItem() to behave like Chrome, Safari, and Internet Explorer.
Flags: needinfo?(annevk)
One option to consider is to always store and retrieve attributes in a case insensitive way. It seems like more places than not use case insensitive logic anyway given that both CSS and .getAttribute() does it. I think that would also result in faster matching logic in CSS since we can just case-fold at parse time and then do a case sensitive lookup using atoms.
Well, you need to store them as-is, but yes, retrieving them case-insensitively is what comment 23 suggests, scoped to where we do case-insensitive matching currently.
(In reply to Anne (:annevk) from comment #25) > Well, you need to store them as-is. Why? I guess to make AttributeNode.nodeName return the right thing? I wonder if we could get away with face-folding that too. But I guess not...
Right, localName, name, etc. all need to return the correct case. If we changed that we'd be the odd-one-out once more and undoubtedly get many compatibility issues.
(In reply to Anne (:annevk) from comment #23) > Based on the above my suggested fix would be that in DOM we modify > https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name by > removing step 1 and performing an ASCII case-insensitive match for > attributes on HTML elements in HTML documents. This would fix both > getAttribute() and attributes.getNamedItem() to behave like Chrome, Safari, > and Internet Explorer. But per DOM this algo is used by much more method than only getAttribute() and attributes.getNamedItem(), like Element.removeAttribute(), Element.setAttribute(), Element.setAttributeNode(), NamedNodeMap.removeNamedItem() and NamedNodeMap.setNamedItem() - maybe more. That change cover all this methods too in more way as now? It would be good to have at least two engines which do that (or plan to do that).
Arkadiusz, as far as I can tell other browsers already do exactly that. But yes, I should have mentioned that.
Probably no... Now last Firefox support all this attr stuff correct (what we have in DOM), this is consistent and makes sense, unfortunately we get compatibility problems. I try check how it looks like after you will change DOM and see that it will not match in every case (compare only Firefox and Chrome because IE has more wrong behaviour even for setAttribute()). This change will not mach for Element.removeAttribute() in Chrome because for this method Chrome works just fine. <script> p = document.createElement("p"); p.setAttributeNS("www.test1.com", "ID", "Test1"); p.setAttribute("id", "Test2"); var allAttr = p.attributes; document.write("Before removeAttribute('ID'):<br>"); for (var i = 0; i < allAttr.length; i++){ var attr = allAttr[i]; document.write(attr.namespaceURI + " , " + attr.prefix + " , " + attr.name + " , " + attr.value + "<br>"); } /* Te same for Firefox and Chrome Before removeAttribute('ID'): www.test1.com , null , ID , Test1 null , null , id , Test2 */ p.removeAttribute("ID"); document.write("<br>After removeAttribute('ID'):<br>"); for (var i = 0; i < allAttr.length; i++){ var attr = allAttr[i]; document.write(attr.namespaceURI + " , " + attr.prefix + " , " + attr.name + " , " + attr.value); } /* Te same for Firefox and Chrome After removeAttribute('ID'): www.test1.com , null , ID , Test1 */ </script> I checked only one method. Looks like Chrome for some methods may be case-insensitive (like getAttribute) but for some other methods (like removeAttribute) converts the passing argument to ASCII lowercase. So common definitions for all attr methods may not be feasible, each requires detailed examination and take a sensible decision (if compatibility is of prime importance).
One more thing about IE for comparing: Before removeAttribute('ID'): www.test1.com , null , ID , Test1 www.test11.com , null , id , Test11 After removeAttribute('ID'): www.test1.com , null , ID , Test1 So IE in this case is case-insensitive, what a mess...
Sorry for spam but copy result from wrong browser, that is for IE: Before removeAttribute('ID'): www.test1.com , null , ID , Test1 www.test11.com , null , id , Test11 After removeAttribute('ID'): www.test11.com , null , id , Test11
The alternative would be to lowercase the input to document.createAttribute() for HTML documents. It would also fix these new bugs though we might get new reports... Lowercasing document.createAttribute() input is a more sane model as sicking was alluding to as well. (I do think that if we start doing case-insensitive matching for attributes on HTML elements in HTML documents rather than just a lowercase the input before matching, we should do so consistently and get other browsers aligned.)
Hmm interesting, lowercase the input for document.createAttribute() should fix https://bugzilla.mozilla.org/show_bug.cgi?id=1167067#c11, but changing behaviour for argument may be risk. But in other way usage document.createAttribute() is not so big (only 0.20): https://www.chromestatus.com/metrics/feature/timeline/popularity/111 And wondering if changing lowercase input to case-insensitive matching in HTML doc for all attr methods again not break something.
This the current state the relevant APIs on Document and Element in Blink: Document.createAttribute(localName) is just an alias of Document.createAttributeNS(null, localName), which doesn't lowercase localName. Element.getAttribute(name) does a case-insensitive lookup for HTML elements in HTML documents, and case-sensitive otherwise. Element.setAttribute(name, value) lowercases name for HTML elements in HTML documents. Even if there was an attribute matching the original case a new one is created. Element.removeAttribute(name) lowercases name for HTML elements in HTML documents. Even if there was an attribute matching the original case it's not removed. (Unless the original and lowercased string is the same, of course.) Element.hasAttribute(name) lowercases name and then does a case-sensitive lookup for that name. Element.getAttributeNode(name) does a case-insensitive lookup for HTML elements in HTML documents, and case-sensitive otherwise. This is from looking at the code, perhaps I overlooked something. It's a curious mix of lowercasing+case-sensitive lookup and case-insensitive lookups.
Hmm can anyone explain what exactly make problem in Firefox because topic of this bug is wrong. Document.createAttribute() and NamedNodeMap.setNamedItem() are correct here because all browser (Firefox, Chrome and IE) product the same code for example in comment0: <select name="cmb" id="cmb"> <option name="op1" id="op1" value="1">hoge1</option> <option TEST="TEST0" value="val0">text0</option> <option TEST="TEST1" value="val1">text1</option> <option TEST="TEST2" value="val2">text2</option> <option TEST="TEST3" value="val3">text3</option> <option TEST="TEST4" value="val4">text4</option> </select> Problem is jQuery and how it try get this attr?
See my bug report - https://bugzilla.mozilla.org/show_bug.cgi?id=1169645#c5. There is an attachment that should help you along with my better description. Embedding the attribute in the HTML markup does not create the problem. It is caused by setting the attribute the old, deprecated way. While support for createAttribute() is slated to be pulled (as evidenced by deprecation), it is still supposed to be supported. Hope this helps you.
(In reply to John R. Scott from comment #38) > support for createAttribute() is slated to be pulled (as evidenced by deprecation) I don't think this will ever happen. It looked hopeful for a while to radically simplify Attr-related things. Some of the less-used bits were removed from Blink, but when it became clear that not all of it could be killed that was revert. The Attr-related thing with highest usage is getAttributeNode(): https://www.chromestatus.com/metrics/feature/timeline/popularity/107 That being said, *some* Attr-related simplification is still possible.
Looks like all this new opened bugs around attributes apply only to Element.getAttribute() and Element.getNamedItem(). After fixing some old attr bugs (what happend for Firefox 38) I don't see any report for other methods. Maybe fixing only this two case in spec will sufficient? Of course idea from comment34 also looks interesting, just changing one method (but maybe more risk).
So as I see it, we have three possible options: 1) Do what comment 34 suggests: have document.createAttribute lower-case its argument in an HTML document. 2) Do what Blink currently implements: document.createAttribute preserves case, getAttribute/getAttributeNode do ASCII-case-insensitive matching, hasAttribute/removeAttribute/setAttribute lower-case the argument and then do case-sensitive matching. 3) Have document.createAttribute preserve case and getAttribute/getAttributeNode/hasAttribute/removeAttribute/setAttribute do case-insensitive matching. My personal preference, honestly, is for option 1. It's simple and has much better performance characteristics than options 2/3, imo.
FWIW, I also think option 1 sounds good. If it works out and the spec changes to match, I'd like to attempt aligning Blink with that.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8621299 [details] [diff] [review] Make it possible to getAttribute a mixed-case attribute name even if the attr was created with createAttribute Well, this breaks various Attr usage in svg, right? (svg in html documents). I'm rather worried about that.
Attachment #8621299 - Flags: review?(bugs) → review-
It does, yes. I was hoping that was a rare case, since SVG in HTML directly is reasonably rare itself. So which of the options from comment 41 do you prefer, then?
Flags: needinfo?(bugs)
I cannot really find examples on GitHub that use createAttribute() for any of https://html.spec.whatwg.org/multipage/syntax.html#adjust-svg-attributes (there are examples of it being used for xmlns and xmlns:xlink, which won't work...). Was our old behavior to only lowercase the attribute name when it was set on an HTML element through setAttributeNode()/setNamedItem()?
<script> var a1 = document.createAttribute("aa"); a1.nodeValue = "lowercase"; var a2 = document.createAttribute("AA"); a2.nodeValue = "UPPERCASE"; document.documentElement.setAttributeNode(a1); document.documentElement.setAttributeNode(a2); alert(document.documentElement.getAttributeNS("", "aa")); // was: UPPERCASE now (and per spec): null alert(document.documentElement.getAttributeNS("", "AA")); // was: null now (and per spec): UPPERCASE </script>
> Was our old behavior to only lowercase the attribute name when it was set on an HTML > element through setAttributeNode()/setNamedItem()? Yes. setAttributeNode just called setNamedItem, and setNamedItem would lowercase the name if the element is HTML in an HTML document. Of course it would not lowercase the name in the Attr itself, just the name under which the value was stored in the element. So you would have some fun mismatches after that point. It really was a pretty weird behavior...
Why other attr's methods don't break SVG here in HTML document, all methods that getting or setting attr that convert argument to lower case (I mean this SVG attr which have upper/lower case what we see in HTML spec)? Insert hook for setting methods to use "adjust SVG attributes" from HTML would give any positive result?
I think the behavior explained in comment 48 would be better than IsHTMLDoc() in createAttribute() The attr methods on elements themselves don't break SVG-in-HTML since we explicitly check IsInHTMLDocument() && IsHTMLElement() before doing the lowercasing. But from comment 41 I think (2), assuming the automatic lowercasing for setAttribute() etc happens only on HTML elements in HTML document. Yes it can be slower, but the slow path needs to be taken only if we know there are mixed case attributes. So, no in HTML unless someone explicitly adds Attr nodes. We can have a flag in elements to indicate possible mixed case attributes. (ugly API anyhow)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #50) > The attr methods on elements themselves don't break SVG-in-HTML since we > explicitly check > IsInHTMLDocument() && IsHTMLElement() before doing the lowercasing. > This information should not be included in DOM specification? We always see checking only HTML doc...
> I think the behavior explained in comment 48 would be better Complete with the mismatch between the name in the attr and the name it's stored under in the element? That caused problems with attr removal and violated various other invariants (e.g. we could easily end up with multiple Attrs in the hashtable that had names differing in case only, but only one of them was in the element's storage, so we could remove one and then hasAttribute would say false but the named item lookup on the attribute map would succeed). Or did you mean mutating the Attr's name on setAttributeNode/setNamedItem?
Flags: needinfo?(bugs)
(In reply to Not doing reviews right now from comment #52) > > I think the behavior explained in comment 48 would be better > > Complete with the mismatch between the name in the attr and the name it's > stored under in the element? That caused problems with attr removal and > violated various other invariants (e.g. we could easily end up with multiple > Attrs in the hashtable that had names differing in case only, but only one > of them was in the element's storage, so we could remove one and then > hasAttribute would say false but the named item lookup on the attribute map > would succeed). > > Or did you mean mutating the Attr's name on setAttributeNode/setNamedItem? I didn't. creating/setting same attribute using different casing (can one say that way, lower/upper/camelCasing)[1] sounds more edge case to me than creating an Attr with camelCase and use it in SVG. But of course I don't have data which one is actually happening more often. [1]It would be great if would still store only one of those attrs in the hashtable, the same which is in the element's storage.
Flags: needinfo?(bugs)
Unless it breaks something, it seems unproblematic to lowercase the name in createAttribute(name). Deliberate uses could resort to createAttributeNS(null, name), or setAttributeNS(null, name, value) instead.
So Olli and I talked this over. The current plan is: 1) Back out bug 1075702 and dependencies (except bug 1070015) for now, on trunk, aurora, and beta. 2) Add telemetry for the situation where an Attr is created via createAttribute with a non-lowercase name from an HTML document and then passed to setAttributeNode on a non-HTML element. 3) Depending on the outcome of that telemetry measurement, decide which of the options in comment 41 to pursue. Philip, would it be possible for Chrome to also add a use counter like the one I describe in #2 there? I'll prepare a patch doing #1 in this bug and file followups for #2 and #3.
Flags: needinfo?(philipj)
Blocks: 1175031
Attachment #8622676 - Flags: review?(bugs) → review+
Attachment #8622677 - Flags: review?(bugs) → review+
Comment on attachment 8622680 [details] [diff] [review] Rollup version for aurora and beta Approval Request Comment [Feature/regressing bug #]: Bug 1075702 [User impact if declined]: Some websites are broken [Describe test coverage new/current, TreeHerder]: There are tests for all this stuff; we're just changing the desired behavior. [Risks and why]: Risk is low: this is just a backout of bug 1075702 so gets us back to the behavior we had in Firefox 37. [String/UUID change made/needed]: None.
Attachment #8622680 - Flags: approval-mozilla-beta?
Attachment #8622680 - Flags: approval-mozilla-aurora?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
This bug is now closed, so I'll follow up on Chrome use counters in bug 1175031.
Flags: needinfo?(philipj)
Comment on attachment 8622680 [details] [diff] [review] Rollup version for aurora and beta Fix a regression, taking it for 40. Liz will make the call for beta.
Attachment #8622680 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It would also be good to get a fix for the 38 ESR series, if at all possible (see bug 1167067).
Comment on attachment 8622680 [details] [diff] [review] Rollup version for aurora and beta Approved for uplift to beta as well.
Attachment #8622680 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch ESR38 version of patch (deleted) — Splinter Review
Oh, good point.
Comment on attachment 8623829 [details] [diff] [review] ESR38 version of patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Functionality regression from Firefox 37. User impact if declined: Some sites are broken. Fix Landed on Version: 41, but being backported to 40 and 39. Risk to taking this patch (and alternatives if risky): Should be pretty low risk: it's just backing out back to the state we had in 37. The alternative is to do nothing. String or UUID changes made by this patch: None. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8623829 - Flags: approval-mozilla-esr38?
> http://logs.glob.uno/?c=freenode%23whatwg&s=18%20Jun%202015&e=18%20Jun%202015#c951755 I could live with that. Is that being proposed as plan A, or as plan B if lowercasing in createAttribute does not pan out?
Maybe as a plan B, if #1 doesn't work
i don't know how you internaly debate this issue. Knowing that the specs advise you to handle tagnames lowercase you cannot expect that. As Javascript allows you for readabilty to use "aA" notations even attributes should be aware of this. Means ... if you put "aA" in you await an "aA" result. I see alot sites currently not responding the way it should be using FF38. Thats concerning.
> Means ... if you put "aA" in you await an "aA" result. Except that's not how it works in general. Simple example: var div = document.createElement("div"); div.setAttribute("aA", "hey"); alert(div.attributes[0].name);
Well yea. Its converted to lower case. Then why is getAttribute("aA") not gettin "aa"? You can argue alot of time while thousands of applications will go down the sink. Remember ... FF37 downwards was ok with that. Breakin up with everything only because specs are tellin you wans't the brightest idea anyways.
> Then why is getAttribute("aA") not gettin "aa"? It returns "hey", because it lowercases the attribute name. I'm really not quite sure what you're arguing about here. This bug is fixed. The fix is shipping in Firefox 39, in less than two weeks. Everyone agrees that your testcase from bug 1175914 should work as you expect. What's not agreed on is how some other edge cases which browsers _don't_ currently all handle the same way should work.
Boris ... all fine. I absolutely love the project. Thats why i incorp. my time into filing these cases. I soly wanted to point out that we're not about specs as the public user tellin ya that its site isn't workin. Thats all. Thanks guys
Smaug, if you think this should have a release note can you help with wording it?
relnote-firefox: --- → ?
Flags: needinfo?(bugs)
Comment on attachment 8623829 [details] [diff] [review] ESR38 version of patch Make sense to take it in esr 38 as it might impact corporate env.
Attachment #8623829 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
I attempted to land this on esr38 but hit bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=14276&repo=mozilla-esr38 Please land this on esr38 ASAP once fixed as RelMan is trying to spin the 38.1 build as soon as they can.
Flags: needinfo?(bzbarsky)
Liz, I'm not sure this is worth for a release note. This is about fixing an issue by backing out the problematic change.
Flags: needinfo?(bugs)
Adding a tracking flag, this is already fixed in ESR 38.
Correcting ESR tracking flag.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: