Last Comment Bug 719503 - selection.deleteFromDocument() throws for collapsed selection
: selection.deleteFromDocument() throws for collapsed selection
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla13
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 10:56 PST by :Aryeh Gregor (away until August 15)
Modified: 2012-02-23 18:51 PST (History)
3 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.01 KB, patch)
2012-02-14 13:36 PST, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch v2 (3.08 KB, patch)
2012-02-22 07:40 PST, :Aryeh Gregor (away until August 15)
ayg: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-01-19 10:56:27 PST
Test-case:

data:text/html,<!doctype html>
<body>
<script>
try {
getSelection().collapse(document.head, 0);
getSelection().deleteFromDocument();
} catch(e) {
document.body.textContent = e;
throw e;
}
document.body.textContent = "success";
</script>

In IE9, Chrome 17 dev, and Opera Next 12.00 alpha, this outputs "success".  In Firefox 12.0a1 (2012-01-17), it outputs

"""
[Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsISelection.deleteFromDocument]" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)" location: "JS frame :: data:text/html,<!doctype%20html><body><script>try%20{getSelection().collapse(document.head,%200);getSelection().deleteFromDocument();}%20catch(e)%20{document.body.textContent%20=%20e;throw%20e;}document.body.textContent%20=%20"success";</script> :: <TOP_LEVEL> :: line 1" data: no]
"""

This causes test failures in <http://dvcs.w3.org/hg/editing/raw-file/tip/selecttest/deleteFromDocument.html>.  The specification requires the non-Gecko behavior, which is IMO less surprising:

"""
The deleteFromDocument() method must do nothing if the context object's range is null, and otherwise must invoke the deleteContents() method on the context object's range.
"""
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#dom-selection-deletefromdocument

deleteContents() on a collapsed selection is therefore a no-op.
Comment 1 :Aryeh Gregor (away until August 15) 2012-02-14 13:36:10 PST
Created attachment 597167 [details] [diff] [review]
Patch v1

Since I didn't have enough time left today to start on transitions/animations testing, and none of the transforms stuff left on my plate looks particularly easy, I figured I'd write a quick Selection patch to round out my day.  Turns out the behavior was actually odder than I thought: it only throws in some cases, and in other cases it does something that looks quite surprising.
Comment 2 Mozilla RelEng Bot 2012-02-14 13:40:10 PST
Autoland Patchset:
	Patches: 597167
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=4f40044ba6c3
Try run started, revision 4f40044ba6c3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=4f40044ba6c3
Comment 3 Mozilla RelEng Bot 2012-02-14 21:01:04 PST
Try run for 4f40044ba6c3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4f40044ba6c3
Results (out of 211 total builds):
    exception: 1
    success: 179
    warnings: 16
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-4f40044ba6c3
Comment 4 Boris Zbarsky [:bz] 2012-02-17 22:02:14 PST
Comment on attachment 597167 [details] [diff] [review]
Patch v1

Goig to punt this to Ehsan, but it looks like that code was trying to deal with deleting the previous char as needed... is that not necessary?
Comment 5 :Aryeh Gregor (away until August 15) 2012-02-20 09:53:35 PST
It's not what any other browser does.  If the idea was to make it work like execCommand("delete"), it looks far too crude to be useful.  To get an effect anything like hitting the backspace key, you need way way more code than that.
Comment 6 Boris Zbarsky [:bz] 2012-02-20 11:55:46 PST
OK.  I assume you did look at the blame for this code and there was nothing useful there, right?
Comment 7 :Aryeh Gregor (away until August 15) 2012-02-20 13:37:41 PST
It seemed to go back to the original hg commit.  I didn't look in CVS.  I just did that and it seems to go back to CVS commit 3.1 (?):

"""
3.1 <cmanske@netscape.com> 2000-03-21 15:57
Cleaning up selection: changed 'RangeList' to 'Selection', thus changing implementation class of nsIDOMSelection and nsIFrameSelection to nsSelection. File nsRangeList.cpp replaced with new nsSelection.cpp
"""
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsSelection.cpp&rev2=3.1&rev1=1.6

That's as far back as I was able to find.
Comment 8 Boris Zbarsky [:bz] 2012-02-20 14:13:51 PST
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Flayout%2Fbase%2Fsrc%2FAttic%2FnsRangeList.cpp&rev=&cvsroot=%2Fcvsroot is a starting point for going further back.

That gets us to:

  1.31 <akkana@netscape.com> 1999-01-29 17:02
  Add IsCollapsed; add collapsed case to DeleteFromDocument

Not helpful, indeed.  I don't think it's worth getting in touch with Akkana to find out what she meant here...  OK.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-02-21 13:14:02 PST
Comment on attachment 597167 [details] [diff] [review]
Patch v1

Review of attachment 597167 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/test/test_bug719503.html
@@ +11,5 @@
> +<pre id="test">
> +<script>
> +getSelection().collapse(document.head, 0);
> +getSelection().deleteFromDocument();
> +ok(true, "Passed, no exception");

This is not the right way of testing this.  This way, if something throws, ok() will never get executed and the test will pass silently.  Please add a try/catch block around those two calls, and ok(false) in the catch block.  (Note that you still need to keep the current ok(true) so that the test would print _something_ when successful, to keep the test framework from complaining about a test finished with no checks run.)

r=me with that.
Comment 10 :Aryeh Gregor (away until August 15) 2012-02-22 07:40:15 PST
Created attachment 599608 [details] [diff] [review]
Patch v2
Comment 11 :Aryeh Gregor (away until August 15) 2012-02-22 07:40:46 PST
Patch updated as requested.
Comment 13 Richard Newman [:rnewman] 2012-02-23 18:51:53 PST
https://hg.mozilla.org/mozilla-central/rev/0e50adaab9d7

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