Bug 90341 - createAttribute/setAttributeNode does not properly normalize case
Summary: createAttribute/setAttributeNode does not properly normalize case
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.7
: P2 Minor
Assignee: Arpita Bahuguna
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-30 15:43 PDT by michael
Modified: 2016-01-15 19:30 PST (History)
19 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description michael 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.
Comment 1 Alexey Proskuryakov 2012-07-01 10:58:48 PDT
What does HTML5 say?
Comment 2 michael 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.
Comment 3 Alexey Proskuryakov 2013-01-02 10:19:57 PST
See alos: bug 105713.
Comment 4 Arpita Bahuguna 2013-02-20 21:54:07 PST
Created attachment 189458 [details]
Patch
Comment 5 WebKit Review Bot 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
Comment 6 Arpita Bahuguna 2013-02-21 03:17:45 PST
Created attachment 189497 [details]
Patch
Comment 7 Hajime Morrita 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.
Comment 8 Arpita Bahuguna 2013-02-22 02:34:15 PST
Created attachment 189728 [details]
Patch
Comment 9 Arpita Bahuguna 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').
Comment 10 Alexey Proskuryakov 2013-02-22 08:25:17 PST
> This is wrong. temporal object shouldn't stored in a reference.

Why?
Comment 11 Ryosuke Niwa 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.
Comment 12 Hajime Morrita 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.
Comment 13 Hajime Morrita 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.
Comment 14 Arpita Bahuguna 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?
Comment 15 Arpita Bahuguna 2013-02-26 23:56:24 PST
Created attachment 190456 [details]
Patch
Comment 16 Build Bot 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
Comment 17 Arpita Bahuguna 2013-02-27 01:37:10 PST
Created attachment 190466 [details]
Patch
Comment 18 Andreas Kling 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?
Comment 19 Darin Adler 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.
Comment 20 Arpita Bahuguna 2013-03-01 03:27:07 PST
Created attachment 190924 [details]
Patch
Comment 21 Arpita Bahuguna 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.
Comment 22 Arpita Bahuguna 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.
Comment 23 Arpita Bahuguna 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?
Comment 24 Arpita Bahuguna 2013-03-02 02:41:26 PST
Created attachment 191103 [details]
Slightly tweaked layout test patch
Comment 25 Darin Adler 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.
Comment 26 WebKit Review Bot 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>
Comment 27 WebKit Review Bot 2013-03-03 21:53:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Arpita Bahuguna 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.
Comment 29 Rafael Weinstein 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?
Comment 30 Darin Adler 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?
Comment 31 Rafael Weinstein 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.
Comment 32 Rafael Weinstein 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>
Comment 33 Rafael Weinstein 2013-03-04 17:53:43 PST
reverted in http://trac.webkit.org/changeset/144702
Comment 34 Arpita Bahuguna 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.
Comment 35 Arpita Bahuguna 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.
Comment 36 Arpita Bahuguna 2013-03-08 03:47:23 PST
Created attachment 192196 [details]
Patch
Comment 37 WebKit Commit Bot 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
Comment 38 Arpita Bahuguna 2013-08-02 07:15:59 PDT
Created attachment 208018 [details]
Patch
Comment 39 Arpita Bahuguna 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.
Comment 40 Ryosuke Niwa 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?
Comment 41 Arpita Bahuguna 2013-08-07 00:23:23 PDT
Created attachment 208240 [details]
Patch
Comment 42 Arpita Bahuguna 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.
Comment 43 Ryosuke Niwa 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.
Comment 44 Arpita Bahuguna 2013-08-12 02:35:13 PDT
Created attachment 208520 [details]
Patch
Comment 45 Arpita Bahuguna 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.
Comment 46 Ryosuke Niwa 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.
Comment 47 Ryosuke Niwa 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'.
Comment 48 Ryosuke Niwa 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.
Comment 49 Arpita Bahuguna 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]
Comment 50 Arpita Bahuguna 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">
Comment 51 Arpita Bahuguna 2013-08-13 05:52:15 PDT
Created attachment 208622 [details]
Patch
Comment 52 Arpita Bahuguna 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.
Comment 53 Arpita Bahuguna 2013-08-13 07:33:48 PDT
Created attachment 208627 [details]
Patch
Comment 54 Darin Adler 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.
Comment 55 Darin Adler 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.
Comment 56 Ryosuke Niwa 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.
Comment 57 Arpita Bahuguna 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.
Comment 58 Ryosuke Niwa 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.
Comment 59 Arpita Bahuguna 2013-08-14 02:03:17 PDT
Created attachment 208706 [details]
Patch
Comment 60 Arpita Bahuguna 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.
Comment 61 WebKit Commit Bot 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
Comment 62 Arpita Bahuguna 2013-08-14 04:36:04 PDT
Created attachment 208714 [details]
Patch for landing
Comment 63 WebKit Commit Bot 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>
Comment 64 Ryosuke Niwa 2016-01-15 19:30:36 PST
All patches have been landed, closing the bug.