31736 - Misleading documentation of nsIContent::GetAttribute()

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[rbs]

The return value of nsIContent::GetAttribute() is documented as
follows:

  /**
   * Get the current value of the attribute. This returns a form that is
   * suitable for passing back into setAttribute.
   *
   * <UL>
   *
   * <LI>If the attribute is not set and has no default value, return
   * NS_CONTENT_ATTR_NOT_THERE.
   *
   * <LI>If the attribute exists, but has no value, return
   * NS_CONTENT_ATTR_NO_VALUE.
   *
   * <LI>If the attribute has a value, empty or otherwise, set ret to
   * be the value, and return NS_CONTENT_ATTR_HAS_VALUE (== NS_OK).
   *
   * </UL>
   */
  NS_IMETHOD GetAttribute(PRInt32 aNameSpaceID, nsIAtom* aName, 
                          nsString& aResult) const = 0;

When reading the above documentation, the frame of mind is that 
it caters for the following cases (in no particular order).

(1) <tag attribute="value">
(2) <tag attribute="">
(3) <tag attribute>  <!-- eg. as in <td nowrap> -->
(4) <tag>

Trying to guess which is which from the documentation...
Af first, one would think that 

case 3 (no value) corresponds to:
   * <LI>If the attribute exists, but has no value, return
   * NS_CONTENT_ATTR_NO_VALUE.

case 1 (non-empty value) and 2 (empty value) correspond to:
   * <LI>If the attribute has a value, empty or otherwise, set ret to
   * be the value, and return NS_CONTENT_ATTR_HAS_VALUE (== NS_OK).

case 4 (attribute not specified) corresponds to:
   * <LI>If the attribute is not set and has no default value, return
   * NS_CONTENT_ATTR_NOT_THERE.

But as plausible as this guess may be, it is wrong, and has led to
MathML bug 31642. 

After checking with some printf(), it appears that:

<tag attribute="value"> means NS_CONTENT_ATTR_HAS_VALUE
<tag attribute="">      means NS_CONTENT_ATTR_NO_VALUE
<tag attribute>         ??
<tag>                   means NS_CONTENT_ATTR_NOT_THERE

It is somewhat confusing, especially given the mention of 
"empty or otherwise" in the documentation.

Comment 1

[Hixie (not reading bugmail)]

Well, 
   <tag attribute>
...is invalid XML and in SGML (i.e., HTML) actually stands for:
   <tag real-attribute=attribute>
e.g.
   <table border>
...actually means
   <table frame="border">
...(not border="1") and
   <td nowrap>
...actually means
   <td nowrap="nowrap">

I don't know if we are internally doing this correctly, but that would explain
the "??" next to "<tag attribute>". What did GetAttribute() actually return for
that case? Did the XML parser even accept it?

Comment 2

[rbs]

No, the XML parser didn't accept it. Also I don't know what the results would
be in HTML. But my guess is that someone reading the documentation would
understand "empty or otherwise" as I explained above, i.e., as attribute="",
or attribute="value". And this is not what the code is doing. On the contrary,
NS_CONTENT_ATTR_HAS_VALUE is returned only if the value is non-empty,
therefore it would be clearer to remove "empty or otherwise", and simply
replace it with "non-empty value".

Comment 3

[vidur (gone)]

True enough. I fixed the comment.

Comment 4

[rbs]

Marking VERIFIED.