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

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?

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?

>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?

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

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

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!

(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)"

(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.

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.

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?

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">

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.

testcase in URL

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...

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...

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

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...

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?

(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.

(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.

(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...

(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.

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

(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.

(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?

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

That looks great, thanks!

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

tinderbox-builds/mozilla-central-win32/1285042374 (20100920211254 e6f66ee57044) seems to be fine. thanks.

Verified per comment 39.