Range.comparePoint() should throw exceptions per spec

RESOLVED FIXED in mozilla14



DOM: Core & HTML
5 years ago
4 years ago


(Reporter: ayg, Assigned: ayg)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment)


3. If node is a doctype, throw an "InvalidNodeTypeError" exception and terminate these steps.

4. If offset is greater than node's length, throw an "IndexSizeError" exception and terminate these steps.

Gecko doesn't throw in either of these cases.  I mandated throwing here in the spec to match Chrome, as noted in the comments in the spec source:

 <!-- Firefox 9.0a2 doesn't throw.  It ignores the offset and returns true or
 false depending on whether the doctype itself is in the range.  This makes
 some sense, but it doesn't match how other Range APIs handle doctypes, and
 having the second argument mandatory but ignored is just weird.  Thus I go
 with Chrome 16 dev, although I can see the merit in how Gecko works here. -->

 <!-- Firefox 9.0a2 doesn't throw.  It seems to return true if the node is
 completely contained in the range, like with selectNode(), and false otherwise
 - even if all boundary points in the node are contained in the range, like
 with selectNodeContents().  This is weird, and inconsistent with other Range
 APIs, so I go with Chrome 16 dev.  This is probably an authoring bug, so it's
 best to throw anyway. -->

Test-case for doctypes:

data:text/html,<!doctype html>
var range = document.createRange();
document.documentElement.textContent = "didn't throw";
try { range.comparePoint(document.doctype, 0) }
catch(e) { document.documentElement.textContent = e }

Gecko and Opera Next 12.00 alpha are "didn't throw".  Chrome 19 dev is "Error: INVALID_NODE_TYPE_ERR: DOM Range Exception 2".  IE10 Developer Preview doesn't support the method at all.

Test-case for excessive offset:

data:text/html,<!doctype html>
var range = document.createRange();
document.documentElement.textContent = "didn't throw";
try { range.comparePoint(document, 75) }
catch(e) { document.documentElement.textContent = e }

Gecko/Opera are "didn't throw", Chrome is "Error: INDEX_SIZE_ERR: DOM Exception 1".

This is causing test failures in <http://w3c-test.org/webapps/DOMCore/tests/approved/Range-comparePoint.html>.
Web compat impact?  Moving from "don't throw" to "throw" has a tendency to cause web compat problems, even if other browsers are in the "throw" bucket (due to browser sniffing)....
Created attachment 607674 [details] [diff] [review]
Patch v1

Well, here's a patch that fixes the bug, if we want to fix it.  If this is WONTFIX, I'll change the spec to match Gecko instead, although IMO Gecko doesn't make sense here.  But I'm guessing there's not much web compat risk.  It would have to be Gecko-specific code that does something that makes little sense to start with.
Assignee: nobody → ayg
Attachment #607674 - Flags: review?(bugs)
Whiteboard: [autoland-try:-u all]


5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 3

5 years ago
Autoland Patchset:
	Patches: 607674
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=f3885a8a6a68
Try run started, revision f3885a8a6a68. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=f3885a8a6a68

Comment 4

5 years ago
Comment on attachment 607674 [details] [diff] [review]
Patch v1

We should try this, and change the spec if something breaks.
Attachment #607674 - Flags: review?(bugs) → review+

Comment 5

5 years ago
Try run for f3885a8a6a68 is complete.
Detailed breakdown of the results available here:
Results (out of 186 total builds):
    exception: 4
    success: 149
    warnings: 19
    failure: 13
    other: 1
Builds (or logs if builds failed) available at:
 Timed out after 12 hours without completing.


5 years ago
Whiteboard: [autoland-in-queue]
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla14
Last Resolved: 5 years ago
Resolution: --- → FIXED
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.