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)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: danswer, Assigned: graememcc)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Updated•19 years ago
|
Summary: oninput problem with ctrl+z → value is wrong during oninput for ctrl+z (undo) if text was blank before undo
Updated•19 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
Comment 2•16 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•16 years ago
|
Assignee | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
Assignee: events → nobody
Component: Event Handling → Editor
OS: Windows XP → All
QA Contact: ian → editor
Hardware: PC → All
Comment 5•16 years ago
|
||
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•16 years ago
|
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Comment 6•16 years ago
|
||
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.
Comment 9•16 years 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-
Comment 10•16 years ago
|
||
It's pre FF1.0. No idea if its a real regression or ever happened.
Flags: wanted1.9.1- → wanted1.9.1?
Comment 11•16 years ago
|
||
Sorry Peter. Mid-air collision sets back the flag. :(
Assignee | ||
Comment 12•16 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•16 years ago
|
||
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)
Comment 17•16 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.
Updated•15 years ago
|
Depends on: better-invisible-br-handling
Comment 18•14 years ago
|
||
is review blocked on bug 503838? (almost 3 years since review request)
Whiteboard: [has patch][needs review]
Comment 19•14 years ago
|
||
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 20•14 years ago
|
||
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-
Assignee | ||
Comment 22•13 years ago
|
||
Attachment #364616 -
Attachment is obsolete: true
Attachment #553447 -
Flags: review?(ehsan)
Comment 23•13 years ago
|
||
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•13 years ago
|
Whiteboard: [has patch][needs review]
Assignee | ||
Comment 24•13 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•13 years ago
|
||
...er called.
Comment 26•13 years ago
|
||
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 27•13 years ago
|
||
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.
Description
•