CTRL + Z combo is not working with the latest Daily.

VERIFIED FIXED in Firefox 16

Status

()

Product:
Core
Component:
Editor
Importance:
--
major
Status:
VERIFIED FIXED
Reported:
7 years ago
Modified:
7 years ago

People

(Reporter: AndrzejL.PCLinuxOS, Assigned: ayg)

Tracking

({regression})

Version:
16 Branch
Target:
mozilla16
Platform:
x86
All
Keywords:
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16+ fixed, thunderbird16?)

Details

Attachments

(1 attachment)

Patch
3.94 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

•
7 years ago 
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
Blocks: 765595
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Linux → All

Updated

•
7 years ago
tracking-thunderbird16: --- → ?

Updated

•
7 years ago
Assignee: nobody → ayg
 (Assignee)

Comment 2

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

•
7 years ago 
(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

•
7 years ago 
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
Assignee: ayg → nobody
Component: General → Editor
Product: Thunderbird → Core
Version: 16 → 16 Branch

Updated

•
7 years ago
tracking-firefox16: --- → ?

Updated

•
7 years ago
Assignee: nobody → ayg
 (Assignee)

Comment 5

•
7 years ago 
Yeah, undo completely doesn't work.  That's very bad.  I'll look.
Severity: normal → major
Status: NEW → ASSIGNED
 (Assignee)

Comment 6

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

Comment 7

•
7 years ago 
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
Attachment #641789 - Flags: review?(ehsan)

Comment 8

•
7 years ago 
(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.  :(

Updated

•
7 years ago
Attachment #641789 - Flags: review?(ehsan) → review+
tracking-firefox16: ? → +
 (Assignee)

Comment 9

•
7 years ago 
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-firefox16: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
 (Reporter)

Updated

•
7 years ago
Status: RESOLVED → VERIFIED
 (Reporter)

Comment 10

•
7 years ago 
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
You need to log in before you can comment on or make changes to this bug.