460156 - range.isPointInRange(range.endContainer,range.endOffset) returns false for TextNodes

Status: RESOLVED FIXED

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

Description

[Elia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1

Hi I just downloaded the new beta version of Firefox (3.1b1) and I noticed the behavior of the isPointInRange function changed since the last stable version (3.0.3). I don't know if this is really a bug or simply how the function will behave from now on.
Basically, before 3.1b1, the following call would return TRUE:

range.isPointInRange(range.endContainer,range.endOffset);

Now instead it returns false. To me it was more obvious the original behavior because endContainer and endOffset should describe the ending bound for the text range. 
I reported this so that if this is the new implementation and not a mistake or oversight people will now to update their code accordingly (knowing that it won't change again in the future).

Reproducible: Always

Steps to Reproduce:
1. Create a range with var range = document.createRange()
2. Set start and end to some nodes
3. Test by calling range.isPointInRange(range.endContainer,range.endOffset);
Actual Results:  
The call returns false.

Expected Results:  
The call used to return true.

Default theme.

Comment 1

[alexander :surkov (:asurkov)]

I think it's invalid bug because of bug 451376 comment #2 and bug 451376 comment #3:

ranges don't use closed intervals
http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html. It means the point is after range if the given node and offset are equal to end node and end offset of the range because
end bound isn't included into range

Comment 2

[Elia]

Then this will be default behavior from now on? Meaning that also release 3.1 will use this approach? Because it works the other way around in 3.0

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Then this will be default behavior from now on? Meaning that also release 3.1
> will use this approach? Because it works the other way around in 3.0

I think so.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I agree with Alex. The old behavior works strangely if you have
2 ranges, where the end point of the first is the start point of the second one.
The old behavior makes .isPointInRange() return true in both cases, the new
behavior clarifies that the point belongs to the latter range (as it belongs
per the spec)

Comment 5

[alexander :surkov (:asurkov)]

invalid then

Comment 6

[Elia]

Ok fine by me. I just wanted to be sure this was an actual implementation choice and not a mistake.

Comment 7

[alexander :surkov (:asurkov)]

thank you for watchfulness but why did you change the bug status, it wasn't intentional?

Comment 8

[Elia]

Uhm? I didn't change anything, I just posted the comment. Probably I didn't refresh the page after you changed the status and so I re-posted the old form values.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Jonas, any comments to this.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Actually we should first check end point, because currently point can be on
a collapsed range.
I know this is a change to the behavior, but that is IMO the only behavior which
makes sense and consistent with other range code.

Comment 11

[Elia]

Yes as I said I just wanted to be sure, I thought this was the fastest and more efficient way, so that other people with the same doubt can just do a search.

Comment 12

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

Elia: Absolutely, filing a bug was the right thing to do.

I do agree with comment 1 and comment 4. It makes sense that a point only belongs to one of two adjacent ranges. I don't really have super strong feelings either way though.

> The old behavior makes .isPointInRange() return true in both cases, the new
> behavior clarifies that the point belongs to the latter range (as it belongs
> per the spec)

What part of the spec says that?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

There is no spec for isPointInRange()

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> What part of the spec says that?
er, you mean about the point. Well that is how range works elsewhere. For example when modifying DOM etc.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

<root><element1/><element2></root>
      ^---------^

start and end container is root and start offset 0 and end offset 1.
if someone then asks if root:1 is in range, that points to element2 (IMHO), so
it shouldn't belong to range.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

But because isPointInRange() isn't specified anywhere, one could argue whatever.
Alex, just being curious, if we kept FF3 isPointInRange() behavior, would it be
difficult to fix Bug 451376.

Comment 17

[Elia]

Just for completion I'm posting here a simple function to determine whether ranges are exclusive (new behavior) or inclusive (old behavior) for endOffsets.

----- CODE ------

function isFirefoxRangeExclusive()
{
	var appInfo = Components.classes["@mozilla.org/xre/app-info;1"]
	                        .getService(Components.interfaces.nsIXULAppInfo);
	var versionChecker = Components.classes["@mozilla.org/xpcom/version-comparator;1"]
	                               .getService(Components.interfaces.nsIVersionComparator);
		
	if(appInfo && versionChecker &&
		 appInfo.ID == '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}' &&
		 versionChecker.compare(appInfo.version, '3.1b1') >= 0) 
	{
	  return true;
	}
	
	return false;
}

----- CODE ------

Because I'm interested only in Firefox this function just checks firefox version but I'm quite sure there's a way to check directly the toolkit/core internal version so that the function is cross-product. Otherwise one can just modify the IF to check for other products by their id and their version supporting this new behavior.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I may have to change my mind. After checking what webkit does and what
is mentioned in the web, the end point is in range :(

This old page has pretty good explanation
http://www.dotvoid.com/2001/03/using-the-range-object-in-mozilla/
isPointInRange( refNode, offset ) – This method returns true if the point described by the refNode node and an offset within the node falls between or equal to the boundary-points of the range.

Although I don't like the old behavior, but
because of backward compatibility, we may have to backout bug 451376.
Sorry Alex. And thanks Elia bringing up this issue. I should have thought about 
it some more when reviewing bug 451376.

Comment 19

[Elia]

Oh, ok...
Well, then I'll wait until this is definitive before updating my code, although the update is really just a couple of variable decrements and the function above.

Just to add my opinion, I really prefer the inclusive behavior, when start and end are both included into range. The reason for this is that it makes a bit more sense and it feels more what one would expect, to have both containers treated in the same way: you either make both bounds exclusive or both inclusive, mixing up things creates confusion.

Every method has its own disadvantages: using inclusive bounds means you can't create a range to describe a collapsed space between nodes; making them exclusive (either of the two) makes it impossible to select the root node's children completely.
I believe the only real solution would be to allow either start or end container to be NULL or offsets to be negative for describing an invalid/collapsed state. For instance startContainer and startOffset describe the position and if endContainer is NULL (preferred method) or endContainer is equal to startContainer and endOffset is -1, then the range is collapsed. Something like that.

Although I do agree it should be as per standard to have a more homogeneous behavior.

Comment 20

[alexander :surkov (:asurkov)]

So let's consider an example:

<body><p id="p1">One</p><p id="p2">Two</p></body>

var range = document.createRange();
range.setStart(document.body, 0);
range.setEnd(document.body, 1);

// true - start point is included into range, it means html:p@id="p1" is included entirerly into range
alert(range.isPointInRange(document.body, 0));

// true - html:p@id="p1" is included into range and it's content is in range
alert(range.isPointInRange(document.getElementById("p2"), 1));

// true - end point is included into range.  should it mean html:p@id="p2" is included entirerly into range?
alert(range.isPointInRange(document.body, 1));

// false - html:p@id="p2" is included into range but it's content is not in range
alert(range.isPointInRange(document.getElementById("p2"), 1));

If the range is collapsed then it's empty and no one point belongs to range I think.

Otherwise could you say what does "point is inside range" mean? Because it seems this term is not well defined.

Comment 21

[alexander :surkov (:asurkov)]

example above doesn't make sense actually. But any way we need to define "point is inside range" term.

Comment 22

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

So I always thought of isPointInRange as testing an actual point, not a node. So for the range

<root> <elem1/> <elem2/> </root>
      ^--------^

calling isPointInRange(root, 1) tests if the '|' is in the range

<root> <elem1/>|<elem2/> </root>
      ^--------^

Which obviously is ambigious. So if you have two ranges:

<root> <elem1/>|<elem2/> </root>
      ^--------^
               ^--------^

Which range does the point belong in. I'd argue that you can make a case for all of "first", "second", "both", and "neither".

Similarly for
<root> <elem1/>|<elem2/> </root>
               ^
you could argue both that the point is in the range and that it is not. It is really a matter of being inclusive or not. And which end it is being inclusive in.

The only API that is available in the spec that deals points is compareBoundaryPoints, which allows you to specify two points (by using range endpoints). That API returns a specific value if two points are on the same position, so it is neither inclusive nor exclusive.


The only other argument I can see is that if you insert at a specific point using:

pointContainer.parent.insertBefore(pointContainer.parent.childNodes[pointIndex+1])

For such code it would make sense if the start point was inclusive but the end point not.

Hence I could go either way :) But there is definitly truth to that breaking compat can hurt.

Comment 23

[alexander :surkov (:asurkov)]

Ok. Let's we have a range

h e l l o
  |       |

range is [1, 4). It means 1 is included into range because 'e' belongs to
range, 4 is excluded from range because 'o' doesn't belongs to range. Therefore
ranges aren't closed intervals, i.e. left bound is included into range, right
bound isn't included.

We can define two definitions of "point in range" term:

1) point is in range's interval. It means offset >= 1 && offset < 4. If we have
collapsed range then range is empty and no one point is in range's interval.

2) point is in closure of range's interval. It means offset >= 1 && offset <=
4. If we have collapsed range then range is empty but it's closure consist of
one point and this point is "point in range".

I think the first definition is more natural. Otherwise the method should be
named as isPointInRangeClosure. If webkit follows the second definition then
possibly we should discuss this with them?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> But because isPointInRange() isn't specified anywhere, one could argue
> whatever.
> Alex, just being curious, if we kept FF3 isPointInRange() behavior, would it be
> difficult to fix Bug 451376.

I don't think so. When isPointInRange() returns true then I should check additionally end container and end offset. Just bug 451376 seems to be good case when isPointInRange should return false for the end bound of the range.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> (In reply to comment #16)
> > But because isPointInRange() isn't specified anywhere, one could argue
> > whatever.
> > Alex, just being curious, if we kept FF3 isPointInRange() behavior, would it be
> > difficult to fix Bug 451376.
> 
> I don't think so. When isPointInRange() returns true then I should check
> additionally end container and end offset. Just bug 451376 seems to be good
> case when isPointInRange should return false for the end bound of the range.

There we have the case:

*Giving 5 gives back startOffset: 5, endOffset:8, and the attributes for that
run.
*Giving 8 gives back StartOffset: 5, endOffset: 8, and attributes for that run.

this is because we have [5, 8), [8, 15) ranges, so since isPointInRange uses closed intervals then it works with [5, 8], [8, 15] intervals. Therefore when they pass 8 offset then we get the range [5, 8) instead of [8, 15).

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #22)
> So I always thought of isPointInRange as testing an actual point, not a node.
Yes, this way the API almost makes sense.

(In reply to comment #25)
> this is because we have [5, 8), [8, 15) ranges, so since isPointInRange uses
> closed intervals then it works with [5, 8], [8, 15] intervals. Therefore when
> they pass 8 offset then we get the range [5, 8) instead of [8, 15).
I'm not sure what gets the range? Are you talking about accessibility.
if you pass 8, from that you could should get both [5, 8], [8, 15], but for some reason you handle only [5, 8]?

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> (In reply to comment #25)
> > this is because we have [5, 8), [8, 15) ranges, so since isPointInRange uses
> > closed intervals then it works with [5, 8], [8, 15] intervals. Therefore when
> > they pass 8 offset then we get the range [5, 8) instead of [8, 15).
> I'm not sure what gets the range? Are you talking about accessibility.
> if you pass 8, from that you could should get both [5, 8], [8, 15], but for
> some reason you handle only [5, 8]?

Example is "This iss a test", "iss" is misspelled word. Here we need to return offsets for misspelled word. Since isPoinInRange(8) on range for misspelled word returns true then we return its offsets, i.e. 5 and 8.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #27)
> Example is "This iss a test", "iss" is misspelled word. Here we need to return
> offsets for misspelled word.
But are you talking about accessibility or spellchecker?
We can hopefully change the code to return "what-is-inside-range" and leave 
isPointInRange to behave like in FF3

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> (In reply to comment #27)
> > Example is "This iss a test", "iss" is misspelled word. Here we need to return
> > offsets for misspelled word.
> But are you talking about accessibility or spellchecker?

I care about accessibility, we expose misspelled ranges to AT (http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#2215)

> We can hopefully change the code to return "what-is-inside-range" and leave 
> isPointInRange to behave like in FF3

Sorry, didn't get you.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

Alex, does this work for a11y?

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> Created an attachment (id=343544) [details]
> possible patch
> 
> Alex, does this work for a11y?

Ok, it should but I didn't test yet. So you decided to rollback to old behaviour? Then could we consider to introduce additional methods to cover a11y needs (since it sounds as common task)?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Yes, we could perhaps add some new method (at least to C++) which excludes end boundary point. But lets take this patch first.
I was trying to run a11y mochitest, but it fails with or without the patch
(same number failures in both cases).

Comment 33

[Elia]

I agree with maintaining the old behavior for the current method and possibly adding another very similar method like isPointWithinRange or isPointWithinBounds.

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 343544 [details] [diff] [review]
possible patch

r=me, please file bug to add new methods and put XXX section into a11y code pointing to that bug. Also please extend comments for range methods to define "point in range" term.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comments