REGRESSION: Major bug with TinyMCE, no value submitted from textarea

• Component: HTML Editing
• Status: RESOLVED FIXED

[Julien KRIVACSY]

As you can check in the example URL, with Safari 2.0.3 (and nightlies), if you submit a form with a textarea and tinyMCE on, no data is sent. I think the bug was not present in Safari 2.0.2 and appeared only with the 10.4.4 update.

[Joost de Valk (AlthA)]

Confirmed, this one needs reduction... Changing component to HTML editing and version to 420+, note this is a regression since 10.4.4 as per reporter's report.

[Justin Garcia]

Created attachment 6117 [details]
reduction

It seems that if a textarea isn't rendered, the setter doesn't work.  This reduction just tries to set the value of a textarea with display:none.

[Justin Garcia]

This regression was caused by the changes for <http://bugzilla.opendarwin.org/show_bug.cgi?id=3401>.  I have a question about textareas and \r\ns.  
(1) We turn \r\ns into \ns when tokenizing, so that any \r\ns that are inside <textarea></textarea>s in the source are turned into \ns, are sent into the renderer as \n and are returned by .value and .defaultValue as \n.  
(2) When someone sets .value, we send \r\ns into the renderer, but translate them into \ns in -[KWQTextArea text], so that they are returned as \n when calling .value (That is, unless .value is called on a textarea with no renderer, in which case we incorrectly return .defaultValue instead of .value, which is this bug).
(3) When someone sets .defaultValue, we put \r\n into the DOM, send \r\n to the renderer, but _do not_ translate them back when asked for .defaultValue.
(4) -[KWQTextArea setSelectedRange] and -[KWQTextArea selectedRange] work around (2)

My question is, why put \r\ns into the renderer?  What do you think about converting \r\ns into \ns in setDefaultValue and setValue?  It seems to me that doing so would make the strings returned by .value and .defaultValue consistent, and would eliminate the need for the workarounds in setSelectedRange and selectedRange (4).

[Justin Garcia]

Created attachment 6170 [details]
patch

Replaces \r\ns and \rs with \n in setValue so that \rs never get into the renderer.  Removes the code from KWQTextArea for working around \rs.  Only retrieve text with hard line wraps when appending form data, not when returning .value, to match the spec and other browsers.

Now that the renderer and the textarea element track the same value (a \rless string without \ns for hard line wraps), some simplification is possible.  The HTMLTextAreaElementImpl's value only becomes invalidated on RenderTextArea::slotTextChanged.  A textareas m_value is only recomputed from the renderer if 1) the renderer is about to be destroyed or 2) if m_value is invalid and someone calls .value on a rendered textarea.

Also fixes <rdar://problem/3968059> Textarea with hard-wrap: pre-filled text doesn't get hard-wrapped

[Darin Adler]

Comment on attachment 6170 [details]
patch

I'm not sure this handles cases where text with \r\n is pasted directly into the <textarea>. Did you test that?

[Darin Adler]

Comment on attachment 6170 [details]
patch

It seems to me that \r\n and \r need to be changed to \n when text is put into the <textarea> directly. Otherwise, this patch looks perfect. Setting review- so you can tackle that part.

[Darin Adler]

Directly, meaning by dragging text in, pasting text, or some other technique like that.

[Justin Garcia]

In -[KWQTextAreaTextView paste] and -[KWQTextAreaTextView performDragOperation] I could let the super NSTextView do the operation and then replace \r\ns and \rs with \ns, but that would have the effect of sending an extra textDidChangeInTextArea notification.  An extra notification would have no ill effects to the listeners we know of (WebBrowsers form delegate).  Or, I could ... no, I will not replace \r\ns and \rs on the pasteboard before calling the super's paste/drag operation.

[Justin Garcia]

Hmm, is there another option?

[Darin Adler]

I'm pretty sure there is a way to catch the stuff as it's pasted in some NSTextView bottleneck.

Maybe this delegate?

    - (BOOL)textView:(NSTextView *)textView shouldChangeTextInRange:(NSRange)affectedCharRange replacementString:(NSString *)replacementString;

[Julien KRIVACSY]

Hello,
Any news of progression with the bug ?
Thank you. Julien

[Justin Garcia]

I'm going to get my revised patch for this landed asap.

[Justin Garcia]

Created attachment 6695 [details]
patch

This patch is described above, but also includes code to normalize line endings coming into the textarea via pasting or drag and drop.  I also renamed m_valueIsValid to m_valueMatchesRenderer, because I think that makes its meaning clearer.  I am currently working on a set of automated tests, and will post those shortly.  

Yes this code will be blown away soon, but the time between now and when the new textarea implementation is landed is too long to let this terrible bug sit in the tree (and 10.4.5) imo.

[Darin Adler]

Comment on attachment 6695 [details]
patch

In new code, please use String instead of DOMString. DOMString is a synonym allowed for backward compatibility, but should not be used in anything new.

Since HTMLTextAreaElementImpl::attach and HTMLTextAreaElementImpl::detach no longer do any special work and just call the base class, we should remove them altogether.

Why change notification to aNotification in textDidChange?

Comment says that turning \r into \n is "just for good measure". But isn't it important to do it for form uploading so that all line breaks are the same?

+        [string replaceOccurrencesOfString:@"\r" withString:@"\n" options:NSLiteralSearch range:range];
+        [string replaceOccurrencesOfString:@"\r\n" withString:@"\n" options:NSLiteralSearch range:range];

The above code will change \r\n into \n\n, right? We need to do the \r\n case first.

Why add the blank lines after calling textDidChangeInTextArea?

Shouldn't we do the "rangeToNormalize" logic before calling widget->textChanged()?

It's fragile to count on the fact that rangeToNormalize is still a good range -- if we're off by a character we'll get an Objective-C exception. Are we certain the text field can't change between where rangeToNormalize is set and where it's used?

Now that newlineSet is not a character set any more, maybe we can just use plain old "find" calls instead of an NSScanner, but I suppose it's not important if this code is going away.

Setting review- because of the "\r\n" after "\r" issue and normalizing before calling textChanged() on the widget.

[Justin Garcia]

Created attachment 6767 [details]
patch

(In reply to comment #14)
> 
> Comment says that turning \r into \n is "just for good measure". But isn't it
> important to do it for form uploading so that all line breaks are the same?

No, when we're about to submit form data, we turn all 3 line endings into \r\n as per the w3c spec.  I changed the comment to "we turn \r\ns into \ns so that a line ending is always a single character, and we turn \rs into \ns to simplify code that finds paragraph boundaries."

> +        [string replaceOccurrencesOfString:@"\r" withString:@"\n"
> options:NSLiteralSearch range:range];
> +        [string replaceOccurrencesOfString:@"\r\n" withString:@"\n"
> options:NSLiteralSearch range:range];
> 
> The above code will change \r\n into \n\n, right? We need to do the \r\n case
> first.

Oops, I don't know why I swapped the order, baad mistake.

> It's fragile to count on the fact that rangeToNormalize is still a good range
> -- if we're off by a character we'll get an Objective-C exception. Are we
> certain the text field can't change between where rangeToNormalize is set and
> where it's used?

I'm not certain.  Instead of saving rangeToNormalize, I just set a bool if the replacementString contains \rs, and then I consider the entire textarea as possibly having \rs in textDidChange.

I added two tests, one for this bug and another for <rdar://problem/3968059> Textarea with hard-wrap: pre-filled text doesn't get hard-wrapped.

[Darin Adler]

Comment on attachment 6767 [details]
patch

r=me

[Julien KRIVACSY]

Solved for me too, thank you.