The default bug view has changed. See this FAQ.

Selection.selectAllChildren() on a Document always produces end offset of 1

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>
<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.
I looked at this for a while, but I couldn't figure out what the problem was.
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
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #606577 - Flags: review?(ehsan)
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.
Attachment #606577 - Flags: review?(ehsan) → review+
Created attachment 606614 [details] [diff] [review]
Patch v2

Same patch as before, with review comment addressed.
Attachment #606577 - Attachment is obsolete: true
Attachment #606614 - Flags: review+

Comment 5

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