Last Comment Bug 135928 - Range.surroundContent broken if start and end of range in same textnode
: Range.surroundContent broken if start and end of range in same textnode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Future
Assigned To: kinmoz
: lchiang
: Andrew Overholt [:overholt]
Mentors:
: 221794 (view as bug list)
Depends on: 58974
Blocks: 30838
  Show dependency treegraph
 
Reported: 2002-04-06 06:22 PST by Peter Van der Beken [:peterv]
Modified: 2014-04-26 03:06 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (1.10 KB, text/html)
2002-04-06 06:23 PST, Peter Van der Beken [:peterv]
no flags Details
Partial patch (1.74 KB, patch)
2002-04-06 06:29 PST, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Testcase (fixed) (1018 bytes, text/html)
2003-10-18 03:16 PDT, kinmoz
no flags Details
SurroundContents part of bug 58974 (5.71 KB, patch)
2004-09-01 08:47 PDT, neil@parkwaycc.co.uk
jst: superreview+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2002-04-06 06:22:48 PST
Calling surroundContent on a range that has its start and end boundary points in
the same textnode fails completely. The first problem is that we check if the
start boundary point is in a textnode, if so we split that node. Then we check
if the end boundary point is in a text node and we try to split that node too.
However, since we split the textnode, the end boundary point is now really in
the node created by the split, so we're using the wrong container and offset for
the end boundary point. I tried moving the split of the end boundary point
before the split of the start boundary point. Then we hit the next problem, we
try to insert the node that gets passed in to surroundContent as a child node of
the common ancestor of the start and end boundary points. Since they're in the
same textnode we seem to take that textnode as the common ancestor and so the
insert fails (textnodes can't have children). Testcase coming up.
Comment 1 Peter Van der Beken [:peterv] 2002-04-06 06:23:22 PST
Created attachment 78028 [details]
Testcase
Comment 2 Peter Van der Beken [:peterv] 2002-04-06 06:29:29 PST
Created attachment 78029 [details] [diff] [review]
Partial patch

This patch just moves the splitting of the text node that contains the end
boundary point before the splitting of the text node that contains the start
boundary point. Doesn't solve all the issues.
Comment 3 Rafael Gattringer 2003-01-22 11:30:50 PST
Related bug:
Range surroundContents not implemented
http://bugzilla.mozilla.org/show_bug.cgi?id=58974
Comment 4 kinmoz 2003-01-22 11:50:50 PST
I think the current implementation of SurroundContents() needs to be trashed and
then re-written to use ExtractContents() to break the "selected" content into a
subtree, InsertNode() for the new container at the collapsed point after the
extraction, and then the extracted contents inserted under the new container.

Note that ExtractContents() still doesn't work exactly as the spec dictates
because the subtree it extracts is an entirely new set of nodes representing
what was selected. See this for more details:

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRange.cpp#1816
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2003-10-10 10:07:51 PDT
*** Bug 221794 has been marked as a duplicate of this bug. ***
Comment 6 kinmoz 2003-10-18 03:16:36 PDT
Created attachment 133565 [details]
Testcase (fixed)

The previous test case had an infinite recursion bug.
Comment 7 kinmoz 2003-10-18 03:40:05 PDT
I just posted a patch in bug 58974 that re-implements SurroundContents() and
ExtractContents(). It fixes the test case in this bug.
Comment 8 neil@parkwaycc.co.uk 2004-09-01 08:47:36 PDT
Created attachment 157617 [details] [diff] [review]
SurroundContents part of bug 58974

I was unable to merge all of Kin's patch to the tip, so I thought I'd just
merge the SurroundContents part of the patch, which would at least fix this
bug. Basically I applied Kin's patch, cut out SurroundContents and pasted it
into a trunk version of nsRange.cpp; it still has the ExtractContents issue
though.
Comment 9 neil@parkwaycc.co.uk 2004-09-01 08:52:57 PDT
Comment on attachment 157617 [details] [diff] [review]
SurroundContents part of bug 58974

In theory I can assume r=jfrancis sr=jst right?
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2004-09-01 09:05:59 PDT
Comment on attachment 157617 [details] [diff] [review]
SurroundContents part of bug 58974

Yeah, seems reasonable to me.
Comment 11 neil@parkwaycc.co.uk 2004-09-01 09:18:51 PDT
Fix checked in.
Comment 12 Akos Maroy 2006-01-11 12:05:14 PST
I wonder if this bug has rell been resolved. I get the same issue today, with:

Mozilla 1.7.12
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20051022

and:

Firefox 1.0.7
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20060106 Firefox/1.0.7


and it's 2006! the bug was suppsedly solved in 2004...?
Comment 13 Akos Maroy 2006-01-11 12:07:54 PST
I wonder why can't one re-open a bug...
Comment 14 Peter Van der Beken [:peterv] 2006-01-12 01:25:35 PST
Don't reopen this bug, it is fixed in Firefox 1.5 and trunk. It was never fixed on the 1.0.x branch (and there's no need to do so). If you still see the issue on a trunk build, open a new bug.

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