Bug 16923 – Attr nodes with a value of "" should not have any childNodes

• Component: DOM
• Status: RESOLVED FIXED

[Michael A. Puls II]

In Firefox, Opera and IE, whenever an Attr's value is "", the node has no child nodes. In Safari 3.0.5 (525.3) with WebKit-SVN-r29603, this is not the case.

[Michael A. Puls II]

Created attachment 18528 [details]
TC

Opera, Firefox and IE pass. Safari does not.

[Alexey Proskuryakov]

Does this affect any real life sites?

While we probably want to match other browsers, it seems weird that an empty (not null) attribute is special.

[Michael A. Puls II]

(In reply to comment #2)
> Does this affect any real life sites?

Not sure. However, when checking out Test 10 of Acid3 (and doing some TCs that are not in there), I realized that webkit has this problem.

> While we probably want to match other browsers, it seems weird that an empty
> (not null) attribute is special.

Element.textContent = "" removes all nodes from the element.
Attr.textContent = "" removes all nodes from the Attr (in Safari also).

It probably makes sense for Attr.value = "" to be consistent and do the same as Attr.textContent = "".

[Michael A. Puls II]

At < http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/dom/Attr.cpp#L118 >, I see removeChildren() and then right after that I see that a textNode is always appended even if v is empty.

Would something like:
 
if (!v.isEmpty()) {
    appendChild(document()->createTextNode(v), e)
    // whatever else needs to be moved to this condition
}

fix it?

It's basically what setTextContent does at < http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/dom/Node.cpp#L1561 >.

[Alexey Proskuryakov]

Yes, this looks quite reasonable.

I'd try to use Attr::createTextChild() here. Also, the test case needs to verify what happens to attributes created by the parser - since we only create Attr nodes on demand, the relationship between Attr and Attribute is tricky.

[David Kilzer (:ddkilzer)]

Confirmed with a local debug build of ToT WebKit.

[Michael A. Puls II]

(In reply to comment #5)
> Also, the test case needs to
> verify what happens to attributes created by the parser - since we only create
> Attr nodes on demand, the relationship between Attr and Attribute is tricky.

I'll make a better TC that covers that.

[Michael A. Puls II]

Created attachment 18561 [details]
TC that tests both new Attrs and parsed Attrs

[Michael A. Puls II]

Created attachment 18562 [details]
Another one that only tests a parsed Attr.

And, I also made < http://shadow2531.com/opera/testcases/attr/suite0.html > that you can test with.

[Michael A. Puls II]

(In reply to comment #5)
> Yes, this looks quite reasonable.
> 
> I'd try to use Attr::createTextChild() here. Also, the test case needs to
> verify what happens to attributes created by the parser - since we only create
> Attr nodes on demand, the relationship between Attr and Attribute is tricky.

Well, I tried:

if (!v.isEmpty())
    appendChild(document()->createTextNode(v), e);

and that seems to do the trick.

Now, if I use createTextChild instead like you suggest, does that mean I have to put:
if (!v.isEmpty())
    createTextChild()

*after*, the m_attribute->setValue(v.impl()); line?

[Alexey Proskuryakov]

Yes, it would need to be called after the value is set (but there is no need to check that v is not empty then - createTextChild() does that internally).

[Michael A. Puls II]

Created attachment 20968 [details]
Example patch

(In reply to comment #11)
> Yes, it would need to be called after the value is set (but there is no need to
> check that v is not empty then - createTextChild() does that internally).
>

Oops, thanks. So, the attached changes would be O.K.?

They make the 2 TCs pass and make <http://shadow2531.com/opera/testcases/attr/suite0.html> score 100% (in the Midori browser). (Not sure how to build the jscore tests etc. on linux, so I didn't do any of that.)

[Alexey Proskuryakov]

Yes, this looks reasonable to me. Could you please make a patch with the code changes, ChangeLogs and tests, and mark it for review? There are some guidelines and tools mentioned at <http://webkit.org/coding/contributing.html>.

[Michael A. Puls II]

Created attachment 21091 [details]
Proposed patch

[Alexey Proskuryakov]

Comment on attachment 21091 [details]
Proposed patch

r=me

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/33442>.

I made the ChangeLog a bit shorter when committing, as people can always go to Bugzilla for additional details.