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

RESOLVED FIXED in mozilla0.9.5

Status

()

P2
normal
RESOLVED FIXED
18 years ago
17 years ago

People

(Reporter: jasonkarldavis, Assigned: fabian)

Tracking

Trunk
mozilla0.9.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

18 years ago
<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.
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()?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
(Assignee)

Comment 2

17 years ago
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.
Keywords: patch, review
Whiteboard: [HAVE FIX]
(Assignee)

Comment 3

17 years ago
Created attachment 47267 [details] [diff] [review]
First attempt to make GetText() work correctly
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).
(Assignee)

Comment 5

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

Comment 6

17 years ago
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 ;-)
Keywords: patch, review
Whiteboard: [HAVE FIX]
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 :-)
(Assignee)

Comment 8

17 years ago
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.
Assignee: jst → hidday
Keywords: patch, review
Priority: -- → P2
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.5
(Assignee)

Comment 9

17 years ago
Created attachment 47536 [details] [diff] [review]
Patch to implement the desired behavior for getText
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).
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 :-)
(Assignee)

Comment 12

17 years ago
Created attachment 47638 [details] [diff] [review]
Patch #2 to implement the correct GetText function for anchors.
(Assignee)

Comment 13

17 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 14

17 years ago
Created attachment 47640 [details]
Complete testcase so that the QA can verify this bug as fixed.
Attachment #47638 - Flags: superreview+
Attachment #47267 - Attachment is obsolete: true
Attachment #47536 - Attachment is obsolete: true
(Assignee)

Comment 15

17 years ago
Created attachment 48052 [details] [diff] [review]
Third revision of GetText(). Fix bad error handling.
(Assignee)

Comment 16

17 years ago
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.
Attachment #47638 - Attachment is obsolete: true
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 on attachment 48052 [details] [diff] [review]
Third revision of GetText(). Fix bad error handling.

What sicking said, sr=jst
Attachment #48052 - Flags: superreview+
Attachment #48052 - Flags: review+
Fix checked in... Thanks for the patch Fabian!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.