| Summary: | createAttribute/setAttributeNode does not properly normalize case | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | michael | ||||||||||||||||||||||||||||||||
| Component: | DOM | Assignee: | Arpita Bahuguna <arpitabahuguna> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
| Severity: | Minor | CC: | ap, arpitabahuguna, buildbot, bzbarsky, cmarcelo, commit-queue, darin, dglazkov, esprehn+autocc, kangil.han, kling, koivisto, morrita, ojan.autocc, rafaelw, rniwa, tkent, vivekg, webkit.review.bot | ||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | Mac (Intel) | ||||||||||||||||||||||||||||||||||
| OS: | OS X 10.7 | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
michael
2012-06-30 15:43:36 PDT
What does HTML5 say? (In reply to comment #1) > What does HTML5 say? The DOM spec specifies that, at least setAttribute should override an existing attribute if it has the same name, which presumably should be a case-insensitive comparison (http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/DOM3-Core.html#core-ID-887236154 http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/DOM3-Core.html#core-ID-5DFED1F0-h3), so running the below example on an image with an existing src attribute should update the src. Note that WebKit parses this properly: <img SRC="a.gif"> img.setAttribute("SRC", "a.gif") works fine as well. I would suggest it's undesired behavior for setAttribute and setAttributeNode to behave differently. See alos: bug 105713. Created attachment 189458 [details]
Patch
Comment on attachment 189458 [details] Patch Attachment 189458 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16646830 New failing tests: fast/dom/Element/getAttribute-check-case-sensitivity.html fast/dom/Element/setAttributeNode-case-insensitivity.html Created attachment 189497 [details]
Patch
Comment on attachment 189497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189497&action=review It looks this test could be turned into a text test by using window.getComputedStyle() > Source/WebCore/dom/Element.cpp:1727 > + const QualifiedName& attrName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); This is wrong. temporal object shouldn't stored in a reference. > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html:1 > +<html> It looks this test could be turned into a text test by using window.getComputedStyle() Then test will be more readable. Created attachment 189728 [details]
Patch
(In reply to comment #7) > (From update of attachment 189497 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189497&action=review > Thanks for the review. Have uploaded another patch with the suggested change for the testcase. > > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html:1 > > +<html> > > It looks this test could be turned into a text test by using window.getComputedStyle() > Then test will be more readable. Have changed the testcase to a text test but since I also wish to verify the values returned by the getAttribute() and getAttributeNode() APIs, instead of using getComputedStyle() have simply compared the values returned by the getAttribute() APIs. Without the fix getAttribute('style') would return a different value from getAttribut('STYLE'). > This is wrong. temporal object shouldn't stored in a reference.
Why?
Comment on attachment 189728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189728&action=review > Source/WebCore/dom/Element.cpp:1729 > - size_t index = elementData->getAttributeItemIndex(attrNode->qualifiedName()); > + const QualifiedName& attrName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); > + > + size_t index = elementData->getAttributeItemIndex(attrName); I would have preferred to share this code with setAttribute. > Source/WebCore/dom/Element.cpp:1737 > - setAttributeInternal(index, attrNode->qualifiedName(), attrNode->value(), NotInSynchronizationOfLazyAttribute); > + setAttributeInternal(index, (index != notFound ? attrName : attrNode->qualifiedName()), attrNode->value(), NotInSynchronizationOfLazyAttribute); Ditto. As Ryosuke pointed out on IRC, this will be worth being reviewed by experts like kling and anttik. > > This is wrong. temporal object shouldn't stored in a reference.
>
> Why?
For the record, On IRC I was convinced that I was wrong.
(In reply to comment #11) > (From update of attachment 189728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189728&action=review > Thanks for the review rniwa. > > Source/WebCore/dom/Element.cpp:1729 > > - size_t index = elementData->getAttributeItemIndex(attrNode->qualifiedName()); > > + const QualifiedName& attrName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); > > + > > + size_t index = elementData->getAttributeItemIndex(attrName); > > I would have preferred to share this code with setAttribute. > > > Source/WebCore/dom/Element.cpp:1737 > > - setAttributeInternal(index, attrNode->qualifiedName(), attrNode->value(), NotInSynchronizationOfLazyAttribute); > > + setAttributeInternal(index, (index != notFound ? attrName : attrNode->qualifiedName()), attrNode->value(), NotInSynchronizationOfLazyAttribute); > > Ditto. I did try to make a shared inline function, but setAttributeNode() requires that setAttributeInternal() be called post: if (index != notFound) { if (oldAttrNode) detachAttrNodeFromElementWithValue(oldAttrNode.get(), elementData->attributeItem(index)->value()); else oldAttrNode = Attr::create(document(), attrNode->qualifiedName(), elementData->attributeItem(index)->value()); } [LayoutTests/fast/dom/attr-style-too-lazy.html fails otherwise. This is because setAttributeInternal() would update the value of the existing attribute prior to retrieving it.] Thus probably only the call to getAttributeItemIndex() can be shared between the two methods. Should we still go ahead and make this change? Created attachment 190456 [details]
Patch
Comment on attachment 190456 [details] Patch Attachment 190456 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16807053 New failing tests: fast/dom/Element/setAttributeNode-case-insensitivity.html Created attachment 190466 [details]
Patch
Comment on attachment 190466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190466&action=review > Source/WebCore/dom/Element.cpp:1726 > - size_t index = elementData->getAttributeItemIndex(attrNode->qualifiedName()); > + const QualifiedName& caseAdjustedName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); > + size_t index = elementData->getAttributeItemIndex(caseAdjustedName); Is it correct to strip away the prefix and namespace for the shouldIgnoreAttributeCase(this) case? Comment on attachment 190466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190466&action=review >> Source/WebCore/dom/Element.cpp:1726 >> - size_t index = elementData->getAttributeItemIndex(attrNode->qualifiedName()); >> + const QualifiedName& caseAdjustedName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); >> + size_t index = elementData->getAttributeItemIndex(caseAdjustedName); > > Is it correct to strip away the prefix and namespace for the shouldIgnoreAttributeCase(this) case? I don’t think it is correct. We need to make sure the test case covers this. Created attachment 190924 [details]
Patch
(In reply to comment #18) > (From update of attachment 190466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190466&action=review > > > Source/WebCore/dom/Element.cpp:1726 > > - size_t index = elementData->getAttributeItemIndex(attrNode->qualifiedName()); > > + const QualifiedName& caseAdjustedName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); > > + size_t index = elementData->getAttributeItemIndex(caseAdjustedName); > > Is it correct to strip away the prefix and namespace for the shouldIgnoreAttributeCase(this) case? I admit I overlooked this part. Thanks for the correction. Have uploaded another patch that does take the prefix and namespace into account as well. The layout test also verifies this behavior. (In reply to comment #19) > (From update of attachment 190466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190466&action=review > > >> Source/WebCore/dom/Element.cpp:1726 > >> - size_t index = elementData->getAttributeItemIndex(attrNode->qualifiedName()); > >> + const QualifiedName& caseAdjustedName = shouldIgnoreAttributeCase(this) ? QualifiedName(nullAtom, attrNode->name().lower(), nullAtom) : attrNode->qualifiedName(); > >> + size_t index = elementData->getAttributeItemIndex(caseAdjustedName); > > > > Is it correct to strip away the prefix and namespace for the shouldIgnoreAttributeCase(this) case? > > I don’t think it is correct. We need to make sure the test case covers this. That's right, it was indeed an oversight on my part. Have uploaded another patch that takes the namespace and prefix into account as well. Have tried to verify the same in the layout testcase as well. Hi Darin, Kling, In the testcase I have tried to verify the behavior when: i) Two attributes with the same name (same case as well) but different namespace (different prefix) are added using setAttributeNode(). The second attribute doesn't modify the first (existing) attribute which should be the proper behavior. ii) Two attributes with the same name but different case and same namespace (different prefix) are added using setAttributeNode(). While adding, the second attribute overwrites the value of the first attribute. Again, this is as per the expected behavior since now we check against existing attributes case-insensitively. Since the namespace too matches for both these attributes (checked in getAttributeItemIndex(QualifiedName) where the matches() method on the QualifiedName checks for the namespace), the existing value is overwritten. However the prefix are different. Do we need to differentiate based on prefix as well? Created attachment 191103 [details]
Slightly tweaked layout test patch
Comment on attachment 191103 [details] Slightly tweaked layout test patch View in context: https://bugs.webkit.org/attachment.cgi?id=191103&action=review > Source/WebCore/dom/Element.cpp:1727 > + const QualifiedName& caseAdjustedName = shouldIgnoreAttributeCase(this) ? QualifiedName(attrName.prefix(), attrName.localName().lower(), attrName.namespaceURI()) : attrName; If this was performance critical, we could have a more-optimal case for the case where attrName.localName() is already all lowercase that avoids a hash table lookup to create a new QualifiedName in that case. But the entire Attr-related API is intrinsically slow already, so that seems like an unneeded micro-optimization. Comment on attachment 191103 [details] Slightly tweaked layout test patch Clearing flags on attachment: 191103 Committed r144595: <http://trac.webkit.org/changeset/144595> All reviewed patches have been landed. Closing bug. (In reply to comment #25) > (From update of attachment 191103 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191103&action=review > > > Source/WebCore/dom/Element.cpp:1727 > > + const QualifiedName& caseAdjustedName = shouldIgnoreAttributeCase(this) ? QualifiedName(attrName.prefix(), attrName.localName().lower(), attrName.namespaceURI()) : attrName; > > If this was performance critical, we could have a more-optimal case for the case where attrName.localName() is already all lowercase that avoids a hash table lookup to create a new QualifiedName in that case. But the entire Attr-related API is intrinsically slow already, so that seems like an unneeded micro-optimization. Thanks for the r+ Darin! I suppose we should consider having a lowercase localname as not just for this case but others too [see: https://bugs.webkit.org/show_bug.cgi?id=105713] we have to convert to lowercase. I'm concerned that this was the source of intermittent crashes in chromium. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fdom%2FElement%2F All of the crashes are hitting crash log for DumpRenderTree (pid 2228): STDOUT: <empty> STDERR: ASSERTION FAILED: m_element STDERR: ../../third_party/WebKit/Source/WebCore/dom/Attr.cpp(215) : void WebCore::Attr::detachFromElementWithValue(const WTF::AtomicString &) STDERR: 1 0x731cf95 WebCore::Attr::detachFromElementWithValue(WTF::AtomicString const&) STDERR: 2 0x744b32b WebCore::Element::detachAllAttrNodesFromElement() The earliest occurance of this crash is at r144595. It's not obvious to me how this could be causing Attrs to not have an m_element upon dtion, but the revision & code proximity make me suspicious. Arpita? Darin? (In reply to comment #29) > It's not obvious to me how this could be causing Attrs to not have an m_element upon dtion, but the revision & code proximity make me suspicious. I don’t have anything to add. It’s not obvious to me either, but if this is the revision where the problem began, perhaps it’s responsible. Which test is the crash happening in? Too many to list. Here's a sample: fast/dom/Element/id-in-param.html fast/dom/Element/id-in-formcollection.html fast/dom/Element/offsetLeft-offsetTop-html.html fast/dom/Element/id-in-getelement01.html fast/dom/Element/id-in-frameset.html fast/dom/Element/id-in-frame.html I'm going to try reverting. Apologies in advance if this isn't the cuprit. Reverted r144595 for reason: Causing multiple crashes in fast/dom/Element/* tests (hits assert in Attr.cp:215 Committed r144702: <http://trac.webkit.org/changeset/144702> reverted in http://trac.webkit.org/changeset/144702 (In reply to comment #30) > (In reply to comment #29) > > It's not obvious to me how this could be causing Attrs to not have an m_element upon dtion, but the revision & code proximity make me suspicious. > > I don’t have anything to add. It’s not obvious to me either, but if this is the revision where the problem began, perhaps it’s responsible. > > Which test is the crash happening in? Hi Darin, The assert occurs whenever garbage collection is fired after executing the test LayoutTests/fast/dom/Element/getAttribute-check-case-sensitivity.html. This is coz, a case within ^^ first adds an attribute with name "B" and then tries to add another attribute with the same name and case. In setAttributeNode() my patch was converting the attr name to lowercase and then verifying against the existing attributes. Thus, not finding an attribute node (since the case won't match), it would add another attribute node to the element's node list (with the same name and case!). Setting the attr in this way we do not clear the node's attr list properly, which during garbage collection causes the assert in Attr::detachFromElementWithValue(). Problem was that converting the input attr name to lowercase does not always guarantee case-insensitive comparison for setAttribute() (since existing attr could be in uppercase). Shall upload another patch, taking the case-insensitive comparison to getAttributeItemIndex(QualifiedName&) instead of doing the lowercase check in setAttribute(). getAttributeItemIndex() calls on QualifiedName::matches() in which we can do a case-insensitive/sensitive check based on an additional param. getAttributeItemIndex(AtomicString&) already takes another param for performing similar functionality. Would appreciate your thoughts on the same. (In reply to comment #31) > Too many to list. Here's a sample: > > fast/dom/Element/id-in-param.html > fast/dom/Element/id-in-formcollection.html > fast/dom/Element/offsetLeft-offsetTop-html.html > fast/dom/Element/id-in-getelement01.html > fast/dom/Element/id-in-frameset.html > fast/dom/Element/id-in-frame.html > > I'm going to try reverting. Apologies in advance if this isn't the cuprit. Thanks for reverting the patch. It was indeed causing the assert. Created attachment 192196 [details]
Patch
Comment on attachment 192196 [details] Patch Rejecting attachment 192196 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 192196, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: rej patching file Source/WebCore/dom/QualifiedName.h Hunk #1 succeeded at 79 (offset -2 lines). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity-expected.txt patching file LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/1326133 Created attachment 208018 [details]
Patch
Hi Ryosuke, many thanks for the review! Unfortunately the code had changed since the time of submitting the last patch. Have merged the changes to the latest and re-submitted. Comment on attachment 208018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208018&action=review > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html:8 > +#test { > + font-family: ahem; > + width: 50; > + height: 50; > +} Do we need this style? Created attachment 208240 [details]
Patch
(In reply to comment #40) > (From update of attachment 208018 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208018&action=review > Thanks for the review Ryosuke! > > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html:8 > > +#test { > > + font-family: ahem; > > + width: 50; > > + height: 50; > > +} > > Do we need this style? Removed since this wasn't really required. Have uploaded another patch with the modifications. Added one small change to the patch in Element::setAttributeInternal(). If an existing attribute is found then we should update the new value to that attribute. So we should use the name corresponding to that attribute. This can be verified by using the previous patch and loading the attached layout testcase. Notice that even though the layout testcase passes, the color of the div in the testcase still remains red. With the fix in setAttributeInternal() however, the value shall be set properly, i.e. to the existing attribute, and we'll get a green colored div. Comment on attachment 208240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208240&action=review > Source/WebCore/dom/QualifiedName.h:82 > - bool matches(const QualifiedName& other) const { return m_impl == other.m_impl || (localName() == other.localName() && namespaceURI() == other.namespaceURI()); } > + bool matches(const QualifiedName& other, bool shouldIgnoreCase = false) const { return m_impl == other.m_impl || (equalPossiblyIgnoringCase(localName(), other.localName(), shouldIgnoreCase) && namespaceURI() == other.namespaceURI()); } Please add a new function such as matchesIgnoringCaseForLocalName. The current patch will bloat thousands of call sites of hasTagName. Created attachment 208520 [details]
Patch
(In reply to comment #43) > (From update of attachment 208240 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208240&action=review > > > Source/WebCore/dom/QualifiedName.h:82 > > - bool matches(const QualifiedName& other) const { return m_impl == other.m_impl || (localName() == other.localName() && namespaceURI() == other.namespaceURI()); } > > + bool matches(const QualifiedName& other, bool shouldIgnoreCase = false) const { return m_impl == other.m_impl || (equalPossiblyIgnoringCase(localName(), other.localName(), shouldIgnoreCase) && namespaceURI() == other.namespaceURI()); } > > Please add a new function such as matchesIgnoringCaseForLocalName. The current patch will bloat thousands of call sites of hasTagName. Thanks for the review R Niwa. Have submitted another patch with the specified change. Comment on attachment 208520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208520&action=review > Source/WebCore/dom/Element.cpp:3243 > - if (attributes[i].name() == attr->qualifiedName()) > + if (attributes[i].name().matchesIgnoringCaseForLocalName(attr->qualifiedName(), shouldIgnoreAttributeCase)) On my second thought, this is incorrect. One one hand, createElement('STYLE') in a HTML document will create an attribute with localName of 'style', NOT 'STYLE'. On the other hand, 'STYLE' attribute is NOT equal to 'style' attribute in a XHTML document. Furthermore, XHTML documents differentiates each attribute in case sensitive matter so there is no way this code is correct at least for XHTML document. r-. > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html:1 > +<html> Missing DOCTYPE. (In reply to comment #46) > (From update of attachment 208520 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208520&action=review > > > Source/WebCore/dom/Element.cpp:3243 > > - if (attributes[i].name() == attr->qualifiedName()) > > + if (attributes[i].name().matchesIgnoringCaseForLocalName(attr->qualifiedName(), shouldIgnoreAttributeCase)) > > On my second thought, this is incorrect. One one hand, createElement('STYLE') in a HTML document will create an attribute with localName of 'style', NOT 'STYLE'. Correction. createElement('STYLE') in a HTML document SHOULD create an attribute with localName of 'style', NOT 'STYLE'. (In reply to comment #47) > (In reply to comment #46) > > (From update of attachment 208520 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208520&action=review > > > > > Source/WebCore/dom/Element.cpp:3243 > > > - if (attributes[i].name() == attr->qualifiedName()) > > > + if (attributes[i].name().matchesIgnoringCaseForLocalName(attr->qualifiedName(), shouldIgnoreAttributeCase)) > > > > On my second thought, this is incorrect. One one hand, createElement('STYLE') in a HTML document will create an attribute with localName of 'style', NOT 'STYLE'. > > Correction. createElement('STYLE') in a HTML document SHOULD create an attribute with localName of 'style', NOT 'STYLE'. Nm... it seems like this isn't specified anywhere since DOM4 deprecates createAttribute. However, DOM4 clearly specifies this behavior for createElement: http://dom.spec.whatwg.org/#dom-document-createelement Given that, I'm inclined to say we should do the same for createAttribute. (In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > (From update of attachment 208520 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=208520&action=review > > > > > > > Source/WebCore/dom/Element.cpp:3243 > > > > - if (attributes[i].name() == attr->qualifiedName()) > > > > + if (attributes[i].name().matchesIgnoringCaseForLocalName(attr->qualifiedName(), shouldIgnoreAttributeCase)) > > > > > > On my second thought, this is incorrect. One one hand, createElement('STYLE') in a HTML document will create an attribute with localName of 'style', NOT 'STYLE'. > > > > Correction. createElement('STYLE') in a HTML document SHOULD create an attribute with localName of 'style', NOT 'STYLE'. > > Nm... it seems like this isn't specified anywhere since DOM4 deprecates createAttribute. However, DOM4 clearly specifies this behavior for createElement: > http://dom.spec.whatwg.org/#dom-document-createelement > > Given that, I'm inclined to say we should do the same for createAttribute. Thanks for the review R Niwa. createElement() and createAttribute() do behave differently as regards the case of the passed argument. Where an uppercase arg passed to createElement() shall still create a lowercase element, the same is not true for createAttribute. Consider a simple testcase as: <!DOCTYPE html> <html> <head> <script> function test() { var newAttr = document.createAttribute("WIDTH"); newAttr.value = "200px"; var testElem = document.getElementById('testTag'); testElem.setAttributeNode(newAttr); var newAttr = document.createAttribute("RANDOM"); newAttr.value = "xyz"; var testElem = document.getElementById('testTag'); testElem.setAttributeNode(newAttr); document.getElementById('result').innerHTML = "lowercase width = "+testElem.getAttributeNode('width').value+" uppercase width = "+testElem.getAttributeNode('WIDTH').value; } </script> </head> <body onload="test()"> <img id="testTag" src="50x50.gif" height="50px" width="50px" /> <div id="result"></div> </body> </html> Here, the generated DOM on FireFox is: <img WIDTH="200px" height="50px" src="50x50.gif" id="testTag" RANDOM="xyz"> whereas on WebKit it is: <img id="testTag" src="50x50.gif" height="50px" width="50px" WIDTH="200px" RANDOM="xyz"> This shows that createAttribute() should allow for creation of attributes in any case. However, setAttributeNode(), while checking against existing attributes, should first convert the passed argument to lowercase and then compare. The specification for setAttribute() [http://dom.spec.whatwg.org/#dom-element-setattribute] states that we should first convert the passed name to ASCII lowercase. This patch assumes that the same should hold true for setAttributeNode() as well. Also, I did make changes to createAttribute() to create only lowercase attributes but that would fail tests such as: LayoutTests/fast/dom/Element/getAttribute-check-case-sensitivity.html which expects that attributes be allowed in any case. [http://trac.webkit.org/changeset/35931] Correction: The DOM on FF for the testcase mentioned in the previous comment [#49] is: <img width="200px" height="50px" src="50x50.gif" id="testTag" RANDOM="xyz"> Created attachment 208622 [details]
Patch
Hi R Niwa, Have uploaded another patch after adding the doctype to the layout testcase. Also added another testcase which verifies setAttributeNode() behavior for xhtml documents. Created attachment 208627 [details]
Patch
Comment on attachment 208627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208627&action=review > Source/WebCore/dom/QualifiedName.h:84 > + bool matchesIgnoringCaseForLocalName(const QualifiedName& other, bool shouldIgnoreCase = false) const { return m_impl == other.m_impl || (equalPossiblyIgnoringCase(localName(), other.localName(), shouldIgnoreCase) && namespaceURI() == other.namespaceURI()); } There is no reason for the shouldIgnoreCase function to have a default of false. When the value is false, it's just a slower version of the matches function, so we would not want anyone to call it in that case. Comment on attachment 208627 [details]
Patch
I’d like to see more test coverage here, covering things like non-XHTML XML documents.
Comment on attachment 208627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208627&action=review > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity-xhtml-expected.txt:2 > +This test verifies that the setAttributeNode() API allows for creation of attributes case-sensitively for XHTML documents. Thus two different attributes with the same name but in different case can exist for XHTML documents. This is not what Firefox 23 does. Firefox 23 does case-sensitive comparison of attribute names in a XHTML document. (In reply to comment #56) > (From update of attachment 208627 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208627&action=review > > > LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity-xhtml-expected.txt:2 > > +This test verifies that the setAttributeNode() API allows for creation of attributes case-sensitively for XHTML documents. Thus two different attributes with the same name but in different case can exist for XHTML documents. > > This is not what Firefox 23 does. Firefox 23 does case-sensitive comparison of attribute names in a XHTML document. Hi Ryosuke, For the following xhtml test: <?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"> <head> <script> function test() { var newAttr = document.createAttribute("WIDTH"); newAttr.value = "200px"; var testElem = document.getElementById('testTag'); testElem.setAttributeNode(newAttr); document.getElementById('result').innerHTML = "lowercase width = "+testElem.getAttributeNode('width').value+" uppercase width = "+testElem.getAttributeNode('WIDTH').value; } </script> </head> <body onload="test()"> <img id="testTag" src="50x50.gif" height="50px" width="50px" /> <div id="result"></div> </body> </html> I get the following o/p in FF (v23): "lowercase width = 50px uppercase width = 200px" Also, the generated DOM is: <body onload="test()"> <img id="testTag" width="50px" height="50px" src="50x50.gif" WIDTH="200px"/> <div id="result">lowercase width = 50px uppercase width = 200px</div> </body> but the same test for an HTML doc doesn't create a new attribute for width in uppercase. Oh never mind. I must have misread the test case. I thought your test case was testing that we'll do case-insensitive comparison even in a XHTML document, which will be wrong. Created attachment 208706 [details]
Patch
(In reply to comment #54) > (From update of attachment 208627 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208627&action=review > > > Source/WebCore/dom/QualifiedName.h:84 > > + bool matchesIgnoringCaseForLocalName(const QualifiedName& other, bool shouldIgnoreCase = false) const { return m_impl == other.m_impl || (equalPossiblyIgnoringCase(localName(), other.localName(), shouldIgnoreCase) && namespaceURI() == other.namespaceURI()); } > > There is no reason for the shouldIgnoreCase function to have a default of false. When the value is false, it's just a slower version of the matches function, so we would not want anyone to call it in that case. Thanks for the r+ Darin! Have made this change in the submitted patch. Comment on attachment 208706 [details] Patch Rejecting attachment 208706 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 208706, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/1453806 Created attachment 208714 [details]
Patch for landing
Comment on attachment 208714 [details] Patch for landing Clearing flags on attachment: 208714 Committed r154054: <http://trac.webkit.org/changeset/154054> All patches have been landed, closing the bug. |