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.
Created attachment 18528 [details] TC Opera, Firefox and IE pass. Safari does not.
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.
(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 = "".
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 >.
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.
Confirmed with a local debug build of ToT WebKit.
(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.
Created attachment 18561 [details] TC that tests both new Attrs and parsed Attrs
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.
(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?
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).
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.)
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>.
Created attachment 21091 [details] Proposed patch
Comment on attachment 21091 [details] Proposed patch r=me
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.