Closed Bug 719523 Opened 12 years ago Closed 12 years ago

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

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
3.84 KB, patch
ayg
: review+
Details | Diff | Splinter Review 
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.
Attached patch Patch v1 (obsolete) — — Splinter Review 
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+
Attached patch Patch v2 — — Splinter Review 
Same patch as before, with review comment addressed.
Attachment #606577 - Attachment is obsolete: true
Attachment #606614 - Flags: review+
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
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
Closed: 12 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.

Attachment

General

Creator:
Created:
Updated:
Size: