Bug 56434 - REGRESSION (r64712): Safari removes the first blank line in a textarea
: REGRESSION (r64712): Safari removes the first blank line in a textarea
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P1 Normal
Assigned To:
: http://www.pgdp.org/~acunning40/temp/...
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2011-03-15 17:40 PST by
Modified: 2011-03-31 19:32 PST (History)


Attachments
Patch (4.74 KB, patch)
2011-03-28 19:41 PST, Naoki Takano
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.24 KB, patch)
2011-03-28 21:04 PST, Naoki Takano
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.47 KB, patch)
2011-03-28 23:02 PST, Naoki Takano
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.83 KB, patch)
2011-03-28 23:18 PST, Naoki Takano
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2011-03-31 18:54 PST, Naoki Takano
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-15 17:40:17 PST
When WebKit loads a textarea, it removes the first blank line it finds in it.

Steps to reproduce:
1) Open the URL in Safari, look at the textareas.
2) Open the URL in Firefox 4 (not 3), look again.
3) Firefox 4 keeps the blank lines, WebKit doesn't.

Additional notes:
a) Reported in Firefox here: https://bugzilla.mozilla.org/show_bug.cgi?id=591988
b) Fixed in Firefox here: https://bugzilla.mozilla.org/show_bug.cgi?id=620267
------- Comment #1 From 2011-03-16 10:47:17 PST -------
Works in Safari 5.0.4.
------- Comment #2 From 2011-03-16 10:47:39 PST -------
<rdar://problem/9142454>
------- Comment #3 From 2011-03-16 22:04:32 PST -------
This regressed in nightly r64727. So, the culprit revision is in [64637, 64727]. If I have some time then I'll try to bisect this further/look through the change log entries in this range.
------- Comment #5 From 2011-03-16 22:45:54 PST -------
Adam & Eric,

Can this be a HTML5 Tree Builder regression?
------- Comment #6 From 2011-03-16 22:47:28 PST -------
Anything could be an HTML5 regression. :)  Although this may also be an intentional change in HTML5.  We'd need to check the spec.
------- Comment #7 From 2011-03-16 22:48:14 PST -------
I suspect we will be finding (long tail) fallout from our HTML5 parser re-write for years.
------- Comment #8 From 2011-03-16 22:53:04 PST -------
Differences between Firefox 4 are likely to be bugs in our code.
------- Comment #9 From 2011-03-16 22:59:11 PST -------
I know there is special code for removing the first \n in a <pre> tag.  I'm not sure about <textarea> without checking the spec.
------- Comment #10 From 2011-03-18 14:14:18 PST -------
Just confirming that this is caused by turning on the HTML5 tree builder in <http://trac.webkit.org/changeset/64712>.
------- Comment #11 From 2011-03-18 15:40:04 PST -------
Capturing test case in case the site goes down:


<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> 
<html lang='en'> 
<head> 
<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1"> 
</head> 

<body> 
<p><textarea cols="20" rows="6">No blank lines here</textarea></p> 
<p><textarea cols="20" rows="6"> 
No blank lines here</textarea></p> 
<p><textarea cols="20" rows="6"> 

One blank line here</textarea></p> 
<p><textarea cols="20" rows="6"> 


Two blank lines here</textarea></p> 
</body> 
</html>
------- Comment #12 From 2011-03-18 15:46:47 PST -------
Looking in the Web Inspector, it looks like the DOM has the proper number of blank lines.  I suspect we used to strip the initial blank line in the render tree rather than in the parser.  We're now probably stripping it twice (both in the parser and in the render tree).
------- Comment #13 From 2011-03-28 19:41:24 PST -------
Created an attachment (id=87250) [details]
Patch
------- Comment #14 From 2011-03-28 19:41:53 PST -------
(From update of attachment 87250 [details])
Please review.
------- Comment #15 From 2011-03-28 20:16:56 PST -------
(From update of attachment 87250 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=87250&action=review

> LayoutTests/fast/forms/textarea-newline.html:12
> +<script>
> +    function checkValue(id, value) {
> +        var ta = document.getElementById(id);
> +        if (ta.value == value)
> +            document.getElementById(id + '_res').innerHTML = "Test Passed";
> +
> +    }
> +    function test() {
> +        if (window.layoutTestController)
> +            layoutTestController.dumpAsText();

This test is acceptable, however I recommend to use fast/js/resources/js-test-pre.js and js-test-post.js.  They make the test simpler.

> Source/WebCore/ChangeLog:8
> +        REGRESSION (r64712): Safari removes the first blank line in a textarea
> +        https://bugs.webkit.org/show_bug.cgi?id=56434
> +
> +        Test: fast/forms/textarea-newline.html

Would you write a reason why HTML5 tree builder made the regression and why this change fixes it please?
------- Comment #16 From 2011-03-28 21:04:36 PST -------
Created an attachment (id=87255) [details]
Patch
------- Comment #17 From 2011-03-28 21:05:17 PST -------
(From update of attachment 87255 [details])
Please review.
------- Comment #18 From 2011-03-28 21:19:31 PST -------
(From update of attachment 87255 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=87255&action=review

> LayoutTests/fast/forms/textarea-newline-expected.txt:10
> +No line feed. 
> + 
> +One line feed after textarea tag is ignored. 
> + 
> +Two line feeds after textarea tag and the first one is ignored. 
> + 
> +One line feed escaped character after textarea tag and line feed and both are not ignored. 
> + 
> +Two line feed escaped characters after textarea tag and both are not ignored. 

The readability of the test result is not good.  These five sentences are confusing.
I recommend:
 - Remove them, and
 - Print them by debug() function, or change ID names.

e.g.
debug('No line feed.');
var textarea = document.getElementById('Madoka');
shouldBe('textarea.value', '"Madoka"');

or

Replacing id="Madoka" with id="no-line-feed", and
shouldBe('document.getElementById("no-line-feed").value', '"Madoka"');

> LayoutTests/fast/forms/textarea-newline.html:46
> +var textarea = document.getElementById('Madoka');
> +shouldBe('textarea.value', '"Madoka"');
> +
> +var textarea = document.getElementById('Sayaka');

Declaring "var textarea" multiple times is not good.

> Source/WebCore/ChangeLog:12
> +        A linefeed removal after a textarea tag is originally processed in WebCore::HTMLTextAreaElement::defaultValue().
> +        But HTML5 tree builder now removes the linefeed. It means linefeed removal happens twice.
> +        And devalutValue() removal is not needed anymore.

Great.  Thank you.
------- Comment #19 From 2011-03-28 23:02:28 PST -------
Created an attachment (id=87263) [details]
Patch
------- Comment #20 From 2011-03-28 23:02:46 PST -------
(From update of attachment 87263 [details])
Review again.
------- Comment #21 From 2011-03-28 23:04:26 PST -------
(From update of attachment 87263 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=87263&action=review

> LayoutTests/fast/forms/textarea-newline.html:11
> +<br>
> +No line feed.

Please remove it.

> LayoutTests/fast/forms/textarea-newline.html:17
> +<br>
> +One line feed after textarea tag is ignored.
> +<br>

ditto.

> LayoutTests/fast/forms/textarea-newline.html:23
> +<br>
> +Two line feeds after textarea tag and the first one is ignored.
> +<br>

ditto.

> LayoutTests/fast/forms/textarea-newline.html:30
> +<br>
> +One line feed escaped character after textarea tag and line feed and both are not ignored.
> +<br>

ditto.

> LayoutTests/fast/forms/textarea-newline.html:36
> +<br>
> +Two line feed escaped characters after textarea tag and both are not ignored.
> +<br>

ditto.
------- Comment #22 From 2011-03-28 23:18:29 PST -------
Created an attachment (id=87266) [details]
Patch
------- Comment #23 From 2011-03-28 23:19:34 PST -------
(From update of attachment 87266 [details])
Great!
------- Comment #24 From 2011-03-28 23:19:44 PST -------
(From update of attachment 87266 [details])
Sorry for bothering you again and again.

Please review.
------- Comment #25 From 2011-03-29 05:46:28 PST -------
(From update of attachment 87266 [details])
Rejecting attachment 87266 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
.........................................................................................................................................................................................................................................................
fast/forms/textarea-default-value-leading-newline.html -> failed

Exiting early after 1 failures. 8825 tests run.
212.47s total testing time

8824 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8273842
------- Comment #26 From 2011-03-29 05:58:06 PST -------
(In reply to comment #25)
> fast/forms/textarea-default-value-leading-newline.html -> failed

Yeah, we need to update setDefaultValue() too.
------- Comment #27 From 2011-03-31 18:54:26 PST -------
Created an attachment (id=87815) [details]
Patch
------- Comment #28 From 2011-03-31 18:55:42 PST -------
(From update of attachment 87815 [details])
Fixed setDefaultValue() function.

Please review again.
------- Comment #29 From 2011-03-31 19:32:44 PST -------
(From update of attachment 87815 [details])
Clearing flags on attachment: 87815

Committed r82656: <http://trac.webkit.org/changeset/82656>
------- Comment #30 From 2011-03-31 19:32:52 PST -------
All reviewed patches have been landed.  Closing bug.