Last Comment Bug 740784 - Undo (Ctrl+z) in textarea adding a newline (\n) to the text
: Undo (Ctrl+z) in textarea adding a newline (\n) to the text
: regression, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla14
Assigned To: Graeme McCutcheon [:graememcc]
: Makoto Kato [:m_kato]
Depends on:
Blocks: 483651
  Show dependency treegraph
Reported: 2012-03-30 06:10 PDT by Drew Waddell
Modified: 2012-04-27 06:53 PDT (History)
7 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (1.14 KB, text/html)
2012-03-31 16:56 PDT, Nickolay_Ponomarev
no flags Details
v1 (4.06 KB, patch)
2012-04-13 03:17 PDT, Graeme McCutcheon [:graememcc]
ehsan: review+
lukasblakk+bugs: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description User image Drew Waddell 2012-03-30 06:10:20 PDT
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:

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".
Comment 1 User image Wes Kocher (:KWierso) 2012-03-30 21:55:51 PDT
Can you test this with a current Firefox Nightly build from 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 @
[23:53:01.114] 0  @
[23:53:13.893] 11 Delete Me
Comment 2 User image Nickolay_Ponomarev 2012-03-31 16:56:06 PDT
I can reproduce on a nightly, here's a jquery-less testcase:

Note also bug 471319 and bug 523441.
Comment 3 User image Nickolay_Ponomarev 2012-03-31 16:56:30 PDT
Created attachment 611231 [details]
Comment 4 User image Alice0775 White 2012-04-08 10:47:00 PDT
Regression window(m-c)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110814 Firefox/8.0a1 ID:20110814030749
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110814 Firefox/8.0a1 ID:20110814044816

Regression window(m-i)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110812 Firefox/8.0a1 ID:20110812113455
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110812 Firefox/8.0a1 ID:20110812125630

437f3eb67c18	Graeme McCutcheon — Bug 483651 - Trailing <br> node not removed when it should be; r=ehsan
Comment 5 User image :Ehsan Akhgari 2012-04-09 12:21:30 PDT
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.
Comment 6 User image Graeme McCutcheon [:graememcc] 2012-04-13 03:17:37 PDT
Created attachment 614715 [details] [diff] [review]

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.
Comment 7 User image :Ehsan Akhgari 2012-04-16 12:24:40 PDT
Comment on attachment 614715 [details] [diff] [review]

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.
Comment 8 User image :Ehsan Akhgari 2012-04-17 15:16:39 PDT
Comment on attachment 614715 [details] [diff] [review]

Zero risk for mobile as the undo functionality does not exist there.
Comment 9 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-04-18 15:45:29 PDT
Comment on attachment 614715 [details] [diff] [review]

[triage comment]
low/no risk to mobile.
Comment 12 User image Graeme McCutcheon [:graememcc] 2012-04-26 01:54:22 PDT
I changed the test to waitForFocus, per comment 7:

The others should be taken care of Ms2ger's patch in bug 747346
Comment 13 User image Ed Morley [:emorley] 2012-04-27 06:53:08 PDT

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