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)
Tracking
()
People
(Reporter: gsato, Assigned: bzbarsky)
References
()
Details
(4 keywords)
Attachments
(6 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
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>
Updated•10 years ago
|
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
Not sure what's going on here, but $URL works in Gecko 34.
Flags: needinfo?(Ms2ger) → needinfo?(annevk)
Keywords: regression
Updated•10 years ago
|
Keywords: regressionwindow-wanted
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)
Keywords: regressionwindow-wanted
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 ?
Assignee | ||
Comment 8•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
FWIW, I agree with Boris.
Comment 10•10 years ago
|
||
[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-firefox38:
--- → affected
tracking-firefox38:
--- → ?
Updated•10 years ago
|
Flags: needinfo?(annevk)
Updated•10 years ago
|
Flags: needinfo?(anujagarwal464)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Comment 14•10 years ago
|
||
[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+.
status-firefox40:
--- → ?
status-firefox41:
--- → ?
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
Flags: needinfo?(bugs)
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
I think we need to change the spec. The question is to what.
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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.
Assignee | ||
Comment 21•10 years ago
|
||
Might need to back out bug 1070015 too.
Comment 22•9 years ago
|
||
@smaug: before Bug 1075702 we were having following issues:
1. https://bugzilla.mozilla.org/show_bug.cgi?id=1061234#c0
2. https://bugzilla.mozilla.org/show_bug.cgi?id=1060938#c0
Flags: needinfo?(anujagarwal464)
Comment 23•9 years ago
|
||
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.
Comment 25•9 years ago
|
||
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...
"case-folding" :)
Comment 28•9 years ago
|
||
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.
Comment 29•9 years ago
|
||
(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).
Comment 30•9 years ago
|
||
Arkadiusz, as far as I can tell other browsers already do exactly that. But yes, I should have mentioned that.
Comment 31•9 years ago
|
||
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).
Comment 32•9 years ago
|
||
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...
Comment 33•9 years ago
|
||
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
Comment 34•9 years ago
|
||
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.)
Comment 35•9 years ago
|
||
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.
Comment 36•9 years ago
|
||
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.
Comment 37•9 years ago
|
||
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?
Comment 38•9 years ago
|
||
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.
Comment 39•9 years ago
|
||
(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.
Comment 40•9 years ago
|
||
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).
Assignee | ||
Comment 41•9 years ago
|
||
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.
Comment 42•9 years ago
|
||
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 | ||
Comment 43•9 years ago
|
||
Attachment #8621299 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 44•9 years ago
|
||
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-
Assignee | ||
Comment 45•9 years ago
|
||
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)
Comment 46•9 years ago
|
||
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()?
Comment 47•9 years ago
|
||
<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>
Assignee | ||
Comment 48•9 years ago
|
||
> 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...
Comment 49•9 years ago
|
||
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?
Comment 50•9 years ago
|
||
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)
Comment 51•9 years ago
|
||
(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...
Assignee | ||
Comment 52•9 years ago
|
||
> 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)
Comment 53•9 years ago
|
||
(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)
Comment 54•9 years ago
|
||
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.
Assignee | ||
Comment 57•9 years ago
|
||
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)
Assignee | ||
Comment 58•9 years ago
|
||
Attachment #8622676 -
Flags: review?(bugs)
Assignee | ||
Comment 59•9 years ago
|
||
Attachment #8622677 -
Flags: review?(bugs)
Assignee | ||
Comment 60•9 years ago
|
||
Updated•9 years ago
|
Attachment #8622676 -
Flags: review?(bugs) → review+
Updated•9 years ago
|
Attachment #8622677 -
Flags: review?(bugs) → review+
Comment 61•9 years ago
|
||
Assignee | ||
Comment 62•9 years ago
|
||
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
Comment 64•9 years ago
|
||
This bug is now closed, so I'll follow up on Chrome use counters in bug 1175031.
Flags: needinfo?(philipj)
Comment 65•9 years ago
|
||
I guess 40 is affected too.
Comment 66•9 years ago
|
||
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+
Comment 67•9 years ago
|
||
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+
Assignee | ||
Comment 69•9 years ago
|
||
Oh, good point.
Assignee | ||
Comment 70•9 years ago
|
||
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?
Comment 71•9 years ago
|
||
Flags: in-testsuite+
Comment 72•9 years ago
|
||
status-firefox-esr38:
--- → affected
tracking-firefox-esr38:
--- → ?
Comment 73•9 years ago
|
||
http://logs.glob.uno/?c=freenode%23whatwg&s=18%20Jun%202015&e=18%20Jun%202015#c951755
sounds rather good to me.
Assignee | ||
Comment 75•9 years ago
|
||
> 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?
Comment 76•9 years ago
|
||
Maybe as a plan B, if #1 doesn't work
Comment 77•9 years ago
|
||
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.
Assignee | ||
Comment 78•9 years ago
|
||
> 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);
Comment 79•9 years ago
|
||
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.
Assignee | ||
Comment 80•9 years ago
|
||
> 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.
Comment 81•9 years ago
|
||
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 83•9 years ago
|
||
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+
Comment 84•9 years ago
|
||
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)
Comment 85•9 years ago
|
||
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)
Assignee | ||
Comment 86•9 years ago
|
||
Flags: needinfo?(bzbarsky)
relnote-firefox:
? → ---
Updated•9 years ago
|
Keywords: dev-doc-needed,
site-compat
Adding a tracking flag, this is already fixed in ESR 38.
Comment 88•9 years ago
|
||
Correcting ESR tracking flag.
Comment 89•9 years ago
|
||
Posted the site compatibility doc for record: https://www.fxsitecompat.com/en-US/docs/2015/getattribute-returns-null-for-attributes-created-with-setattributenode-and-containing-upper-case-characters-in-name/
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•