Closed Bug 6052 Opened 21 years ago Closed 18 years ago

Node::nodeValue returning empty string instead of null

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: dbaron, Assigned: jst)

References

()

Details

(4 keywords, Whiteboard: [XPCDOM] relnote-devel)

Attachments

(2 files)

On an Element, Node::nodeValue is returning the empty string instead of null.
Run the "passive" tests on the above page.

I checked, and null is printing correctly, so it's not a toString or comparison
problem.

MSIE5 does get this correct.
Status: NEW → ASSIGNED
Target Milestone: M7
Hmm...you'll probably find that's true for all of the node types whose
nodeValue should be null.

I have a related bug that discussed how the idl->XPCOM+JS glue conversion
process converts DOMStrings to nsString references instead of nsString pointers.
Because of that, I can't return null for a string return type. This should
change (possibly in M7) since the nsIString interface is coming together.
Target Milestone: M7 → M10
Looks like there's still some contention internally over what the correct type
should be for strings in IDL. Pushing this one off till M10.
Blocks: 1994
Depends on: 1663
Can't solve this one till the IDL->XPCOM code generator stops using nsString
references. Adding the dependency in and leaving the M10 milestone, though I
suspect it may be pushed off till later.
Lots of upcoming string stuff in the DOM world, I hear, but I'm going to move
this to post-beta.  brendan+vidur, does the [shared] wstring plan provide for a
way to fix this?
Target Milestone: M10 → M14
HTML DOM bugs are M11/P2 for Vidur.
Target Milestone: M11 → M15
I think shaver moved this one incorrectly to M11 (even though he meant
post-beta). Yes, the [shared] wstring plan will fix this problem. Moving to M15.
Since we need to wait for the nsIString work to happen first, this will need to 
be pushed off.
Target Milestone: M15 → M17
This is also true for localName, namespaceURI and prefix. According to the
specification they should *always* return null for everything except Attr and
Element. Attr and Element should return null for everything created from the
Non-NS Level 2 methods.

localName has the problem that instead of returning null it is returning the
nodeName for newly created Attr, CDATASection, Comment, Text, DocumentFragment,
DocumentType, Element and ProcessingInstruction. Should this be a separate bug?

See http://www.mindspring.com/~bobclary/bugzilla/report3.html for a summary and
http://www.mindspring.com/~bobclary/TestFrame_dom_core.html for live tests (M17+
required)


Nominating for beta3, we must have a way to pass null strings through the DOM
APIs for beta3, should be trivial once scc fixes our string classes to support this.
Keywords: correctness, nsbeta3
OS: other → All
Hardware: Other → All
Target Milestone: M17 → M18
Marking nsbeta3+ and re-assigning to jst.
Assignee: vidur → jst
Status: ASSIGNED → NEW
Whiteboard: [nsbeta3+]
Whiteboard: [nsbeta3+] → [nsbeta3+] [trivial fix]
PDT thinks that in order to justify a P2 priority, we'd like know what the user
impact of this ancient bug is. Does it really matter to users?
Whiteboard: [nsbeta3+] [trivial fix] → [nsbeta3+] [trivial fix][PDT needs info]
I believe this bug should be fixed, especially if it is trivial to do so.

First, the W3C DOM Core is the standard reference for everyone using the DOM. 
Having Mozilla return a null string instead of null will result in continual 
confusion now and in the future as more and more people code the DOM using the 
W3C DOM Core specification. Additionally, this bug will show up where ever the 
specification says a String value should return null, not just in nodeValue. 
Namespace related attributes are an example. This bug is not restricted to the 
DOM Core. It can appear anywhere in the DOM as well.

Second, conformance to the W3C DOM specification is important for cross-
platform scripting. I would like to be able to use the same code for Mozilla, 
IE and server-side scripts.  The attractiveness of the W3C DOM is that it is a 
*standard* API.  I don't want to get back into the situation where my scripts 
have browser/platform checks scattered through out them...  been there, done 
that, no thanks!

Finally, I can't speak to the priority issue or to the effect fixing this bug 
will have on existing code.  I just wanted you to know that it is important to 
me that Mozilla adhere to the standard as closely as possible. Given the choice 
between a standards compliant browser and a non-standards compliant browser I 
will choose the compliant browser everytime. But given the choice between two 
non-standards compliant browsers, what choice do I make?
The root of this is a fundamental flaw in our string handling in the DOM
implementation, this affects a lot of widely used methods, such as
getAttribute(), this is not only a problem with Node::nodeValue. We're finally
close to having a string implementation that can handle this. Without this bug
fixed, existing pages that rely on *correct* DOM behavior will break!
Status: NEW → ASSIGNED
Mass update of qa contact
QA Contact: gerardok → janc
PDT thinks this is a P3, but if this is an issue on top100 sites, please let us 
know right away.
Priority: P2 → P3
Whiteboard: [nsbeta3+] [trivial fix][PDT needs info] → [nsbeta3+] [trivial fix][PDTP3]
We will plan on fixing this in a future release. In the meantime, for developers
who wish to create content that complies with the intended functioning of the
DOM1 spec and works cross-browser with IE , the workaround is to
check whether you're running on Gecko via client sniffing, and insert a
conditional code fork in your JavaScript so that when you're running on Gecko,
you check the return value you get from executing this call, and if it's the
empty string, you manually change it to null.

Also, content developers should *not* write content that depends on the current
incorrect return value of empty string, as we will fix this bug in a future
release and such code would then break.
Keywords: relnote
Whiteboard: [nsbeta3+] [trivial fix][PDTP3] → [nsbeta3-] [trivial fix][PDTP3]
Target Milestone: M18 → Future
Adding helpwanted keyword.  Maybe scc and I can get this done.  Speaking from
bitter experience with the "level 0 DOM" (JS1.0, Nav 2.0, before there was a
standard, and still very much a de facto standard), telling people not to count
on functional behavior you ship is pretty much an exercise in futility.

/be
Keywords: helpwanted
Whiteboard: [nsbeta3-] [trivial fix][PDTP3] → [nsbeta3-] relnote-devel [trivial fix][PDTP3]
D'oh!  I guess this missed Netscape 6 RTM cleanly.  What's the status for 
mozilla0.9 (for which I'm nominating)?

/be
Keywords: mozilla0.9
I (and jband) need scc's new string code to fix this but AFAIK that code is
close to done and once that's checked in this should be fairly easy to fix.
Aiming for mozilla0.9.
Priority: P3 → P2
Whiteboard: [nsbeta3-] relnote-devel [trivial fix][PDTP3] → [XPCDOM] relnote-devel
Target Milestone: Future → mozilla0.9
jst: per your request here are additional attributes that may be null.

Node.localName: for nodes other than Element or Attr localName should always be
null. Currently, localName contains other stuff (not "").  See
http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-NodeNSLocalN

Node.namespaceURI: for nodes other than Element or Attr namespaceURI should
always be null. Currently, namespaceURI is "". Note that the spec explicitly
says that namespaceURI == "" is different from namespaceURI == null. See
http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-NodeNSname

Node.prefix: See http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-NodeNSPrefix

Node.nodeValue: nodeValue can be null (see DocumentFragment) but currently is
reported as "". See http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-F68D080

Hope that helps.
bc
Target Milestone: mozilla0.9 → mozilla0.9.1
Keywords: dom1
Component: DOM Level 1 → DOM Core
See bug #67876.
since the page on www.mindspring.com/~bobclary/ is no longer available,
attaching the test cases for localName, namespaceURI, nodeValue, prefix. Adding
testcase keyword.
Keywords: testcase
QA contact Update
QA Contact: janc → desale
Blocks: 75185
No longer blocks: 1994
moving to TM of 0.9.2 per PDT triage (you can check it into 0.9.1 until Friday,
18/May/01 or into 0.9.2 after the tree opens)
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
Moving to mozilla0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Target Milestone: mozilla0.9.3 → mozilla0.9.4
I just tested this with 0.9.3 on a MacOS 9.1 box, and this still is a bug.  It
fails on the tests provided by Netscape on the Node.localName,
Node.namespaceURI, Node.nodeValue, and Node.prefix in the Netscape test document
tc_cdot501.  This test document is available here: 

http://developer.netscape.com/evangelism/tools/testsuites/tc.html?txtTitle=DOM%20Core%20Level%202%20Conformance%20Tests&txtApiPath=w3c-dom-core-2/&txtTestCases=tc&txtInvariantApiTestFrame=tc_invariantTestFrame&txtMutantApiTestFrame=tc_mutantTestFrame

If this link doesn't work for you, try clicking on the DOM Core Level 2
Conformance test link on this page: 
http://developer.netscape.com/evangelism/tools/testsuites/
Moving to mozilla0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.5 → mozilla0.9.6
The patch I attached fixes dbaron's tests. It does not fix every case where we
should return a null string in nsGenericDOMDataNode::GetNodeValue, because that
fix is more involved.
I'm more worried about the fallout from doing this change: what happened in
composer with getAttribute returning null might happen again with this patch. I
did check all the places where we get nodeValue, but I can't say for sure that
there won't be any problem. I did do some random browsing, mailing & composing
though and didn't see any problem.
Is this patch in the Oct 27 nightly build (2001102708)?  If so, then it doesn't
fix the namespaceURI property bug for element nodes.  Instead, it seems to 
have changed the way the namespaceURI property is broken.

The correct behavior is described as follows:
If <foo> is the root element for a document, then you should have

<foo>            --  node.namespaceURI=null (null for the namespaceURI)  
<foo xmlns="">   --  node.namespaceURI=""   (an 'empty' string for namespaceURI)

With the current build (2001102708) _both_ of the above produce a null
value for namespaceURI, which is incorrect.

Previously (Moz 0.9.5), the bug was different. In that case both examples
produce an 'empty' string value for namespaceURI, which is also wrong 
(but in a different way). 
I see. Indeed we always return a null string for namespaceURI on elements.
Should be easy to fix. Thanks.
[Additional hopefully useful comments]

Whatever is done, we should probably note the following weirdness
associated with it [From DOM 2 (core) spec, Sect 1,1,8, paragraph 3 -
maybe as a comment in the code referencing this location in the spec?]:

    "Note that because the DOM does no lexical checking, the empty 
    string will be treated as a real namespace URI in DOM Level 2 
    methods. Applications must use the value null as the namespaceURI 
    parameter for methods if they wish to have no namespace."

The reason for this note is because the namespace specification 
[Section 5.2] states the following:

    "The default namespace can be set to the empty string. This has 
    the same effect, within the scope of the declaration, of there 
    being no default namespace."

So that although the DOM says that <foo> and <foo xmlns=""> are 
different (one having namespaceURI=null and the other 
namespaceURI=""), the namespace spec says that the two are
equivalent, and declare that there is 'no' default namespace.

This brings up the question of where NAMESPACE_ERR exceptions should
be thrown -- should they be thrown when parsing XML to form a DOM,
or only when manipulating using the DOM?  Right now I don't think
these exceptions are ever thrown .... certainly I can use the DOM
to parse example documents that should throw NAMESPACE_ERR, such as
and of the following 3 examples::

<?xml version="1.0" ?>
<!-- This should fail - you're not allowed to declare
     a no-namespace value for a prefixed attribute namespace declaration
     like xmlns:foo  -->
<xmltest xmlns:foo=""
         xmlns:html="http://www.w3.org/TR/REC-html40" 
         foo:name="value" >
 <html:script src="tree-traverse.js" />
</xmltest>

<?xml version="1.0" ?>
<!-- This should fail - the two prefixes map onto the same 
     namespace, so the two attributes conflict.
     -->
<xmltest xmlns:abc="http://www.foo.org"  
         xmlns:html="http://www.w3.org/TR/REC-html40" 
         xmlns:foo="http://www.foo.org"
         foo:name="value"
	 abc:name="value2">
    <html:script src="tree-traverse.js" />
</xmltest>

<?xml version="1.0" ?>
<!-- This should raise a NAMESPACE_ERR exception because
     the Qname foo:name uses a prefix that has no declared namespace" -->
<xmltest foo:name="value"  xmlns:html="http://www.w3.org/TR/REC-html40" >
 <html:script src="tree-traverse.js" />
</xmltest>



Pushing to mozilla0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Fixed, please open a new bug on the element.namespaceURI issues.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
verified fixed, all the tests provided passed. build 2001-12-14-09
Status: RESOLVED → VERIFIED
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.