Last Comment Bug 318065 - Value is wrong during oninput for ctrl+z (undo) if text was blank before undo
: Value is wrong during oninput for ctrl+z (undo) if text was blank before undo
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
: 320006 356895 433574 499677 653204 698726 (view as bug list)
Depends on: 503838
Blocks: 318355 468835 304145 461673 471330
  Show dependency treegraph
 
Reported: 2005-11-28 18:17 PST by Csaba Gabor
Modified: 2011-11-01 06:04 PDT (History)
25 users (show)
roc: wanted1.9.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase for oninput / ctrl+z issue (855 bytes, text/html)
2005-11-28 18:19 PST, Csaba Gabor
no flags Details
Patch v1 (15.07 KB, patch)
2008-10-28 12:04 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Patch v2 (Unbitrotted) (14.45 KB, patch)
2009-02-27 16:55 PST, Graeme McCutcheon [:graememcc]
ehsan: review-
Details | Diff | Review
Patch v3 (6.83 KB, patch)
2011-08-16 05:58 PDT, Graeme McCutcheon [:graememcc]
ehsan: review+
Details | Diff | Review

Description Csaba Gabor 2005-11-28 18:17:50 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051020 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051020 Firefox/1.6a1

If I have an oninput event handler for a textbox input and the textbox is empty and ctrl+z produces text in the textbox, then the .value for the textbox shows empty in the event handler even though the textbox is nonempty.

The attachment shows the steps to reproduce.  It has a single text intput with an oninput handler.  This handler simply prepends the text input's value on a separate line to a TD element.

First select what is in the lone textbox and then type a letter to make it go away.  Then type ctrl+z to get the original contents back.  This part just demonstrates basic functionality.  Now type ctrl+x (the contents are already selected from the last step) to make the textbox empty.  Everything is OK to here.

Finally, press ctrl+z to get the original contents back.  That happens, but the TD shows that the oninput event handler thought the .value of the textbox was empty.

Csaba Gabor from Vienna

Reproducible: Always

Steps to Reproduce:
Comment 1 Csaba Gabor 2005-11-28 18:19:44 PST
Created attachment 204405 [details]
Testcase for oninput / ctrl+z issue
Comment 2 arno renevier 2008-10-23 14:46:00 PDT
Hi,
I ran into this bug when trying to validate a form while user is typing.

I tried to investigate, and discovered where the problem comes frome:

When running undo command, nsPlaintextEditor::Undo calls nsEditor::Undo then
mRules->DidUndo [1]

nsEditor::undo does all the real work, then it calls NotifyEditorObservers [2]

mRules->DidUndo resets mBogusNode if it was non null (ie: if input was empty) [3]

So, when calling NotifyEditorObservers (and therefore triggering input event
handler), mBogusNode is not null if input was empty before undoing. So, in
nsTextEditRules::WillOutputText (called when trying to read input.value),
output string will be null [4]

[1]: http://mxr.mozilla.org/mozilla/source/editor/libeditor/text/nsPlaintextEditor.cpp#1092
[2]: http://mxr.mozilla.org/mozilla/source/editor/libeditor/base/nsEditor.cpp#754
[3]: http://mxr.mozilla.org/mozilla/source/editor/libeditor/text/nsTextEditRules.cpp#1065
[4]: http://mxr.mozilla.org/mozilla/source/editor/libeditor/text/nsTextEditRules.cpp#1134
Comment 3 Graeme McCutcheon [:graememcc] 2008-10-28 09:22:55 PDT
*** Bug 320006 has been marked as a duplicate of this bug. ***
Comment 4 Graeme McCutcheon [:graememcc] 2008-10-28 12:04:30 PDT
Created attachment 345131 [details] [diff] [review]
Patch v1

This adds flags to signal whether the text in the editor is in a state to be read following an undo/redo
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2008-11-14 16:50:20 PST
Comment on attachment 345131 [details] [diff] [review]
Patch v1

Peter, I think you'd be a better reviewer for this change.
Comment 6 Henrik Skupin (:whimboo) 2009-02-15 12:24:53 PST
Tony, probably you can help us here? As it looks like we can't reach peterv to this and other bugs reviewed. Who else is able to review this piece of code or how should we better handle the review request? It would be really helpful to have this fixed in Firefox 3.1.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-17 11:39:57 PST
Is this a regression?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-17 11:40:30 PST
If it's a regression I guess it must have regressed a long time ago.
Comment 9 Peter Van der Beken [:peterv] 2009-02-17 11:54:38 PST
Don't think this is a regression. I've looked at the patch, but not at the point where I feel confident that I know what the effects of this change will be :-/.
Comment 10 Henrik Skupin (:whimboo) 2009-02-17 12:01:52 PST
It's pre FF1.0. No idea if its a real regression or ever happened.
Comment 11 Henrik Skupin (:whimboo) 2009-02-17 12:02:34 PST
Sorry Peter. Mid-air collision sets back the flag. :(
Comment 12 Graeme McCutcheon [:graememcc] 2009-02-20 15:23:12 PST
> If it's a regression I guess it must have regressed a long time ago.

Yeah, I suspect this was broken with checkin 1.50 in 1999 (the will undo/redo logic was added in before this).
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsTextEditRules.cpp&branch=&root=/cvsroot&subdir=mozilla/editor/libeditor/text&command=DIFF_FRAMESET&rev1=1.49&rev2=1.50

As with bug 471722, I'm reluctant to label a case where we've shipped with it broken far longer than we ever shipped it working correctly as a "regression" ;)
However, we are clearly returning the wrong value in these cases, as the dependencies show, and any other case where js reads the value from an input handler during undo.
Comment 13 Graeme McCutcheon [:graememcc] 2009-02-27 16:55:04 PST
Created attachment 364616 [details] [diff] [review]
Patch v2 (Unbitrotted)

I've unbitrotted this, and made a couple of tweaks:

- Improved the test coverage to cover all the possible ways an incorrect value could be reported

- In the previous patch, any failures in the bogus node checking code would be reported by WillOutputText rather than Did(Un/Re)do, which is wrong. This version reports those errors at the correct action.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-03-13 14:00:58 PDT
*** Bug 433574 has been marked as a duplicate of this bug. ***
Comment 15 Michael Ryan 2009-06-22 11:42:10 PDT
*** Bug 356895 has been marked as a duplicate of this bug. ***
Comment 16 Michael Ryan 2009-06-22 11:42:44 PDT
*** Bug 499677 has been marked as a duplicate of this bug. ***
Comment 17 Michael Ryan 2009-06-22 11:52:33 PDT
FYI, I've marked those as dupes because bug 320006 has been marked as a dupe of this. They may actually be different bugs - if so, bug 320006 should be re-opened and bug 356895 and bug 499677 should be marked as dupes of it.
Comment 18 Wayne Mery (:wsmwk, NI for questions) 2010-12-02 09:27:58 PST
is review blocked on bug 503838?  (almost 3 years since review request)
Comment 19 Peter Van der Beken [:peterv] 2010-12-02 10:52:08 PST
Comment on attachment 364616 [details] [diff] [review]
Patch v2 (Unbitrotted)

I don't think i can safely review this.
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2010-12-02 16:12:24 PST
Comment on attachment 364616 [details] [diff] [review]
Patch v2 (Unbitrotted)

Graeme, firstly, sorry that you've been waiting for this review so long.

While your approach would work, the right way to fix this is to just move the NotifyEditorObservers call from the end of nsEditor::Undo to the end of the nsPlaintextEditor override.

That will make sure that NotifyEditorObservers is called after DidUndo has done its job, and would mean minimum changes to the code, which is a good thing.

Are you willing to write a patch which does that?

I'm reviewing the test below:

>diff --git a/editor/libeditor/text/tests/test_bug318065.html b/editor/libeditor/text/tests/test_bug318065.html
>+
>+      /** Test for Bug 318065 **/
>+      SimpleTest.waitForExplicitFinish();

Can you please wrap everything 

>+      function doTest() {

Can you please run doTest off of SimpleTest.waitForFocus, instead of onload?  This will make sure that we don't get intermittent oranges on Linux since the test is using synthesizeKey.

>+        netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

This shouldn't be needed, right?

>+        var input = document.getElementById("t1");
>+        input.addEventListener("input", onInput, false);
>+        input.focus();
>+
>+        // Tests 0 + 1: Input letter and delete it again
>+        synthesizeKey("A", {});
>+        synthesizeKey("VK_BACK_SPACE", {});
>+
>+        // Test 2: Undo deletion. Value of input should be "A"
>+        synthesizeKey("Z", {ctrlKey: true});

You want accelKey here to make this work on Mac as well.

>+        // Test 3: Redo deletion. Value of input should be ""
>+        synthesizeKey("Y", {ctrlKey: true});

You should use this to make it work on all platforms:

synthesizeKey("Z", {accelKey: true, shiftKey: true});

>+        var input2 = document.getElementById("t2");
>+        input2.addEventListener("input", onInput, false);
>+        input2.focus();
>+        
>+        // Test 4: Input letter
>+        synthesizeKey("A", {});
>+
>+        // Test 5: Undo typing. Value of input should be ""
>+        synthesizeKey("Z", {ctrlKey: true});
>+
>+        // Test 6: Redo typing. Value of input should be "A"
>+        synthesizeKey("Y", {ctrlKey: true});

ditto.

>+      }
>+   </script>
>+  </pre>
>+
>+  <input type="text" value="" id="t1" />
>+  <input type="text" value="" id="t2" />
>+</body>
>+</html>

The test looks awesome overall, thanks for working on this!
Comment 21 Tim (fmdeveloper) 2011-06-29 22:00:38 PDT
*** Bug 653204 has been marked as a duplicate of this bug. ***
Comment 22 Graeme McCutcheon [:graememcc] 2011-08-16 05:58:03 PDT
Created attachment 553447 [details] [diff] [review]
Patch v3
Comment 23 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-17 08:31:03 PDT
Comment on attachment 553447 [details] [diff] [review]
Patch v3

Review of attachment 553447 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!  You were only missing a SimpleTest.finish() call, which I added myself.  I will push this to mozilla-inbound.  Thanks for your patch!  :-)
Comment 24 Graeme McCutcheon [:graememcc] 2011-08-17 08:37:14 PDT
> You were only missing a SimpleTest.finish() call,
> which I added myself.

There's a call to SimpleTest.finish() is in the input handler, triggered the final time the handler is call
Comment 25 Graeme McCutcheon [:graememcc] 2011-08-17 08:37:47 PDT
...er called.
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-17 13:41:03 PDT
Ah, right.  I missed that, sorry.  I pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/8fd6d6928229 as a followup to remove the second finish call.
Comment 28 Mardeg 2011-11-01 06:04:42 PDT
*** Bug 698726 has been marked as a duplicate of this bug. ***

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