Merging blocks via forward delete sends selection to front of doc

VERIFIED FIXED in M1

Status

()

Core
Editor
--
major
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Joe Francis, Assigned: Joe Francis)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: editorbase+; fixinhand; need r=, sr=, URL)

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

15 years ago
Status: NEW → ASSIGNED
Whiteboard: editorbase
Target Milestone: --- → M1
(Assignee)

Updated

15 years ago
Whiteboard: editorbase → editorbase moose
(Assignee)

Updated

15 years ago
Whiteboard: editorbase moose → editorbase+
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

15 years ago
*** Bug 180784 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

15 years ago
Created attachment 106898 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Whiteboard: editorbase+ → editorbase+; fixinhand; need r=, sr=
(Assignee)

Updated

15 years ago
Attachment #106898 - Flags: superreview?(kin)
Attachment #106898 - Flags: review?(brade)
(Assignee)

Updated

15 years ago
Attachment #106898 - Flags: review?(brade) → review?(cmanske)

Comment 5

15 years ago
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)
Attachment #106898 - Flags: superreview?(kin) → superreview+

Comment 6

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

Looks good. Works well. :)
Attachment #106898 - Flags: review?(cmanske) → review+
(Assignee)

Comment 7

15 years ago
fix landed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 8

15 years ago
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.
(Assignee)

Comment 9

15 years ago
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

15 years ago
*** Bug 170801 has been marked as a duplicate of this bug. ***
QA Contact: sujay → sairuh
vrfy'd fixed on win2k and linux rh8.0 (2003.03.03) and mac 10.2.4 (2003.02.28).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.