171243 - redo doesn't work if more than one letter is to be reinserted

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Kathleen :Brade]

Redo doesn't work in text widgets or in Composer:
 1) type: abc
 2) choose: edit>undo
 3) choose: edit>redo
Expectation:
    see: abc
Actual Result:
    see: a

Comment 1

[kinmoz]

Looks like during the redo, just after we put the textnode back into the tree
with "a" in it, a call to nsSelectionState::RestoreSelection() is failing
because it tries to create a range that places the caret at range(textnode, 2),
and there is only one character currently in the text node.

Comment 2

[kinmoz]

This broke when the fix for bug 149320 went in. It's not so much the fix for bug
149320, but the fact that we have a placeholder transaction with children that
look like this:

  1. delete txn (the br node)
  2. insert element (text node with "a")
  3. placeholder (empty one created for the txn that follows)
  4. insert text ("b")
  5. placeholder (empty one created for the txn that follows)
  6. insert text ("c")

The empty placeholders are being appended into the outer one, and as a result
when we redo them, the placeholder in #3 tries to restore the selection as if
the insert text txn for "b" already happened.

Comment 3

[kinmoz]

--> jfrancis

Comment 4

[Joe Francis]

Attachment: patch to libeditor

I pulling patch apart from the others I worked on, I found other redo problems.
 Investigating those I discovered that they were caused by recent changes I
made to nsEditor::InsertContainerAbove() and nsEditorReplaceContainer() a while
back.  So I had to also undo those changes, and since those changes had been a
fix for another bug (voidArray assertions caused by comparing two ranges that
were not in same document) I had to find another solution to that bug as well. 
That other solution is in nsHTMLEditRules.cpp. where I sanity check incoming
ranges to UpdateDocChangedRange().

Meanwhile the original part of this patch carries on: I now do not place
Placeholder txns as children inside other Placeholders.  Since the editor needs
a concept of the current placeholder txn, I had to make changes there for it to
peek at the Undo stack.

I'm confident none of this makes sense to anyone except maybe Kin.  :-P

Comment 5

[Kathleen :Brade]

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

r=brade

can we delete these lines in htmleditrules?
+//    we got startNode above already
+//    res = aRange->GetStartContainer(getter_AddRefs(startNode));
+//    if (NS_FAILED(res)) return res;

Comment 6

[kinmoz]

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

==== Can we remove some of the code you commented out?

-	       AppendChild(editTxn);
+	       // AppendChild(editTxn);


...


-      nsCOMPtr<nsIDOMNode> startNode;
       PRInt32 startOffset;
-      res = aRange->GetStartContainer(getter_AddRefs(startNode));
-      if (NS_FAILED(res)) return res;
+//    we got startNode above already
+//    res = aRange->GetStartContainer(getter_AddRefs(startNode));
+//    if (NS_FAILED(res)) return res;



==== In |nsEditor::Do()| what's the purpose of |txn|? Why can't we just call
|Do(theTxn);|?


     // finally we QI to an nsITransaction since that's what Do() expects
     nsCOMPtr<nsITransaction> theTxn = do_QueryInterface(plcTxn);
     nsITransaction* txn = theTxn;
     Do(txn);  // we will recurse, but will not hit this case in the nested
call


==== Is there any serious perf impact of calling this in
|UpdateDocChangeRange()|? For example during a paste or some other operation
over lots of nodes?


+  if (!mHTMLEditor->IsDescendantOfBody(startNode))


sr=[e-mail] if there isn't any significant perf degradation, and the
points above are addressed.

Comment 7

[Joe Francis]

i addressed comments in checked in version.  But I did leave one of the
commented out code lines becasue I need it - else i'll forget later why i'm not
merging placeholders in the most obvious way.

fixed on trunk

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This caused a memory leak: bug 174123 (and no, that isn't just a typo of this
bug number with some of digits mixed up).

Comment 9

[sairuh (rarely reading bugmail)]

works fine with 2003.04.03 builds.