Bug 16923 - Attr nodes with a value of "" should not have any childNodes
Summary: Attr nodes with a value of "" should not have any childNodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-01-18 06:11 PST by Michael A. Puls II
Modified: 2008-05-14 09:22 PDT (History)
1 user (show)

See Also:


Attachments
TC (643 bytes, text/html)
2008-01-18 06:13 PST, Michael A. Puls II
no flags Details
TC that tests both new Attrs and parsed Attrs (2.48 KB, text/html)
2008-01-19 22:12 PST, Michael A. Puls II
no flags Details
Another one that only tests a parsed Attr. (810 bytes, text/html)
2008-01-20 00:16 PST, Michael A. Puls II
no flags Details
Example patch (770 bytes, patch)
2008-05-05 00:36 PDT, Michael A. Puls II
no flags Details | Formatted Diff | Diff
Proposed patch (8.69 KB, patch)
2008-05-12 11:02 PDT, Michael A. Puls II
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael A. Puls II 2008-01-18 06:11:28 PST
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.
Comment 1 Michael A. Puls II 2008-01-18 06:13:05 PST
Created attachment 18528 [details]
TC

Opera, Firefox and IE pass. Safari does not.
Comment 2 Alexey Proskuryakov 2008-01-18 23:18:56 PST
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.
Comment 3 Michael A. Puls II 2008-01-18 23:49:03 PST
(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 = "".
Comment 4 Michael A. Puls II 2008-01-19 00:55:53 PST
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 >.
Comment 5 Alexey Proskuryakov 2008-01-19 11:15:36 PST
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.
Comment 6 David Kilzer (:ddkilzer) 2008-01-19 11:59:35 PST
Confirmed with a local debug build of ToT WebKit.

Comment 7 Michael A. Puls II 2008-01-19 20:59:25 PST
(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.

Comment 8 Michael A. Puls II 2008-01-19 22:12:37 PST
Created attachment 18561 [details]
TC that tests both new Attrs and parsed Attrs
Comment 9 Michael A. Puls II 2008-01-20 00:16:46 PST
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.
Comment 10 Michael A. Puls II 2008-05-02 14:35:32 PDT
(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?
Comment 11 Alexey Proskuryakov 2008-05-04 22:49:23 PDT
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).
Comment 12 Michael A. Puls II 2008-05-05 00:36:27 PDT
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.)
Comment 13 Alexey Proskuryakov 2008-05-05 03:45:22 PDT
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>.
Comment 14 Michael A. Puls II 2008-05-12 11:02:45 PDT
Created attachment 21091 [details]
Proposed patch
Comment 15 Alexey Proskuryakov 2008-05-14 09:09:56 PDT
Comment on attachment 21091 [details]
Proposed patch

r=me
Comment 16 Alexey Proskuryakov 2008-05-14 09:22:01 PDT
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.