85448 - document.links[X].text only returns the first child node value

Status: RESOLVED FIXED

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

Description

[Jason Davis]

<a href="javascript://" onclick="alert(this.text)">Works</a>
<br />
<a href="javascript://" onclick="alert(this.text)">Probably <span>doesn't</span>
work</a>

The first link when clicked alerts "Works". But the second link, when clicked,
only alerts "Probably". (In Mozilla 0.91 build 2001060703)

Testing it in NS4, the second link alerts "work". (Although I was expecting
"Probably doesn't work", there is still inconsistency between the browsers in
this matter.)

In Mozilla, when the first child of the <a> node isn't a text node, it will
alert '', which tells me that the text property of an anchor element doesn't
work as its supposed to.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK... First of all, looks like this is not part of the W3C DOM spec.  The
property is defined in
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/html/nsIDOMNSHTMLAnchorElement.idl

Is this part of any W3C spec at all?

Secondly,
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLAnchorElement.cpp#639

does indeed just grab the first child and then return its nodeValue.  If this is
not a W3C-defined spec, would not compatibility with NS 4.x be what we should
aim for here?  So would it not make more sense to use LastChild() instead of
FirstChild()?

Comment 2

[Fabian Guisset]

The patch I'm about to attach returns the full text that is in the link, not
just the first (or the last) text node. jst (or bz), I just have some doubts
about my refcounting skills so please pay particular attention to that if you
take a look at the patch, particularly the static cast.
The code is borrowed from nsRange.cpp so it should be more or less correct.

Comment 3

[Fabian Guisset]

Attachment: First attempt to make GetText() work correctly

Comment 4

[Johnny Stenback (:jst)]

Link.text is an NS4.x:ism, if we make any changes to how it works we should make
it work like it does in NS4.x, that is, grab the last text node in the link and
get the value out of that node (we should make sure this is what 4.x does before
we actually make these changes).

Comment 5

[Fabian Guisset]

Ok, I thought my solution was more correct wrt the method name, but let's go for
NS4 compat. Will attach patch tomorrow. 

Comment 6

[Fabian Guisset]

Ok this is more complicated than I thought it would be. I tested NS4.7 with
several tag combinations for this testcase. Here are some interesting results:
1) In general, it returns the last text node
2) If the last child is a tag and not a text node, then NS4 returns the deepest
text node in the children of the last child. For example:
<a href="javascript://" onclick="alert(this.text)">Level 1 <span> Level 2 <span>
Level 3 <span> Level 4 </span> </span> </span></a>
will return "Level4".
3) If there is no text node, i.e. the link is an image or something like that,
it returns "null".
Is there any way to find the NS4 code for this property? I'm not sure I have
gotten all the behaviors correctly.I'm also not sure this function is worth the
trouble of implementing every little quirk of NS4 ;-)

Comment 7

[Johnny Stenback (:jst)]

Fabian, I bet if you'd implement what you describe above we'd be close enough,
it makes sense from what I've heard about how the 4.x code works. The 4.x code
is available in lxr, but I'd recommend you stay away from that code :-)

Comment 8

[Fabian Guisset]

First attempt to implement the desired behavior. All the testcases I came up
with yield the same results in NS4 and Mozilla with this patch. Not sure what
the cost of an nsIContentIterator is, but I assume this attribute isn't called
in a tight loop too often so it doesn't really matter. Comments?
Taking the bug, trying for 0.9.5, P2, CC'ing jst.

Comment 9

[Fabian Guisset]

Attachment: Patch to implement the desired behavior for getText

Comment 10

[Boris Zbarsky [:bzbarsky]]

I assume the content iterator is cheaper than the alternative, which is to call 
GetLastChild recursively until we get a text node or nothing (if there is no text 
inside).

Comment 11

[Johnny Stenback (:jst)]

Thanks Fabian for the patch! Here's a few comments about it:

- You'll need some error checking around this call, it can fail:

+  NS_NewContentIterator(getter_AddRefs(iter));

Make it:

+  nsresult rv = NS_NewContentIterator(getter_AddRefs(iter));
+  NS_ENSURE_SUCCESS(rv, rv);

- I don't see a need for 'contentPtr' here:

+  nsIContent *contentPtr = NS_STATIC_CAST(nsGenericElement *, this);
+
+  // Initialize the content iterator with the children of the anchor
+  iter->Init(contentPtr);

this should be enough:

+  // Initialize the content iterator with the children of the anchor
+  iter->Init(this);

- 'tempString' is unnecessary in the patch, remove it and replace:

+      textNode->GetData(tempString);
+      aText += tempString;
+      break;

with:

+      textNode->GetData(aText);
+      break;

- The file you're modifying uses the brace-style:

  if (foo) {
    ...
  }

and not:

  if ( foo )
  {
    ...
  }

or any variation thereof, please follow the same style used in the file where
you're making changes to the code.
- If you make the changes above, you can change 'nsresult res = ...' to 'rv =
...' ('rv' is the prefered name for nsresults when used like this).

Fix the above and you'll be real close to having an sr=jst :-)

Comment 12

[Fabian Guisset]

Attachment: Patch #2 to implement the correct GetText function for anchors.

Comment 13

[Fabian Guisset]

New patch addresses the review comments I think.

+  nsIContent *contentPtr = NS_STATIC_CAST(nsGenericElement *, this);
+
+  // Initialize the content iterator with the children of the anchor
+  iter->Init(contentPtr);

this should be enough:

+  // Initialize the content iterator with the children of the anchor
+  iter->Init(this);

Yep, I woke up in the middle of the night shouting "Damn, why did I cast it!"
but you had already reviewed it ;-)
Accepting bug.

Comment 14

[Fabian Guisset]

Attachment: Complete testcase so that the QA can verify this bug as fixed.

Comment 15

[Fabian Guisset]

Attachment: Third revision of GetText(). Fix bad error handling.

Comment 16

[Fabian Guisset]

Comment on attachment 47638 [details] [diff] [review]
Patch #2 to implement the correct GetText function for anchors.

On sicking's request, changed the error handling of iter->Prev() to the simpler NS_ENSURE_SUCCESS. That will teach me to copy code. Re-requesting sr=, sorry for the trouble.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

just a couple of nits, no need to attach a new patch

Add two NS_ENSURE_SUCCESS after the first iter->Last() and iter->Prev(). Also a 
couple of whitespace thingies:

+  while(curNode && (NS_ENUMERATOR_FALSE == iter->IsDone()) )
+  {

should be

+  while(curNode && (NS_ENUMERATOR_FALSE == iter->IsDone())) {

and

+    nsCOMPtr<nsIDOMText> textNode( do_QueryInterface(curNode) );

should be

+    nsCOMPtr<nsIDOMText> textNode(do_QueryInterface(curNode));


other then that r=sicking. I'll also offer to be your checkin bitch

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 48052 [details] [diff] [review]
Third revision of GetText(). Fix bad error handling.

What sicking said, sr=jst

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fix checked in... Thanks for the patch Fabian!