Last Comment Bug 719523 - Selection.selectAllChildren() on a Document always produces end offset of 1
: Selection.selectAllChildren() on a Document always produces end offset of 1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: mozilla14
Assigned To: Aryeh Gregor (:ayg) (away until October 25)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 11:35 PST by Aryeh Gregor (:ayg) (away until October 25)
Modified: 2012-03-18 12:02 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.83 KB, patch)
2012-03-16 07:57 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Patch v2 (3.84 KB, patch)
2012-03-16 09:57 PDT, Aryeh Gregor (:ayg) (away until October 25)
ayg: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (away until October 25) 2012-01-19 11:35:09 PST
Test case:

data:text/html,<!doctype html>
<script>
getSelection().selectAllChildren(document);
document.documentElement.textContent =
getSelection().focusNode.childNodes.length
+ " " +
getSelection().focusOffset
</script>

In Firefox 12.0a1 (2012-01-17), this outputs "2 1".  The document has two children, but only the first is selected.  This happens for any document, but not any other node type.  Other browsers all get this wrong too in one way or another, but this is still a bug in Gecko -- it doesn't make sense, and it's not per spec.

(Spec: http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#dom-selection-selectallchildren)

This causes test failures in <http://dvcs.w3.org/hg/editing/raw-file/tip/selecttest/selectAllChildren.html>.  If this bug and bug 711047 are fixed, it looks like all tests in that file will pass.
Comment 1 Aryeh Gregor (:ayg) (away until October 25) 2012-02-24 10:08:17 PST
I looked at this for a while, but I couldn't figure out what the problem was.
Comment 2 Aryeh Gregor (:ayg) (away until October 25) 2012-03-16 07:57:22 PDT
Created attachment 606577 [details] [diff] [review]
Patch v1

Okay, I still have no idea what the bug was in the old code, but it was much more complicated than seems necessary.  I rewrote it based on nsRange::SelectNodeContents, and now the bug is fixed and the code is significantly shorter.

This patch needs to be applied on top of bug 719515.  Autoland will conflict (bug 734467), so I pushed manually to try instead: https://tbpl.mozilla.org/?tree=Try&rev=d95a5d075cb3
Comment 3 :Ehsan Akhgari 2012-03-16 08:24:56 PDT
Comment on attachment 606577 [details] [diff] [review]
Patch v1

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

::: layout/generic/nsSelection.cpp
@@ +5301,4 @@
>    {
> +    mFrameSelection->PostReason(nsISelectionListener::SELECTALL_REASON);
> +  }
> +  result = Extend(node, node->GetChildCount());

Nit: just return Extend(...) directly.
Comment 4 Aryeh Gregor (:ayg) (away until October 25) 2012-03-16 09:57:41 PDT
Created attachment 606614 [details] [diff] [review]
Patch v2

Same patch as before, with review comment addressed.
Comment 5 Mozilla RelEng Bot 2012-03-16 12:03:58 PDT
Try run for d95a5d075cb3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d95a5d075cb3
Results (out of 221 total builds):
    exception: 8
    success: 171
    warnings: 24
    failure: 17
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-d95a5d075cb3
Comment 7 Phil Ringnalda (:philor) 2012-03-17 17:33:14 PDT
https://hg.mozilla.org/mozilla-central/rev/e2f2d30f1046
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-03-18 12:02:38 PDT
(In reply to Aryeh Gregor from comment #2)
> Created attachment 606577 [details] [diff] [review]
> Patch v1
> 
> Okay, I still have no idea what the bug was in the old code, but it was much
> more complicated than seems necessary.  I rewrote it based on
> nsRange::SelectNodeContents, and now the bug is fixed and the code is
> significantly shorter.

\o/

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