186540 - document.createRange() should create a usable Range object

Status: VERIFIED FIXED

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

Description

[Laurens van den Oever]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20021222
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20021222

If there is no selection, reading any property on the range object returned by
document.createRange() will generate an error.

Reproducible: Always

Steps to Reproduce:
1. Enter the following url in the address bar:
   javascript:document.createRange().commonAncestorContainer;
2. See javascript console for the error.

Actual Results:  
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMRange.commonAncestorContainer]" 
nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
javascript:document.createRange().commonAncestorContainer; :: <TOP_LEVEL> ::
line 1"  data: no]

Expected Results:  
IE will return a range of length 0 positioned before the body element. I'm not
sure if this behaviour is desired, but at least it returns a queryable range object.

I would be happy if the commonAncestorContainer == null or if there would be
some clean way to find out that the range object is invalid.

Comment 1

[Boris Zbarsky [:bzbarsky]]

caillon, you know anything about this code?  

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Um, this is a glaring fault in our range implementation.  Our impl has it so
that ranges must first be initialized ("positioned") before they can be used. 
However this is not the case per the spec.

   The initial state of the Range returned from [document.createRange()] is
   such that both of its boundary-points are positioned at the beginning of
   the corresponding Document, before any content. In other words, the
   container of each boundary-point is the Document node and the offset
   within that node is 0. 
http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html#Level-2-Range-Creating

I found one particular comment quite interesting in nsRange.cpp:

202   PRBool isPositioned;
203   nsresult err = ((nsRange*)aRange)->GetIsPositioned(&isPositioned);
204   // Why do I have to cast above?  Because GetIsPositioned() is
205   // mysteriously missing from the nsIDOMRange interface.  dunno why.

The "why" is because all ranges are positioned: they start out that way and
unless I misread something in the spec, cannot become unpositioned.  setStart()
and friends all take non-null arguments, which I might add seem to not throw if
the node is null (which is IMO the only thing that is mysterious about this
portion of the spec). So this method should really be returning true all the time.

Anyway.... the real bug is that document.createRange() does not create a usable
Range object.  All you can do once you have created the range is a few things
which "initialize" it, though that is not necessary per the spec.  It should be
usable out of the box.

The fix is to update the callers of NS_NewRange and friends to pass in an
nsIDOMDocument, and that sent on to the nsRange constructor where the proper
defaults per the spec are set up.  GetIsPositioned(), mIsPositioned, etc. can
then be removed from the class, as well as a bunch of checks throughout the file.

I am tentatively taking this for now since I will try and work on it after the
new year, but if anyone (kin, jfrancis?) wants to grab this bug, by all means go
ahead since it may be a while before I get to it.

Comment 3

[Joe Francis]

hmmm.  I'm pretty sure that the range spec wasn't this way back when we wrote
the code in nsRange.cpp.  

One thing we don't want to do is to change the Range implementation to require a
document passed in.  Ranges work on things that aren't documents, and that is
very useful.  

It is createRange in DocumentRange that needs to change, not anything in Range.
 createRange should create a range, and then fetch the document root and
initialize the created range appropriately.  Don't change Range itself to
require a document: it shouldn't.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 158182 has been marked as a duplicate of this bug. ***

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Make this work from DOM calls

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 148102 [details] [diff] [review]
Make this work from DOM calls

r+sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 245668 has been marked as a duplicate of this bug. ***

Comment 8

[Boris Zbarsky [:bzbarsky]]

Chris, what remains to be done here?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148102 [details] [diff] [review]
Make this work from DOM calls

I think this should go on the branch.. it's very safe and makes createRange
much more usable.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Nothing else needs to be done.  I just forgot to mark it fixed.  I don't mind
landing it on the branch.

The backend work I outlined in comment 2 should be done at some point, but can
be shifted to a different bug.

(In reply to comment #3)
> One thing we don't want to do is to change the Range implementation to
> require a document passed in.  Ranges work on things that aren't documents,
> and that is very useful.

Yes, those "things that aren't documents" (assuming you meant that aren't _in_
documents) are in document fragments, and those also must have a document
associated with them.  They do most of the time currently in Mozilla.  I think
there is one caller which does not provide a document (at least there was only
one when I looked last year).  We could save some code forkage/duplication if we
just did this the right way and always created fragments and ranges with the
correct parentage.  And as a bonus, we will ensure we follow the relevant specs.
 ;-)

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 148102 [details] [diff] [review]
Make this work from DOM calls

a=asa (on behalf of drivers) for checkin to 1.7

Comment 12

[Asa Dotzler [:asa]]

Did this land on the branch? Time is nearly up.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> Did this land on the branch? Time is nearly up.

Based on bonsai graph, it did not:
it only went into Trunk r3.498 (= Mv1.8a1).

Christopher (or anyone):
Could you check it in Mv1.7 branch too ?

Comment 14

[Stefan Borggraefe]

bz, jst: Could one of you land this on the branch? I think this should be done
by someone who understands this patch and caillon doesn't seem to be available
at the moment.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148102 [details] [diff] [review]
Make this work from DOM calls

It seems that it's a little too late to take this for 1.7... we may still want
this for 1.7.x, though (note it was approved for 1.7 but not one managed to
land it on the branch...)

Comment 16

[Mike Kaply [:mkaply]]

Comment on attachment 148102 [details] [diff] [review]
Make this work from DOM calls

a=mkaply for 1.7.2

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed on the 1.7 branch.