Closed Bug 773262 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
16 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox16 + fixed
thunderbird16 ? ---

People

(Reporter: AndrzejL.PCLinuxOS, Assigned: ayg)

References

Details

(Keywords: regression)

Attachments

(1 file)

Patch
3.94 KB, patch
ehsan
: review+
Details | Diff | Splinter Review 
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.
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
Assignee: nobody → ayg
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!
(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.
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
tracking-firefox16: --- → ?
Assignee: nobody → ayg
Yeah, undo completely doesn't work.  That's very bad.  I'll look.
Severity: normal → major
Status: NEW → ASSIGNED
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.
Attached patch Patch — — Splinter Review 
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)
(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.  :(
Attachment #641789 - Flags: review?(ehsan) → review+
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
Closed: 9 years ago
status-firefox16: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Status: RESOLVED → VERIFIED
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.