The default bug view has changed. See this FAQ.

Selection.extend() should replace the existing range, not mutate it

RESOLVED FIXED in mozilla14

Status

()

Core
Selection
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Trunk
mozilla14
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
Assignee: nobody → ayg
Attachment #605885 - Flags: review?(ehsan)
https://tbpl.mozilla.org/?tree=Try&rev=a363a7c71a54
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)?
Attachment #605885 - Flags: review?(ehsan) → review-
(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.  :-)
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.
Attachment #605885 - Attachment is obsolete: true
Attachment #606305 - Flags: review?(ehsan)
Whiteboard: [autoland-try:605860,606305:-u all]

Comment 6

5 years ago
Autoland Failure
Specified patches [605860] do not exist, or are not posted to this bug.

Updated

5 years ago
Whiteboard: [autoland-try:605860,606305:-u all]

Updated

5 years ago
Attachment #606305 - Flags: review?(ehsan) → review+
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

5 years ago
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
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b5c354bbf7
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/b7b5c354bbf7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.