179384 - Merging blocks via forward delete sends selection to front of doc

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Joe Francis]

See Jim Booth's good example in the URL above.  Originally reported in an
unrelated bug, so I opened a new one.

Comment 1

[Joe Francis]

*** Bug 179880 has been marked as a duplicate of this bug. ***

Comment 2

[Patrick]

Sorry for my dupe (bug 179384). I'm surprised to find this bug listed under
"browser" (that's why i didn't find it). 

Comment 3

[Joe Francis]

*** Bug 180784 has been marked as a duplicate of this bug. ***

Comment 4

[Joe Francis]

Attachment: patch to libeditor

The bug fix is actually just a few lines in nsRangeUpdater::SelAdjJoinNodes(),
but I did some other work while I was at it.  I made
nsRangeUpdater::SelAdjDeleteNode() also adjust range items that are in or under
the deleted node.  I used to not do this becasue I was afraid of the
performance impact, but I was probably thinking about examining all descendants
of the deleted node.  The quick way to do it is simply to look up the parent
chain of the range item, and see if it is under the deleted node.

In order to made that check, I needed the util routine IsDecendantOf(), which
was in nsHTMLEditUtils.  nsSelectionState is used in the base editor, so I
moved IsDecendantOf() out of nsHTMLEditUtils and into nsEditorUtils.  While I
was at it I also moved IsLeafNode(), which is another routine that is not
html-specific.	Then I had to fix up all callers of these routines to use the
new class name.

So most of the diff is just mechanically moving code around to where it should
have been.

Comment 5

[kinmoz]

Comment on attachment 106898 [details] [diff] [review]
patch to libeditor

==== In general the .get() shouldn't be necessary anymore, but you know that.
:-)

==== Can we put a blank line before this comment?


+  nsCOMPtr<nsIDOMNode> oldStart;
+  // check for range endpoints that are in descendants of aNode
+  if (nsEditorUtils::IsDescendantOf(item->startNode, aNode))


==== And this one?


+  }
+  // avoid having to call IsDescendantOf() for common case of range startnode
== range endnode.
+  if ((item->endNode == oldStart) ||
nsEditorUtils::IsDescendantOf(item->endNode, aNode))


==== And this one?

       item->endOffset += aOldLeftNodeLength;
+    // adjust endpoints in aLeftNode
+    if (item->startNode.get() == aLeftNode)

Comment 6

[Charles Manske]

Comment on attachment 106898 [details] [diff] [review]
patch to libeditor

Looks good. Works well. :)

Comment 7

[Joe Francis]

fix landed on trunk

Comment 8

[Aleksey Nogin]

Comment on attachment 106898 [details] [diff] [review]
patch to libeditor

This change have added a warning on brad TBox:

+editor/libeditor/base/nsSelectionState.cpp:307
+ `struct nsRangeStore * item' might be used uninitialized in this function

Indeed, if the count is <=0, then the for loop is never executed and we end up
dereferencing an uninitialized pointer!

P.S. Per bug 179819 such "uninitalized" warnings ought to be considered
compilation errors.

Comment 9

[Joe Francis]

Wow, what a brain fart.  All the new lines in that function were supposed to be
inside the for loop, not outside.  I'm landing a fix for that right now.  Good
catch!

Comment 10

[Tristan Seligmann]

*** Bug 170801 has been marked as a duplicate of this bug. ***

Comment 11

[sairuh (rarely reading bugmail)]

vrfy'd fixed on win2k and linux rh8.0 (2003.03.03) and mac 10.2.4 (2003.02.28).