Last Comment Bug 719515 - Selection.extend() should replace the existing range, not mutate it
: Selection.extend() should replace the existing range, not mutate it
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: mozilla14
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 11:17 PST by :Aryeh Gregor (working until September 2)
Modified: 2012-03-17 17:05 PDT (History)
3 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.49 KB, patch)
2012-03-14 11:58 PDT, :Aryeh Gregor (working until September 2)
ehsan: review-
Details | Diff | Splinter Review
Patch v2 (11.94 KB, patch)
2012-03-15 11:36 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2012-01-19 11:17:25 PST
Test case:

data:text/html,<!doctype html>
<body>
<script>
getSelection().collapse(document.body, 0);
var range = getSelection().getRangeAt(0);
getSelection().extend(document.body, 0);
document.body.textContent =
range == getSelection().getRangeAt(0);
</script>

Firefox 12.0a1 (2012-01-17) outputs "true".  IE9 throws because it doesn't support extend().  Chrome 17 dev and Opera Next 12.00 alpha output "false", although admittedly getRangeAt() returns copies in both, so it would output "false" even without the extend() call.

The specification requires that extend() create a new range that replaces the existing one, not mutate the existing one:

"""
3. Let new range be a new range.
. . .
7. Set the context object's range to new range.
"""
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#dom-selection-extend

This is consistent with collapse(), collapseToStart(), collapseToEnd(), and selectAllChildren().  It's also consistent with what happens if the user modifies the selection: any existing range is replaced, not mutated.  deleteFromDocument() does mutate any existing range, but it's the odd one out (and perhaps it should be changed too).

This causes test failures in <http://aryeh.name/tmp/editing/selecttest/extend.html>.  If this bug, bug 711047, and one more that I'm about to file are fixed, Firefox will pass all the tests in that file AFAICT.
Comment 1 :Aryeh Gregor (working until September 2) 2012-03-14 11:58:34 PDT
Created attachment 605885 [details] [diff] [review]
Patch v1

I admit to having no idea if this is a sane way to do this.  I cheated and used AddRange and RemoveRange instead of internal methods, because I know what those actually do.  To my surprise, it seems to actually work.

Note: this applies on top of the patch for bug 719518.  It will conflict if you apply it to m-c due to different context in the Makefile.

Also note: I removed a stray semicolon and a redundant (apparently?) "if (NS_FAILED(res)) return res;" that were nearby.  Tell me if that sort of minor cleanup should be in a separate patch.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-03-14 12:11:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=a363a7c71a54
Comment 3 :Ehsan Akhgari 2012-03-14 13:58:19 PDT
Comment on attachment 605885 [details] [diff] [review]
Patch v1

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

Looks good overall, minusing for the comment about Clear and AddItem.

::: layout/generic/nsSelection.cpp
@@ +5109,5 @@
> +    return res;
> +
> +  res = AddRange(range);
> +  if (NS_FAILED(res))
> +    return res;

Please use Clear and AddItem here.

::: layout/generic/test/Makefile.in
@@ +128,5 @@
>    test_selection_splitText-normalize.html \
>    test_bug524925.html \
>    test_bug719503.html \
>    test_bug719518.html \
> +  test_bug719515.html \

Nit: please preserve the alphabetical ordering.

::: layout/generic/test/test_bug719515.html
@@ +13,5 @@
> +getSelection().collapse(document.body, 0);
> +var range = getSelection().getRangeAt(0);
> +getSelection().extend(document.body, 1);
> +isnot(range, getSelection().getRangeAt(0),
> +      "extend() must replace existing ranges, not mutate them");

This looks good.  Can you also test the case where the node and offset are the same as the existing selection (this is what we were optimizing for previously)?
Comment 4 :Ehsan Akhgari 2012-03-14 13:59:03 PDT
(In reply to Aryeh Gregor from comment #1)
> Also note: I removed a stray semicolon and a redundant (apparently?) "if
> (NS_FAILED(res)) return res;" that were nearby.  Tell me if that sort of
> minor cleanup should be in a separate patch.

I generally appreciate this kind of cleanup in the patch as long as they're not so pervasive to hide what the patch is really doing.  :-)
Comment 5 :Aryeh Gregor (working until September 2) 2012-03-15 11:36:01 PDT
Created attachment 606305 [details] [diff] [review]
Patch v2

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Please use Clear and AddItem here.

As I mentioned on IRC, Clear would remove all ranges, not just the one we want to remove.  After a bit of fiddling, I took a different tactic with this patch: I renamed CopyRangeToAnchorFocus to SetAnchorFocusToRange, and made it overwrite mAnchorFocusRange instead of mutating it.  There was a comment by bz suggesting doing that anyway.  There's one other caller of that method, which might or might not be broken by that change, but I'm being optimistic.  :)  The rewritten version uses RemoveItem and AddItem.

> Nit: please preserve the alphabetical ordering.

Fixed in new version, although the file wasn't actually alphabetical before.

> This looks good.  Can you also test the case where the node and offset are
> the same as the existing selection (this is what we were optimizing for
> previously)?

Done in new version.
Comment 6 Mozilla RelEng Bot 2012-03-15 11:38:03 PDT
Autoland Failure
Specified patches [605860] do not exist, or are not posted to this bug.
Comment 7 :Aryeh Gregor (working until September 2) 2012-03-15 13:21:39 PDT
Okay, I submitted to the tryserver manually, since autoland would require me to post a new patch version and it seemed like more effort to do that: https://tbpl.mozilla.org/?tree=Try&rev=c9df683796bd
Comment 8 Mozilla RelEng Bot 2012-03-15 20:17:13 PDT
Try run for c9df683796bd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c9df683796bd
Results (out of 216 total builds):
    success: 176
    warnings: 38
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-c9df683796bd
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-17 17:05:15 PDT
https://hg.mozilla.org/mozilla-central/rev/b7b5c354bbf7

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