90341 – createAttribute/setAttributeNode does not properly normalize case

RESOLVED FIXED 90341

createAttribute/setAttributeNode does not properly normalize case

https://bugs.webkit.org/show_bug.cgi?id=90341

Summary createAttribute/setAttributeNode does not properly normalize case

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	Details Formatted Diff Diff
Patch (7.14 KB, patch) 2013-02-21 03:17 PST, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (6.62 KB, patch) 2013-02-22 02:34 PST, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (5.80 KB, patch) 2013-02-26 23:56 PST, Arpita Bahuguna	buildbot: commit-queue-	Details Formatted Diff Diff
Patch (5.81 KB, patch) 2013-02-27 01:37 PST, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (8.90 KB, patch) 2013-03-01 03:27 PST, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Slightly tweaked layout test patch (8.62 KB, patch) 2013-03-02 02:41 PST, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (10.21 KB, patch) 2013-03-08 03:47 PST, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (10.45 KB, patch) 2013-08-02 07:15 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (11.01 KB, patch) 2013-08-07 00:23 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (10.30 KB, patch) 2013-08-12 02:35 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (13.15 KB, patch) 2013-08-13 05:52 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (13.14 KB, patch) 2013-08-13 07:33 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (13.14 KB, patch) 2013-08-14 02:03 PDT, Arpita Bahuguna	kling: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
Patch for landing (13.18 KB, patch) 2013-08-14 04:36 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Minor

Classification Unclassified

Version 528+ (Nightly build)

Hardware Mac (Intel)

OS OS X 10.7

Product WebKit

Component DOM

Assignee

Arpita Bahuguna

Reported

2012-06-30 15:43 PDT

Modified

2016-01-15 19:30 PST History

CC List

19 users Show

URL

Keywords

Depends on

Blocks