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.
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)?
(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.
Autoland Failure Specified patches  do not exist, or are not posted to this bug.
5 years ago
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://email@example.com