Last Comment Bug 737565 - Range.comparePoint() should throw exceptions per spec
: Range.comparePoint() should throw exceptions per spec
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla14
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-20 12:02 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2013-04-04 13:53 PDT (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (6.02 KB, patch)
2012-03-20 12:57 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
bugs: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-03-20 12:02:36 PDT
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>.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-03-20 12:05:56 PDT
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)....
Comment 2 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-03-20 12:57:48 PDT
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.
Comment 3 Mozilla RelEng Bot 2012-03-20 13:04:16 PDT
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 Olli Pettay [:smaug] 2012-03-20 13:50:14 PDT
Comment on attachment 607674 [details] [diff] [review]
Patch v1

We should try this, and change the spec if something breaks.
Comment 5 Mozilla RelEng Bot 2012-03-21 06:53:45 PDT
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.
Comment 6 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-03-21 09:30:40 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/cb1d22d54346
Comment 7 Marco Bonardo [::mak] 2012-03-22 06:26:44 PDT
https://hg.mozilla.org/mozilla-central/rev/cb1d22d54346

Note You need to log in before you can comment on or make changes to this bug.