596455 - <br> is deleted from textarea after saving (Movable Type)

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[pal-moz]

Movable Type v4.27 (latest is 5.03)
http://www.movabletype.org/


[STR]
(login needed)
edit/create new entry.
input text in textarea, like below.
 aaaaa
 bbbbb
 ccccc
save entry.
(created blog page looks OK)
but now text in textarea is
 aaaabbbbccccdddd

if save again at this point, text on created page also change from
aaaaa
bbbbb
ccccc
to
aaaaabbbbbccccc

Comment 1

[pal-moz]

Attachment: screenshot

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I tried this in the movable type I have access to, and I can't reproduce.  But that's MT 3.something....

In that screenshot, is MT really using a textarea?  Or a designmode kind of thing?  What happens if you click those formatting buttons?

Comment 3

[pal-moz]

buttons are javascript like
<a href="javascript: void 0;" title="テキストサイズを小さくする" class="command-font-size-smaller toolbar button"><b>a</b></a>

around this test input area,

<div class="editor" id="editor-content-enclosure" mt:min-height="66" mt:update-field-height="editor-content-height" style="height: 195px">
<textarea id="editor-content-textarea" name="_text_" class="full-width" style="background: #fff; height: 195px"></textarea>
<!-- the iframe bootstraps the js -->

<iframe tabindex="3" id="editor-content-iframe" frameborder="0" scrolling="yes" src="http://my-domain/MT4/mt-static/html/editor-content.html?cs=UTF-8" style="height: 195px"></iframe>
<input type="hidden" name="text_height" id="editor-content-height" value="195" />
<input type="hidden" id="editor-input-content" name="text" value="aaaaa
bbbbb
ccccc" />
<input type="hidden" id="editor-input-extended" name="text_more" value="" />
<div class="resizer" mt:delegate="resizer" mt:target="editor-content-enclosure" mt:lock="x">
<img src="http://my-domain/MT4/mt-static/images/spacer.gif" width="100%" height="10"/>
</div>
</div>


BTW,
using text-input (format) mode is "Convert Line Breaks"
other modes are "Rich Text","Markdown","Markdown+SmartyPants","Textile 2"

Comment 4

[Boris Zbarsky [:bzbarsky]]

> <div class="editor" id="editor-content-enclosure" mt:min-height="66"
> mt:update-field-height="editor-content-height" style="height: 195px">
> <textarea id="editor-content-textarea" name="_text_" class="full-width"
> style="background: #fff; height: 195px"></textarea>

Right, so which of them are you actually editing in?  The editable HTML div or the textarea?

> using text-input (format) mode is "Convert Line Breaks"

Meaning what?

Comment 5

[pal-moz]

>Right, so which of them are you actually editing in?  The editable HTML div or
the textarea?

no idea.
test seems to be in
<input type="hidden" id="editor-input-content" name="text" value="aaaaa
bbbbb
ccccc" />


> "Convert Line Breaks"
http://www.movabletype.org/documentation/author/text-formatting.html
"This format will automatically enclose your paragraphs in p tags, and converts line breaks to br tags."

Comment 6

[Boris Zbarsky [:bzbarsky]]

> no idea.

Well, when you click the formatting buttons above your edit area, does your text become bold/italic/whatever?

Comment 7

[pal-moz]

(In reply to comment #6)
> > no idea.
> 
> Well, when you click the formatting buttons above your edit area, does your
> text become bold/italic/whatever?

select text and click button,
text change like below

aaaaa >>> <b>aaaaa</b>
aaaaa >>> <i>aaaaa</i>

Comment 8

[Boris Zbarsky [:bzbarsky]]

As in, it shows the tags in the area you're editing?

Comment 9

[(no longer active)]

Is it possible for me to get a test account on your installation so that I can debug this?  If yes, would you mind sending the login info to my email address?  Thanks!

Comment 10

[pal-moz]

(In reply to comment #9)
> Is it possible for me to get a test account on your installation so that I can
> debug this?  If yes, would you mind sending the login info to my email address?
>  Thanks!

sent a mail.
title "Bug 596455 - <br> is deleted from textarea after saving (Movable Type)"

Comment 11

[pal-moz]

(In reply to comment #10)
> (In reply to comment #9)
> > Is it possible for me to get a test account on your installation so that I can
> > debug this?  If yes, would you mind sending the login info to my email address?
> >  Thanks!
> 
> sent a mail.
> title "Bug 596455 - <br> is deleted from textarea after saving (Movable Type)"

if needed, share it with other developers.

Comment 12

[Biju]

I may be wrong from the test case it appear similar to bug 559261
to solve I modified function viewsource(source) 
see attachment 438938 [details] for details

It it same here is the simple test to reproduce 

STEPS
1. go to http://www.mozilla.org/editor/midasdemo/
2. check "view HTML source"
3. type following 3 line of text
aaaaa
bbbbb
ccccc
4. uncheck "view HTML source"
5. again check "view HTML source"

Result: you get "aaaaabbbbbccccc"


This may be a bug in 
    htmlrange.toString(); 
and as well as 
   Node.textContent

as both
1.
  var htmlrange = document.body.ownerDocument.createRange();
  Node.innerHTML = "aaaa<br>bbbb<br>cccc";
  htmlrange.selectNodeContents(Node);
  alert(htmlrange.toString()); 

2.
   Node.innerHTML = "aaaa<br>bbbb<br>cccc";
   alert(Node.textContent);

gives "aaaabbbbcccc"

Comment 13

[Boris Zbarsky [:bzbarsky]]

That's the right behavior for .textContent and .toString on ranges, yes.  Both should return just the text involved, ignoring any tags.

Comment 14

[(no longer active)]

The page in question has this element:

<input type="hidden" id="editor-input-content" name="text" value="aaaa
bbbb
cccc" />

And it seems that it's setting the textarea value based on the value of this element.  We basically sanitize this value too soon, before we know the type.  Mounir, do you have any idea how best to solve this?

Comment 15

[(no longer active)]

Note that the first case is handled correctly because we set the attributes from last to first:

<input value="foo
bar" type="hidden">

<input type="hidden" value="foo
bar">

Comment 16

[Mounir Lamouri (:mounir)]

I think there is an easy fix: we have BF_PARSER_CREATING we can use to _not_
sanitize the value while true and then sanitize the value when the parser is
done creating the element.

Considering it's a regression, I'm asking for a blocker flag.

Comment 17

[Mounir Lamouri (:mounir)]

testcase in URL

Comment 18

[(no longer active)]

I made nsHTMLInputElement::SanitizeValue a no-op to test my theory, in comment 14 and 15, and indeed with that, the problem doesn't happen.  So this doesn't have anything to do with bug 240933 after all...

Comment 19

[(no longer active)]

I spoke too soon!

Consider this test case:

data:text/html,<input type=hidden value="foo&#13;bar"><script>alert(document.querySelector("input").value.length)</script>

You'll get 7 on a build from 9-11, and you'll get 6 on trunk.  So, bug 240933 has changed _something_ here.  bz is bisecting this...

Comment 20

[Boris Zbarsky [:bzbarsky]]

The first bad revision is:
changeset:   53749:7a5fed78b780
user:        Mounir Lamouri <[e-mail]>
date:        Tue Sep 14 02:23:03 2010 +0200
summary:     Bug 590363 - Implement the new input type change algorithm. r=sicking a2.0=blocking

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment 16 sounds like the right approach.

That said, it sounds like we're sanitizing text inputs on value get, not on attr set?  Or something?  Is that correct?  Can the difference be detected (e.g. via onchange events)?

Comment 22

[Boris Zbarsky [:bzbarsky]]

This needs to block beta7; we're now corrupting hidden input values depending on the attr order...

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 24

[(no longer active)]

Comment on attachment 476390 [details] [diff] [review]
Patch v1

Firstly, I don't really like the way that you're testing this.  I think it's better to write a mochitest which makes sure that CR and LF characters are not stripped in hidden input controls (btw, setting display:none on the control is not the same thing as type=hidden, so I don't think we need to test it here).

On the implementation, isn't it better to set a flag like BF_SHOULD_SANITIZE, and only do the sanitization in DoneCreatingElement if that flag is present, the same way that BF_SHOULD_INIT_CHECKED works?

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to comment #24)
> Comment on attachment 476390 [details] [diff] [review]
> Patch v1
> 
> Firstly, I don't really like the way that you're testing this. 

Reftests are less CPU consuming than mochitests, that's why I used reftests.

> (btw, setting display:none on the control is
> not the same thing as type=hidden, so I don't think we need to test it here).

It's not to test the same thing. It's to be sure <input type='text'> will have it's value sanitized when parsed.

> On the implementation, isn't it better to set a flag like BF_SHOULD_SANITIZE,
> and only do the sanitization in DoneCreatingElement if that flag is present,
> the same way that BF_SHOULD_INIT_CHECKED works?

We want to sanitize when the parsing is done and not during parsing. I don't really understand why we should add yet another boolean for that. With another boolean, it would be unclear (first reaction would be to check when the value is changing..). If the issue is we are relying on a boolean which may be changed someday I will say, we have tests and the regression will be catched.

Comment 26

[(no longer active)]

(In reply to comment #25)
> (In reply to comment #24)
> > Comment on attachment 476390 [details] [diff] [review]
> > Patch v1
> > 
> > Firstly, I don't really like the way that you're testing this. 
> 
> Reftests are less CPU consuming than mochitests, that's why I used reftests.

But you're testing the wrong thing...  CRs are not rendered, so I think that the reftest will pass even without this test.  I'd say it's best not to optimize for a few microseconds here, and just write a mochitest which tests the right thing.

> > (btw, setting display:none on the control is
> > not the same thing as type=hidden, so I don't think we need to test it here).
> 
> It's not to test the same thing. It's to be sure <input type='text'> will have
> it's value sanitized when parsed.

Hmm, don't we already have tests for that?  If not, then yes, we'd definitely need this test.

> > On the implementation, isn't it better to set a flag like BF_SHOULD_SANITIZE,
> > and only do the sanitization in DoneCreatingElement if that flag is present,
> > the same way that BF_SHOULD_INIT_CHECKED works?
> 
> We want to sanitize when the parsing is done and not during parsing. I don't
> really understand why we should add yet another boolean for that. With another
> boolean, it would be unclear (first reaction would be to check when the value
> is changing..). If the issue is we are relying on a boolean which may be
> changed someday I will say, we have tests and the regression will be catched.

I'm kind of worried that calling GetValue/SetValueInternal after each element is created needlessly would open us up to weird problems...  But I'll let Boris weigh in there.

Comment 27

[Mounir Lamouri (:mounir)]

(In reply to comment #26)
> But you're testing the wrong thing...  CRs are not rendered, so I think that
> the reftest will pass even without this test.  I'd say it's best not to
> optimize for a few microseconds here, and just write a mochitest which tests
> the right thing.

I'm not sure I understand... there is a line break in textarea's.
And another reason is I need to use the parser. Using mochitests with a static html page and then test the value seems a bit useless.

> Hmm, don't we already have tests for that?  If not, then yes, we'd definitely
> need this test.

I wouldn't bet that I wrote a test for value sanitizing algorithm using the parser with elements created using the parser.

> > We want to sanitize when the parsing is done and not during parsing. I don't
> > really understand why we should add yet another boolean for that. With another
> > boolean, it would be unclear (first reaction would be to check when the value
> > is changing..). If the issue is we are relying on a boolean which may be
> > changed someday I will say, we have tests and the regression will be catched.
> 
> I'm kind of worried that calling GetValue/SetValueInternal after each element
> is created needlessly would open us up to weird problems...  But I'll let Boris
> weigh in there.

I don't see what could go wrong but I believe on Boris to found any problem if there is one ;)

Comment 28

[Boris Zbarsky [:bzbarsky]]

So a few things....

1)  What's the new conceptual setup?  Do we now aim to sanitize consistently on
    set?  Or on get?  Or something else?  It seems now we're sanitizing for
    both, but should only sanitize for set, right?  Note that I haven't
    carefully read over all the code yet (I need to be a lot more awake for
    that, so probably not happening till Monday), so I may just be missing
    something.

2)  The range of what can happen while BF_PARSER_CREATING is set is pretty
    limited: the actual element creation and some SetAttr calls with
    aNotify == false.  A lot of these checks can probably just be asserts that
    the flag isn't set.

I'll do the really careful reading on Monday, like I said...

Comment 29

[Mounir Lamouri (:mounir)]

(In reply to comment #28)
> So a few things....
> 
> 1)  What's the new conceptual setup?  Do we now aim to sanitize consistently on
>     set?  Or on get?  Or something else?  It seems now we're sanitizing for
>     both, but should only sanitize for set, right?  Note that I haven't
>     carefully read over all the code yet (I need to be a lot more awake for
>     that, so probably not happening till Monday), so I may just be missing
>     something.

We only sanitize on set. We sanitize on get because nsTextControlState tries to directly get the default value which shouldn't be done (see bug 597525).
We only sanitize on SetAttr if the default value is used as the value (ie. the dirty value flag is false). IOW, we do not sanitize the default value but only the real value. I guess the idea of the specs is to keep a raw default value which could be re-used whatever the type is.

> 2)  The range of what can happen while BF_PARSER_CREATING is set is pretty
>     limited: the actual element creation and some SetAttr calls with
>     aNotify == false.  A lot of these checks can probably just be asserts that
>     the flag isn't set.

If @value is set, SetAttr will be called and considering the dirty value flag can't be true at this point, the default value will be sanitized when set as the value. We want to prevent this sanitizing and we do it when the element parsing is done. Which means that the value will be incorrect in the meanwhile but I guess we do not care.

> I'll do the really careful reading on Monday, like I said...

That's fine for me. I just hope that it will be fine with the code freeze (there is no news about the new deadline afaik).

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 476390 [details] [diff] [review]
Patch v1

For each of the tests, add another one (so have 1a/1b and 2a/2b) with the value and type attrs in the other order, please.  That way the tests won't accidentally pass if we change the attr setting order and then regress the bug.

With this change, it seems like the GetValue() call and the switch in HandleTypeChange can both be skipped if BF_PARSER_CREATING. Please do that, if so?

r=me with those nits.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Also, in SetValueInternal, should we be doing a value-mode check instead of a IsSingleLineTextControl check?

Comment 32

[Mounir Lamouri (:mounir)]

(In reply to comment #30)
> Comment on attachment 476390 [details] [diff] [review]
> Patch v1
> 
> For each of the tests, add another one (so have 1a/1b and 2a/2b) with the value
> and type attrs in the other order, please.  That way the tests won't
> accidentally pass if we change the attr setting order and then regress the bug.

Ok.

> With this change, it seems like the GetValue() call and the switch in
> HandleTypeChange can both be skipped if BF_PARSER_CREATING. Please do that, if
> so?

I'm not sure what you mean by "the GetValue() call" but we can't change anything interesting in HandleTypeChange switch. The only path that could be prevented if BF_PARSER_CREATING is when the current value mode is VALUE_MODE_VALUE and the previous was VALUE_MODE_VALUE too (because we would try to do GetValue() and SetValueInternal(). All other cases have to stay. I don't think we want to add a lot of checks for this specific situations.

Comment 33

[Mounir Lamouri (:mounir)]

(In reply to comment #31)
> Also, in SetValueInternal, should we be doing a value-mode check instead of a
> IsSingleLineTextControl check?

Just open bug 598119. That's clearly not the only place where we use IsSingleTextControl or we check mType. That's correct but not future-safe (and I think less readable).

Comment 34

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what you mean by "the GetValue() call"

The one at the beginning of HandleTypeChange.

> All other cases have to stay.

Why, if BF_PARSER_CREATING?

In particular, if the new value mode is default or default_on and BF_PARSER_CREATING then our attr is already set to the correct value, no?  So we can skip the SetAttr call.  If the current mode is MODE_VALUE, then the only reason we're doing the SetValueInternal call is to sanitize the value, right?  And if BF_PARSER_CREATING we don't need to do that...

Am I just missing something?

Comment 36

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

I'm going to push that. Let me know if that isn't what you were asking.

Comment 37

[Boris Zbarsky [:bzbarsky]]

That looks great, thanks!

Comment 38

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/82a48093cf93

Comment 39

[pal-moz]

tinderbox-builds/mozilla-central-win32/1285042374
(20100920211254 e6f66ee57044)

seems to be fine.
thanks.

Comment 41

[Mounir Lamouri (:mounir)]

Verified per comment 39.