166419 - Mozilla's DOM Ranges don't support comment nodes

Status: RESOLVED FIXED

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

Description

[Aaron Leventhal]

If one tries to use nsIDOMRange::SetStart() or SetEnd() with a comment node, it
returns an error.

This is because nsRange::GetNodeLength() does not handle comment nodes.
It only supports node length for text nodes and cdata section nodes.

However, I noticed there are other places in dom ranges where comment nodes are
not supported. Is this by design or was it just never completed?

Comment 1

[Aaron Leventhal]

Attachment: Fixes nsIDOMRange::GetNodeLength() to support comment nodes via nsITextContent::GetTextLength()

Is this the right thing to do? Seeking r=/sr=

Comment 2

[Aaron Leventhal]

I already have a work around for my particular bug, so I'm going to pass this to
jrfrancis, so he can get all the places that should support comment nodes right.

On the other hand, if this doesn't seem to affect anyone it doesn't bother me if
it's futured.

Comment 3

[Boris Zbarsky [:bzbarsky]]

What does the DOM Range spec say on this?  This sounds to me like something we
should really fix....

Comment 4

[Joe Francis]

futuring for now.  if someone is affected by this please comment.

Comment 5

[Alex Vincent [:WeirdAl]]

I think my fix for bug 332148 handles this, if it's not fixed already.

Comment 6

[Alex Vincent [:WeirdAl]]

Someone want to submit a testcase beyond the one provided in bug 332148?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Alex, does that testcase test comment nodes?  If not, care to just write a test for this bug?

Comment 8

[Alex Vincent [:WeirdAl]]

bz: it does, but only for the extractContents and cloneContents cases.  IMHO, that's not enough.  :)

I'd really rather not write the tests myself (nor would I want to burden you, bz).

Comment 9

[Boris Zbarsky [:bzbarsky]]

So setStart/setEnd per comment 0 work?  That's what needs to be tested.

For what it's worth, this is now in my to-do list, since _someone_ has to do it.  I'd be quite happy if someone else gets to it, of course.

Comment 10

[Alex Vincent [:WeirdAl]]

(In reply to comment #9)
> So setStart/setEnd per comment 0 work?  That's what needs to be tested.

Yes, that's one thing the tests for extractContents covers.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sounds like this bug is covered then... what else needs testing?