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

VERIFIED FIXED in mozilla2.0b7

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: pal-moz, Assigned: mounir)

Tracking

({regression, testcase})

Trunk
mozilla2.0b7
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

8 years ago
Created attachment 475382 [details]
screenshot
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?
(Reporter)

Comment 3

8 years ago
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"
> <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?
(Reporter)

Comment 5

8 years ago
>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."
> no idea.

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

Comment 7

8 years ago
(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>
Keywords: testcase-wanted
As in, it shows the tags in the area you're editing?

Comment 9

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

Comment 10

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

Comment 11

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

8 years ago
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"
That's the right behavior for .textContent and .toString on ranges, yes.  Both should return just the text involved, ignoring any tags.

Comment 14

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

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

Comment 16

7 years ago
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.
Assignee: nobody → mounir.lamouri
Blocks: 549475
Status: NEW → ASSIGNED
blocking2.0: --- → ?
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 17

7 years ago
testcase in URL
Keywords: testcase-wanted → testcase

Comment 18

7 years ago
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...
No longer blocks: 240933

Comment 19

7 years ago
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...
Blocks: 240933
The first bad revision is:
changeset:   53749:7a5fed78b780
user:        Mounir Lamouri <mounir.lamouri@gmail.com>
date:        Tue Sep 14 02:23:03 2010 +0200
summary:     Bug 590363 - Implement the new input type change algorithm. r=sicking a2.0=blocking
Blocks: 590363
No longer blocks: 240933
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)?
This needs to block beta7; we're now corrupting hidden input values depending on the attr order...
blocking2.0: ? → beta7+
(Assignee)

Updated

7 years ago
Component: Editor → DOM: Core & HTML
QA Contact: editor → general
(Assignee)

Comment 23

7 years ago
Created attachment 476390 [details] [diff] [review]
Patch v1
Attachment #476390 - Flags: review?(bzbarsky)
Attachment #476390 - Flags: feedback?(ehsan)

Comment 24

7 years ago
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?
Attachment #476390 - Flags: feedback?(ehsan) → feedback-
(Assignee)

Comment 25

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

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

Comment 27

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

Comment 29

7 years ago
(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 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.
Attachment #476390 - Flags: review?(bzbarsky) → review+
Also, in SetValueInternal, should we be doing a value-mode check instead of a IsSingleLineTextControl check?
(Assignee)

Comment 32

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

Comment 33

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

Comment 36

7 years ago
Created attachment 476900 [details] [diff] [review]
Patch v1.1

I'm going to push that. Let me know if that isn't what you were asking.
That looks great, thanks!
(Assignee)

Comment 38

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/82a48093cf93
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
(Reporter)

Comment 39

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

seems to be fine.
thanks.
Duplicate of this bug: 598343
(Assignee)

Comment 41

7 years ago
Verified per comment 39.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.