469304 - Attribute nodes set with setAttributeNode get mutated

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See the Webkit testcase. We get failures like this:
FAIL testAttribNodeNamePreservesCaseGetNode() should be A,A. Was a,a.
The attribute name in the attribute node has been lowercased after we set it. That seems wrong.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Note to myself, need to verify this is actually valid.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

The testcase tests something which isn't specified anywhere.
Attributes are by default lowercase in HTML (per web compatibility).
So what I'd like to do is to make testAttribNodeNamePreservesCaseGetNode2()
fail because testAttribNodeNamePreservesCaseGetNode() fails and
make attrib.name fail because node.getAttributeNode('myattrib').name fails.
That way we would handle attributes consistently within HTML elements.
(and those are both the same bug anyway and it is a real bug.)

We could perhaps do the lowercasing already when creating attribute nodes in HTML document.

I really don't want to do what webkit does here. With latest webkit it is 
possible to add attribute nodes which values aren't available using .getAttribute().

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a testcase

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Taking so that I can fix the testAttribNodeNamePreservesCaseGetNode2() case.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Actually, I'll fix this in a bit different way.
DOMAttr.nodeName and DOMAttr.name could keep their caseness, but 
getAttribute should still work correctly.
This should fix other cases than (new XMLSerializer).serializeToString(node) should be <div myAttrib="XXX"></div>. Was <DIV myattrib="XXX"/>.
(which we do correctly IMO, because in HTML attributes are in lowercase).

Patch coming.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

Comment 7

[Julien Chaffraix]

(In reply to comment #2)
> The testcase tests something which isn't specified anywhere.
> Attributes are by default lowercase in HTML (per web compatibility).
> So what I'd like to do is to make testAttribNodeNamePreservesCaseGetNode2()
> fail because testAttribNodeNamePreservesCaseGetNode() fails and
> make attrib.name fail because node.getAttributeNode('myattrib').name fails.
> That way we would handle attributes consistently within HTML elements.
> (and those are both the same bug anyway and it is a real bug.)

FYI, this test should match IE, Opera and WebKit behaviours [1]. By failing part of it, you will be the only one to have this behaviour.
When fixing this bug on WebKit, I did not find any specification too and decided to side with IE and Opera as they had a more consistent behaviour.

> I really don't want to do what webkit does here. With latest webkit it is 
> possible to add attribute nodes which values aren't available using
> .getAttribute().

Seems like a bug. Could you file a bug about that on the WebKit bugzilla?

[1] https://bugs.webkit.org/attachment.cgi?id=22610&action=view (part of the bug https://bugs.webkit.org/show_bug.cgi?id=20247)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Julien, see the comment 5 ;)
The patch makes Gecko to pass all the other case
except XMLSerializer (). That test fails on Opera too, although in a different
way than Gecko.
Gecko serializes HTML to <DIV myattrib="XXX"/>
Opera <div myAttrib="XXX"/>
WebKit <div myAttrib=\"XXX\"></div>

Comment 9

[Julien Chaffraix]

(In reply to comment #8)
> Julien, see the comment 5 ;)

Oh, I had missed that. The changes you are proposing are great, sorry for the noise.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 352785 [details] [diff] [review]
possible patch

This isn't working atm.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date, changed namespace checks for html

Added also xmlns, at least temporarily. Depends on what we want to do with
Bug 501226.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Jonas, ping

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Jonas, re-ping.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/27f96382b94b