Bug 6893 - REGRESSION: Major bug with TinyMCE, no value submitted from textarea
Summary: REGRESSION: Major bug with TinyMCE, no value submitted from textarea
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Justin Garcia
URL: http://www.emprint.fr/webkit/test2.html
Keywords: HasReduction, Regression
Depends on:
Blocks: 6627
  Show dependency treegraph
 
Reported: 2006-01-28 15:32 PST by Julien KRIVACSY
Modified: 2006-02-28 03:28 PST (History)
2 users (show)

See Also:


Attachments
reduction (166 bytes, text/html)
2006-01-30 14:12 PST, Justin Garcia
no flags Details
patch (18.21 KB, patch)
2006-01-31 17:05 PST, Justin Garcia
darin: review-
Details | Formatted Diff | Diff
patch (15.11 KB, patch)
2006-02-23 22:14 PST, Justin Garcia
darin: review-
Details | Formatted Diff | Diff
patch (20.37 KB, patch)
2006-02-27 20:49 PST, Justin Garcia
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien KRIVACSY 2006-01-28 15:32:31 PST
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.
Comment 1 Joost de Valk (AlthA) 2006-01-30 00:16:59 PST
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.
Comment 2 Justin Garcia 2006-01-30 14:12:39 PST
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.
Comment 3 Justin Garcia 2006-01-30 19:25:12 PST
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).
Comment 4 Justin Garcia 2006-01-31 17:05:32 PST
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
Comment 5 Darin Adler 2006-01-31 19:05:17 PST
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?
Comment 6 Darin Adler 2006-01-31 19:13:34 PST
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.
Comment 7 Darin Adler 2006-01-31 19:13:59 PST
Directly, meaning by dragging text in, pasting text, or some other technique like that.
Comment 8 Justin Garcia 2006-01-31 20:19:56 PST
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.
Comment 9 Justin Garcia 2006-01-31 20:24:30 PST
Hmm, is there another option?
Comment 10 Darin Adler 2006-02-01 10:20:17 PST
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;
Comment 11 Julien KRIVACSY 2006-02-23 16:48:59 PST
Hello,
Any news of progression with the bug ?
Thank you. Julien
Comment 12 Justin Garcia 2006-02-23 17:04:27 PST
I'm going to get my revised patch for this landed asap.
Comment 13 Justin Garcia 2006-02-23 22:14:57 PST
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.
Comment 14 Darin Adler 2006-02-24 19:54:44 PST
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.
Comment 15 Justin Garcia 2006-02-27 20:49:46 PST
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.
Comment 16 Darin Adler 2006-02-27 22:44:25 PST
Comment on attachment 6767 [details]
patch

r=me
Comment 17 Julien KRIVACSY 2006-02-28 03:28:53 PST
Solved for me too, thank you.