Closed
Bug 740784
Opened 12 years ago
Closed 12 years ago
Undo (Ctrl+z) in textarea adding a newline (\n) to the text
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: drew.waddell, Assigned: graememcc)
References
Details
(Keywords: regression, testcase)
Attachments
(2 files)
1.14 KB,
text/html
|
Details | |
4.06 KB,
patch
|
ehsan.akhgari
:
review+
lsblakk
:
approval-mozilla-central+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0 Build ID: 20120312181643 Steps to reproduce: I have text in a textarea that is delete and then restored using Ctrl+z. When the text is restored an mysterious \n is added to the end of the text. Here is an example of the the bug: http://jsfiddle.net/dwaddell/S9mAv/18/ I have tested this in IE and Chorme and it works properly. Actual results: If the text was "abc" it will end up being "abc\n". Expected results: The text should be restored as "abc".
Can you test this with a current Firefox Nightly build from nightly.mozilla.org and see if it still happens? For me in Nightly, if I follow the steps in that jsfiddle, I see the following things logged to the console, and there is no \n in the textarea: [23:52:45.290] 9 Delete Me @ http://fiddle.jshell.net/dwaddell/S9mAv/18/show/:33 -- [23:53:01.114] 0 @ http://fiddle.jshell.net/dwaddell/S9mAv/18/show/:41 -- [23:53:13.893] 11 Delete Me @ http://fiddle.jshell.net/dwaddell/S9mAv/18/show/:41
Comment 2•12 years ago
|
||
I can reproduce on a nightly, here's a jquery-less testcase: http://jsfiddle.net/S9mAv/20/ Note also bug 471319 and bug 523441.
Status: UNCONFIRMED → NEW
Component: Untriaged → Editor
Ever confirmed: true
Keywords: testcase
OS: Windows 7 → All
Product: Firefox → Core
QA Contact: untriaged → editor
Hardware: x86_64 → All
Summary: Ctrl+z in textarea adding a \n to the text → Undo (Ctrl+z) in textarea adding a newline (\n) to the text
Version: 11 Branch → Trunk
Comment 3•12 years ago
|
||
Comment 4•12 years ago
|
||
Regression window(m-c) Works: http://hg.mozilla.org/mozilla-central/rev/145c98d55ae1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110814 Firefox/8.0a1 ID:20110814030749 Fails: http://hg.mozilla.org/mozilla-central/rev/2de3cff973b2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110814 Firefox/8.0a1 ID:20110814044816 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=145c98d55ae1&tochange=2de3cff973b2 Regression window(m-i) Works: http://hg.mozilla.org/integration/mozilla-inbound/rev/56eaf58dc5fb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110812 Firefox/8.0a1 ID:20110812113455 Fails: http://hg.mozilla.org/integration/mozilla-inbound/rev/437f3eb67c18 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110812 Firefox/8.0a1 ID:20110812125630 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=56eaf58dc5fb&tochange=437f3eb67c18 Suspected: 437f3eb67c18 Graeme McCutcheon — Bug 483651 - Trailing <br> node not removed when it should be; r=ehsan
Blocks: 483651
Keywords: regression
Comment 5•12 years ago
|
||
The cause of this bug is that nsTextEditorState::mCachedValue contains the value with the trailing BR. The value that the underlying editor has is actually correct.
Assignee | ||
Comment 6•12 years ago
|
||
Essentially the opposite of bug 483651. Post-undo, the trailing BR does not get the mozBR attribute re-instated, so it gets interpreted as a newline. Green on try, and fixes another test that had been forced to rely on the broken behaviour.
Attachment #614715 -
Flags: review?(ehsan)
Comment 7•12 years ago
|
||
Comment on attachment 614715 [details] [diff] [review] v1 Review of attachment 614715 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments below addressed. Thanks for your patch! ::: editor/libeditor/text/nsTextEditRules.cpp @@ +1109,5 @@ > + // Check to see if the trailing BR is a former bogus node - this will have stuck > + // around if we previously morphed a trailing node into a bogus node > + nsCOMPtr<nsIContent> lastBR = do_QueryInterface(lastChild); > + if (!mEditor->IsMozEditorBogusNode(lastBR)) > + return NS_OK; Nit: please use braces. @@ +1112,5 @@ > + if (!mEditor->IsMozEditorBogusNode(lastBR)) > + return NS_OK; > + > + // Morph it back to a mozBR > + dom::Element* elem = lastBR->AsElement(); Nit: Please add MOZ_ASSERT(lastBR->IsElement()) before this line as a sanity check. ::: editor/libeditor/text/tests/test_bug740784.html @@ +14,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"> > +</head> > + > +<body onload="doTest();"> Please use waitForFocus as you're sending keyboard events.
Attachment #614715 -
Flags: review?(ehsan) → review+
Comment 8•12 years ago
|
||
Comment on attachment 614715 [details] [diff] [review] v1 Zero risk for mobile as the undo functionality does not exist there.
Attachment #614715 -
Flags: approval-mozilla-central?
Comment 9•12 years ago
|
||
Comment on attachment 614715 [details] [diff] [review] v1 [triage comment] low/no risk to mobile.
Attachment #614715 -
Flags: approval-mozilla-central? → approval-mozilla-central+
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d54fc7d18b
Flags: in-testsuite+
Target Milestone: --- → mozilla14
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a7d54fc7d18b
Assignee: nobody → graememcc_firefox
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•12 years ago
|
||
I changed the test to waitForFocus, per comment 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/304cd5d5124b The others should be taken care of Ms2ger's patch in bug 747346
You need to log in
before you can comment on or make changes to this bug.
Description
•