The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
Editor
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Csaba Gabor, Assigned: graememcc)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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:
(Reporter)

Comment 1

12 years ago
Created attachment 204405 [details]
Testcase for oninput / ctrl+z issue

Updated

12 years ago
Summary: oninput problem with ctrl+z → value is wrong during oninput for ctrl+z (undo) if text was blank before undo

Updated

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

Comment 2

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

Updated

9 years ago
Blocks: 318355, 461673
(Assignee)

Updated

9 years ago
Duplicate of this bug: 320006
(Assignee)

Comment 4

9 years ago
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
Attachment #345131 - Flags: superreview?(jst)
Attachment #345131 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
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)

Updated

8 years ago
Blocks: 468835
(Assignee)

Updated

8 years ago
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?
Is this a regression?
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. :(
(Assignee)

Comment 12

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

Comment 13

8 years ago
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.
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)
Duplicate of this bug: 433574

Updated

8 years ago
Duplicate of this bug: 356895

Updated

8 years ago
Duplicate of this bug: 499677

Comment 17

8 years ago
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.
Depends on: 503838
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-

Updated

6 years ago
Duplicate of this bug: 653204
(Assignee)

Comment 22

6 years ago
Created attachment 553447 [details] [diff] [review]
Patch v3
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+

Updated

6 years ago
Whiteboard: [has patch][needs review]
(Assignee)

Comment 24

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

Comment 25

6 years ago
...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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Updated

6 years ago
Duplicate of this bug: 698726
You need to log in before you can comment on or make changes to this bug.