Closed Bug 318065 Opened 19 years ago Closed 13 years ago

Value is wrong during oninput for ctrl+z (undo) if text was blank before undo

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: danswer, Assigned: graememcc)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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:
Summary: oninput problem with ctrl+z → value is wrong during oninput for ctrl+z (undo) if text was blank before undo
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: 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
Blocks: 304145
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
Blocks: 318355, 461673
Attached patch Patch v1 (obsolete) — Splinter Review
This adds flags to signal whether the text in the editor is in a state to be read following an undo/redo
Attachment #345131 - Flags: superreview?(jst)
Attachment #345131 - Flags: review?(jst)
Assignee: events → nobody
Component: Event Handling → Editor
OS: Windows XP → All
QA Contact: ian → editor
Hardware: PC → All
Comment on attachment 345131 [details] [diff] [review]
Patch v1

Peter, I think you'd be a better reviewer for this change.
Attachment #345131 - Flags: superreview?(peterv)
Attachment #345131 - Flags: superreview?(jst)
Attachment #345131 - Flags: review?(peterv)
Attachment #345131 - Flags: review?(jst)
Blocks: 468835
Blocks: 471330
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
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.
Flags: wanted1.9.1?
If it's a regression I guess it must have regressed a long time ago.
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 :-/.
Flags: wanted1.9.1? → wanted1.9.1-
It's pre FF1.0. No idea if its a real regression or ever happened.
Flags: wanted1.9.1- → wanted1.9.1?
Sorry Peter. Mid-air collision sets back the flag. :(
> 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.
Flags: wanted1.9.1? → wanted1.9.1-
Attached patch Patch v2 (Unbitrotted) (obsolete) — Splinter Review
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.
Attachment #345131 - Attachment is obsolete: true
Attachment #364616 - Flags: superreview?(peterv)
Attachment #364616 - Flags: review?(peterv)
Attachment #345131 - Flags: superreview?(peterv)
Attachment #345131 - Flags: review?(peterv)
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.
is review blocked on bug 503838?  (almost 3 years since review request)
Whiteboard: [has patch][needs review]
Comment on attachment 364616 [details] [diff] [review]
Patch v2 (Unbitrotted)

I don't think i can safely review this.
Attachment #364616 - Flags: superreview?(peterv)
Attachment #364616 - Flags: review?(peterv)
Attachment #364616 - Flags: review?(ehsan)
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!
Attachment #364616 - Flags: review?(ehsan) → review-
Attached patch Patch v3Splinter Review
Attachment #364616 - Attachment is obsolete: true
Attachment #553447 - Flags: review?(ehsan)
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!  :-)
Attachment #553447 - Flags: review?(ehsan) → review+
Whiteboard: [has patch][needs review]
> 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
...er called.
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.
http://hg.mozilla.org/mozilla-central/rev/170e2522e530
http://hg.mozilla.org/mozilla-central/rev/8fd6d6928229
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.