Closed Bug 719515 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla14

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(1 file, 1 obsolete file)

Patch v2
11.94 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
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.
Attached patch Patch v1 (obsolete) — — Splinter Review
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)
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. :-)
Attached patch Patch v2 — — Splinter Review
(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]
Autoland Failure Specified patches [605860] do not exist, or are not posted to this bug.
Whiteboard: [autoland-try:605860,606305:-u all]
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
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
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
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: