Bug 6893

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.

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.

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

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

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

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.

Hmm, is there another option?

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;

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

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

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

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 on attachment 6767 [details] patch r=me

Solved for me too, thank you.