Bug 90341

Summary: createAttribute/setAttributeNode does not properly normalize case
Product: WebKit Reporter: michael
Component: DOMAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
buildbot: commit-queue-
Patch
none
Patch
none
Slightly tweaked layout test patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
kling: review+, commit-queue: commit-queue-
Patch for landing none

michael
Reported 2012-06-30 15:43:36 PDT
Online documents indicate that createAttribute should be case-insensitive in HTML documents (http://help.dottoro.com/ljiclkfc.php http://docs.oracle.com/javase/1.5.0/docs/api/org/w3c/dom/Document.html#createAttribute(java.lang.String)): "The name is case-sensitive in XML documents and case-insensitive in HTML documents." This example Javascript code will fail to properly set the image src. var img = document.getElementById("img"); var img_src = document.createAttribute("SRC"); img_src.value = "image.gif"; img.setAttributeNode(img_src); Changing the createAttribute argument to "src" works properly. Specifically, this behavior is exhibited in the program "anyterm." Fails in Google Chrome and latest WebKit, works in Firefox.
Attachments
Patch (8.55 KB, patch)
2013-02-20 21:54 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (7.14 KB, patch)
2013-02-21 03:17 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (6.62 KB, patch)
2013-02-22 02:34 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (5.80 KB, patch)
2013-02-26 23:56 PST, Arpita Bahuguna 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Patch (5.81 KB, patch)
2013-02-27 01:37 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (8.90 KB, patch)
2013-03-01 03:27 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Slightly tweaked layout test patch (8.62 KB, patch)
2013-03-02 02:41 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (10.21 KB, patch)
2013-03-08 03:47 PST, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (10.45 KB, patch)
2013-08-02 07:15 PDT, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (11.01 KB, patch)
2013-08-07 00:23 PDT, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (10.30 KB, patch)
2013-08-12 02:35 PDT, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (13.15 KB, patch)
2013-08-13 05:52 PDT, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (13.14 KB, patch)
2013-08-13 07:33 PDT, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Patch (13.14 KB, patch)
2013-08-14 02:03 PDT, Arpita Bahuguna 		kling: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (13.18 KB, patch)
2013-08-14 04:36 PDT, Arpita Bahuguna 		no flags
DetailsFormatted DiffDiff
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2012-07-01 10:58:48 PDT
What does HTML5 say?
michael
Comment 2 2012-07-01 11:39:14 PDT
(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.
Alexey Proskuryakov
Comment 3 2013-01-02 10:19:57 PST
See alos: bug 105713.
Arpita Bahuguna
Comment 4 2013-02-20 21:54:07 PST
Created attachment 189458 [details] Patch
WebKit Review Bot
Comment 5 2013-02-20 22:36:49 PST
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
Arpita Bahuguna
Comment 6 2013-02-21 03:17:45 PST
Created attachment 189497 [details] Patch
Hajime Morrita
Comment 7 2013-02-21 21:40:40 PST
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.
Arpita Bahuguna
Comment 8 2013-02-22 02:34:15 PST
Created attachment 189728 [details] Patch
Arpita Bahuguna
Comment 9 2013-02-22 02:40:30 PST
(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').
Alexey Proskuryakov
Comment 10 2013-02-22 08:25:17 PST
> This is wrong. temporal object shouldn't stored in a reference. Why?
Ryosuke Niwa
Comment 11 2013-02-23 00:12:57 PST
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.
Hajime Morrita
Comment 12 2013-02-23 00:25:32 PST
As Ryosuke pointed out on IRC, this will be worth being reviewed by experts like kling and anttik.
Hajime Morrita
Comment 13 2013-02-23 00:26:49 PST
> > This is wrong. temporal object shouldn't stored in a reference. > > Why? For the record, On IRC I was convinced that I was wrong.
Arpita Bahuguna
Comment 14 2013-02-26 06:26:55 PST
(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?
Arpita Bahuguna
Comment 15 2013-02-26 23:56:24 PST
Created attachment 190456 [details] Patch
Build Bot
Comment 16 2013-02-27 00:42:21 PST
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
Arpita Bahuguna
Comment 17 2013-02-27 01:37:10 PST
Created attachment 190466 [details] Patch
Andreas Kling
Comment 18 2013-02-27 05:00:50 PST
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?
Darin Adler
Comment 19 2013-02-27 15:48:54 PST
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.
Arpita Bahuguna
Comment 20 2013-03-01 03:27:07 PST
Created attachment 190924 [details] Patch
Arpita Bahuguna
Comment 21 2013-03-01 04:47:29 PST
(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.
Arpita Bahuguna
Comment 22 2013-03-01 04:50:18 PST
(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.
Arpita Bahuguna
Comment 23 2013-03-01 05:02:26 PST
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?
Arpita Bahuguna
Comment 24 2013-03-02 02:41:26 PST
Created attachment 191103 [details] Slightly tweaked layout test patch
Darin Adler
Comment 25 2013-03-02 18:44:21 PST
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.
WebKit Review Bot
Comment 26 2013-03-03 21:53:47 PST
Comment on attachment 191103 [details] Slightly tweaked layout test patch Clearing flags on attachment: 191103 Committed r144595: <http://trac.webkit.org/changeset/144595>
WebKit Review Bot
Comment 27 2013-03-03 21:53:54 PST
All reviewed patches have been landed. Closing bug.
Arpita Bahuguna
Comment 28 2013-03-04 06:09:52 PST
(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.
Rafael Weinstein
Comment 29 2013-03-04 16:58:53 PST
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?
Darin Adler
Comment 30 2013-03-04 17:35:27 PST
(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?
Rafael Weinstein
Comment 31 2013-03-04 17:44:45 PST
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.
Rafael Weinstein
Comment 32 2013-03-04 17:47:37 PST
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>
Rafael Weinstein
Comment 33 2013-03-04 17:53:43 PST
reverted in http://trac.webkit.org/changeset/144702
Arpita Bahuguna
Comment 34 2013-03-08 03:10:09 PST
(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.
Arpita Bahuguna
Comment 35 2013-03-08 03:17:29 PST
(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.
Arpita Bahuguna
Comment 36 2013-03-08 03:47:23 PST
Created attachment 192196 [details] Patch
WebKit Commit Bot
Comment 37 2013-08-02 02:43:41 PDT
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
Arpita Bahuguna
Comment 38 2013-08-02 07:15:59 PDT
Created attachment 208018 [details] Patch
Arpita Bahuguna
Comment 39 2013-08-02 07:19:49 PDT
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.
Ryosuke Niwa
Comment 40 2013-08-02 11:07:59 PDT
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?
Arpita Bahuguna
Comment 41 2013-08-07 00:23:23 PDT
Created attachment 208240 [details] Patch
Arpita Bahuguna
Comment 42 2013-08-07 00:52:30 PDT
(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.
Ryosuke Niwa
Comment 43 2013-08-12 00:29:05 PDT
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.
Arpita Bahuguna
Comment 44 2013-08-12 02:35:13 PDT
Created attachment 208520 [details] Patch
Arpita Bahuguna
Comment 45 2013-08-12 02:47:33 PDT
(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.
Ryosuke Niwa
Comment 46 2013-08-12 22:06:44 PDT
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.
Ryosuke Niwa
Comment 47 2013-08-12 22:08:43 PDT
(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'.
Ryosuke Niwa
Comment 48 2013-08-12 22:22:21 PDT
(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.
Arpita Bahuguna
Comment 49 2013-08-13 04:08:10 PDT
(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]
Arpita Bahuguna
Comment 50 2013-08-13 04:19:50 PDT
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">
Arpita Bahuguna
Comment 51 2013-08-13 05:52:15 PDT
Created attachment 208622 [details] Patch
Arpita Bahuguna
Comment 52 2013-08-13 05:54:31 PDT
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.
Arpita Bahuguna
Comment 53 2013-08-13 07:33:48 PDT
Created attachment 208627 [details] Patch
Darin Adler
Comment 54 2013-08-13 08:51:35 PDT
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.
Darin Adler
Comment 55 2013-08-13 08:52:26 PDT
Comment on attachment 208627 [details] Patch I’d like to see more test coverage here, covering things like non-XHTML XML documents.
Ryosuke Niwa
Comment 56 2013-08-13 17:10:15 PDT
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.
Arpita Bahuguna
Comment 57 2013-08-13 22:25:39 PDT
(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.
Ryosuke Niwa
Comment 58 2013-08-14 01:44:14 PDT
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.
Arpita Bahuguna
Comment 59 2013-08-14 02:03:17 PDT
Created attachment 208706 [details] Patch
Arpita Bahuguna
Comment 60 2013-08-14 02:05:56 PDT
(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.
WebKit Commit Bot
Comment 61 2013-08-14 04:14:16 PDT
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
Arpita Bahuguna
Comment 62 2013-08-14 04:36:04 PDT
Created attachment 208714 [details] Patch for landing
WebKit Commit Bot
Comment 63 2013-08-14 10:07:32 PDT
Comment on attachment 208714 [details] Patch for landing Clearing flags on attachment: 208714 Committed r154054: <http://trac.webkit.org/changeset/154054>
Ryosuke Niwa
Comment 64 2016-01-15 19:30:36 PST
All patches have been landed, closing the bug.
Note You need to log in before you can comment on or make changes to this bug.