Bug 56434 - REGRESSION (r64712): Safari removes the first blank line in a textarea
Summary: REGRESSION (r64712): Safari removes the first blank line in a textarea
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P1 Normal
Assignee: Nobody
URL: http://www.pgdp.org/~acunning40/temp/...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-03-15 17:40 PDT by Daniele Metilli
Modified: 2011-03-31 19:32 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Daniele Metilli 2011-03-15 17:40:17 PDT
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 Alexey Proskuryakov 2011-03-16 10:47:17 PDT
Works in Safari 5.0.4.
Comment 2 Alexey Proskuryakov 2011-03-16 10:47:39 PDT
<rdar://problem/9142454>
Comment 3 Daniel Bates 2011-03-16 22:04:32 PDT
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 Ryosuke Niwa 2011-03-16 22:45:54 PDT
Adam & Eric,

Can this be a HTML5 Tree Builder regression?
Comment 6 Eric Seidel (no email) 2011-03-16 22:47:28 PDT
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 Eric Seidel (no email) 2011-03-16 22:48:14 PDT
I suspect we will be finding (long tail) fallout from our HTML5 parser re-write for years.
Comment 8 Adam Barth 2011-03-16 22:53:04 PDT
Differences between Firefox 4 are likely to be bugs in our code.
Comment 9 Eric Seidel (no email) 2011-03-16 22:59:11 PDT
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 Andy Estes 2011-03-18 14:14:18 PDT
Just confirming that this is caused by turning on the HTML5 tree builder in <http://trac.webkit.org/changeset/64712>.
Comment 11 Adam Barth 2011-03-18 15:40:04 PDT
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 Adam Barth 2011-03-18 15:46:47 PDT
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 Naoki Takano 2011-03-28 19:41:24 PDT
Created attachment 87250 [details]
Patch
Comment 14 Naoki Takano 2011-03-28 19:41:53 PDT
Comment on attachment 87250 [details]
Patch

Please review.
Comment 15 Kent Tamura 2011-03-28 20:16:56 PDT
Comment on attachment 87250 [details]
Patch

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 Naoki Takano 2011-03-28 21:04:36 PDT
Created attachment 87255 [details]
Patch
Comment 17 Naoki Takano 2011-03-28 21:05:17 PDT
Comment on attachment 87255 [details]
Patch

Please review.
Comment 18 Kent Tamura 2011-03-28 21:19:31 PDT
Comment on attachment 87255 [details]
Patch

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 Naoki Takano 2011-03-28 23:02:28 PDT
Created attachment 87263 [details]
Patch
Comment 20 Naoki Takano 2011-03-28 23:02:46 PDT
Comment on attachment 87263 [details]
Patch

Review again.
Comment 21 Kent Tamura 2011-03-28 23:04:26 PDT
Comment on attachment 87263 [details]
Patch

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 Naoki Takano 2011-03-28 23:18:29 PDT
Created attachment 87266 [details]
Patch
Comment 23 Kent Tamura 2011-03-28 23:19:34 PDT
Comment on attachment 87266 [details]
Patch

Great!
Comment 24 Naoki Takano 2011-03-28 23:19:44 PDT
Comment on attachment 87266 [details]
Patch

Sorry for bothering you again and again.

Please review.
Comment 25 WebKit Commit Bot 2011-03-29 05:46:28 PDT
Comment on attachment 87266 [details]
Patch

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 Kent Tamura 2011-03-29 05:58:06 PDT
(In reply to comment #25)
> fast/forms/textarea-default-value-leading-newline.html -> failed

Yeah, we need to update setDefaultValue() too.
Comment 27 Naoki Takano 2011-03-31 18:54:26 PDT
Created attachment 87815 [details]
Patch
Comment 28 Naoki Takano 2011-03-31 18:55:42 PDT
Comment on attachment 87815 [details]
Patch

Fixed setDefaultValue() function.

Please review again.
Comment 29 WebKit Commit Bot 2011-03-31 19:32:44 PDT
Comment on attachment 87815 [details]
Patch

Clearing flags on attachment: 87815

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