Closed Bug 737565 Opened 12 years ago Closed 12 years ago

Range.comparePoint() should throw exceptions per spec

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(1 file)

Spec:

"""
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.
"""
http://dvcs.w3.org/hg/domcore/raw-file/tip/dom-core.html#dom-range-comparepoint

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>
<script>
var range = document.createRange();
document.documentElement.textContent = "didn't throw";
try { range.comparePoint(document.doctype, 0) }
catch(e) { document.documentElement.textContent = e }
</script>

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>
<script>
var range = document.createRange();
document.documentElement.textContent = "didn't throw";
try { range.comparePoint(document, 75) }
catch(e) { document.documentElement.textContent = e }
</script>

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)....
Attached patch Patch v1Splinter Review
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
Status: NEW → ASSIGNED
Attachment #607674 - Flags: review?(bugs)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
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 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+
Try run for f3885a8a6a68 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f3885a8a6a68
Results (out of 186 total builds):
    exception: 4
    success: 149
    warnings: 19
    failure: 13
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-f3885a8a6a68
 Timed out after 12 hours without completing.
Whiteboard: [autoland-in-queue]
http://hg.mozilla.org/integration/mozilla-inbound/rev/cb1d22d54346
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/cb1d22d54346
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: