Last Comment Bug 773262 - CTRL + Z combo is not working with the latest Daily.
: CTRL + Z combo is not working with the latest Daily.
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 16 Branch
: x86 All
: -- major (vote)
: mozilla16
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on:
Blocks: 765595
  Show dependency treegraph
 
Reported: 2012-07-12 07:00 PDT by AndrzejL
Modified: 2012-07-21 11:32 PDT (History)
4 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
?


Attachments
Patch (3.94 KB, patch)
2012-07-13 02:56 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description AndrzejL 2012-07-12 07:00:14 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/16.0 Firefox/16.0
Build ID: 20120712030541

Steps to reproduce:

I have restarted Daily after the latest upgrade (about 2 - 3 hours ago) and I chose to compose a new e-mail message. I pasted a wrong link into the message so decided that I will CTRL + Z (undo) it.


Actual results:

It turns out that the key kombo was not working. I checked CTRL + A, CTRL + C and CTRL + V combos and this are working fine. Edit dropdown menu has Undo entry grayed out.


Expected results:

I really love this part of the bug reporting but... I will humor You ;). The CTRL + Z control key press should undo the wrong link.
Comment 1 Alice0775 White 2012-07-12 07:56:22 PDT
Regression window:
Good:
http://hg.mozilla.org/mozilla-central/rev/4b1249ae1906
http://hg.mozilla.org/comm-central/rev/a9ea38d7a17c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Thunderbird/16.0a1 ID:20120706030536
Bad:
http://hg.mozilla.org/mozilla-central/rev/6d7fae9764b3
http://hg.mozilla.org/comm-central/rev/7b0299689777
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Thunderbird/16.0a1 ID:20120707030544
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4b1249ae1906&tochange=6d7fae9764b3
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=a9ea38d7a17c&tochange=7b0299689777

I guess regression of Bug 765595
Comment 2 :Aryeh Gregor (working until September 2) 2012-07-12 23:23:33 PDT
Thanks for the report.  I'm not able to reproduce in Firefox.  Can you reproduce in Firefox on this page?  <http://www-archive.mozilla.org/editor/midasdemo/>  That would help me track down and fix the problem much more easily.  Thanks!

(In reply to Alice0775 White from comment #1)
> I guess regression of Bug 765595

Seems possible, since that messed around with transaction-related code a bunch, although I'm not sure exactly how it would have happened.  Thanks for the regression range!
Comment 3 Alice0775 White 2012-07-12 23:36:37 PDT
(In reply to :Aryeh Gregor from comment #2)
> Thanks for the report.  I'm not able to reproduce in Firefox.  Can you
> reproduce in Firefox on this page? 
> <http://www-archive.mozilla.org/editor/midasdemo/>  That would help me track
> down and fix the problem much more easily.  Thanks!

This problem only happens with Compose window of Thunderbird and SeaMonkey.


And I confirmed in Thunderbird local build, this is regression of Bug 765595.
Comment 4 Alice0775 White 2012-07-13 01:55:23 PDT
In reply to :Aryeh Gregor from comment #2)

The following Ritch Text Editor are also not working(CTRL+Z)

http://www.kevinroth.com/rte/demo.htm
http://developer.yahoo.com/yui/examples/resize/rte_resize.html

Regression range (mozilla-inbound)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0158f2d0b32d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120705225257
Bad;
http://hg.mozilla.org/integration/mozilla-inbound/rev/510602478d52
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120706005656
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0158f2d0b32d&tochange=510602478d52
Comment 5 :Aryeh Gregor (working until September 2) 2012-07-13 02:04:41 PDT
Yeah, undo completely doesn't work.  That's very bad.  I'll look.
Comment 6 :Aryeh Gregor (working until September 2) 2012-07-13 02:30:20 PDT
Minimal test-case:

data:text/html,<!DOCTYPE html>
<iframe></iframe>
<script>
onload = function() {
var doc = document.body.firstChild.contentDocument;
doc.body.innerHTML = "<p>Hello";
doc.designMode = "on";
doc.designMode = "off";
doc.designMode = "on";
doc.getSelection().selectAllChildren(doc.body.firstChild);
doc.execCommand("bold");
document.body.textContent = doc.queryCommandEnabled("undo");
}
</script>

Outputs "false", should output "true".  One-line fix coming up.
Comment 7 :Aryeh Gregor (working until September 2) 2012-07-13 02:56:51 PDT
Created attachment 641789 [details] [diff] [review]
Patch

Bug 765595 part 2 basically did this to nsEditor::EnableUndo, after cleanup:

 NS_IMETHODIMP
 nsEditor::EnableUndo(bool aEnable)
 {
   if (aEnable) {
     if (!mTxnMgr) {
-      mTxnMgr = do_CreateInstance(NS_TRANSACTIONMANAGER_CONTRACTID, &result);
-      if (NS_FAILED(result) || !mTxnMgr) {
-        return NS_ERROR_NOT_AVAILABLE;
-      }
+      mTxnMgr = new nsTransactionManager();
     }
-    mTxnMgr->SetMaxTransactionCount(-1);
   } else if (mTxnMgr) {
     // disable the transaction manager if it is enabled
     mTxnMgr->Clear();
     mTxnMgr->SetMaxTransactionCount(0);
   }
 
   return NS_OK;
 }

I removed the SetMaxTransactionCount(-1) line because I saw that the nsTransactionManager constructor did that anyway, and now I was explicitly using the constructor.  But of course, this is totally wrong, because the constructor would only run if mTxnMgr was false!  So the first time something called EnableUndo(true), it would create a new nsTransactionManager with its max transaction count correctly initialized to -1.  But if something then called EnableUndo(false) and then EnableUndo(true), such as by toggling designMode on and off, the max transaction count would remain stuck at 0, stopping any undos or redos.

Try: https://tbpl.mozilla.org/?tree=Try&rev=6e0abc054287
Comment 8 :Ehsan Akhgari 2012-07-13 09:09:02 PDT
(In reply to :Aryeh Gregor from comment #7)
> I removed the SetMaxTransactionCount(-1) line because I saw that the
> nsTransactionManager constructor did that anyway, and now I was explicitly
> using the constructor.  But of course, this is totally wrong, because the
> constructor would only run if mTxnMgr was false!  So the first time
> something called EnableUndo(true), it would create a new
> nsTransactionManager with its max transaction count correctly initialized to
> -1.  But if something then called EnableUndo(false) and then
> EnableUndo(true), such as by toggling designMode on and off, the max
> transaction count would remain stuck at 0, stopping any undos or redos.

Ah, dammit, I saw this when I was reviewing, and then I convinced myself the exact same way that you did.  :(
Comment 9 :Aryeh Gregor (working until September 2) 2012-07-14 23:21:56 PDT
Callek wasn't sure an m-i push now would make it into the uplift tomorrow, so I pushed straight to m-c:

https://hg.mozilla.org/mozilla-central/rev/57abb5f70e01

He volunteered to watch the tree for me.
Comment 10 AndrzejL 2012-07-21 11:32:39 PDT
Big thanks to all of You who got involved in fixing this.

It works as expected now.

May the source be with You.

Regards.

Andrzej

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