Bug 56434

Summary:

REGRESSION (r64712): Safari removes the first blank line in a textarea

Product:

WebKit

Reporter:

Daniele Metilli <daniele.metilli>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, adele, aestes, ap, commit-queue, dbates, eric, rniwa, tkent

Priority:

Keywords:

InRadar, Regression

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

URL:

http://www.pgdp.org/~acunning40/temp/textareas.html

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Daniele Metilli

Reported 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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2011-03-16 10:47:17 PDT

Works in Safari 5.0.4.

Alexey Proskuryakov

Comment 2 2011-03-16 10:47:39 PDT

<rdar://problem/9142454>

Daniel Bates

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

Ryosuke Niwa

Comment 4 2011-03-16 22:34:44 PDT

http://trac.webkit.org/log/trunk/WebCore?action=stop_on_copy&mode=stop_on_copy&rev=64727&stop_rev=64637&limit=100 Suspicious changes: http://trac.webkit.org/changeset/64712 http://trac.webkit.org/changeset/64711 http://trac.webkit.org/changeset/64702 http://trac.webkit.org/changeset/64677

Ryosuke Niwa

Comment 5 2011-03-16 22:45:54 PDT

Adam & Eric, Can this be a HTML5 Tree Builder regression?

Eric Seidel (no email)

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

Eric Seidel (no email)

Comment 7 2011-03-16 22:48:14 PDT

I suspect we will be finding (long tail) fallout from our HTML5 parser re-write for years.

Adam Barth

Comment 8 2011-03-16 22:53:04 PDT

Differences between Firefox 4 are likely to be bugs in our code.

Eric Seidel (no email)

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

Andy Estes

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

Adam Barth

Comment 11 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>

Adam Barth

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

Naoki Takano

Comment 13 2011-03-28 19:41:24 PDT

Created attachment 87250 [details] Patch

Naoki Takano

Comment 14 2011-03-28 19:41:53 PDT

Comment on attachment 87250 [details] Patch Please review.

Kent Tamura

Comment 15 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?

Naoki Takano

Comment 16 2011-03-28 21:04:36 PDT

Created attachment 87255 [details] Patch

Naoki Takano

Comment 17 2011-03-28 21:05:17 PDT

Comment on attachment 87255 [details] Patch Please review.

Kent Tamura

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

Naoki Takano

Comment 19 2011-03-28 23:02:28 PDT

Created attachment 87263 [details] Patch

Naoki Takano

Comment 20 2011-03-28 23:02:46 PDT

Comment on attachment 87263 [details] Patch Review again.

Kent Tamura

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

Naoki Takano

Comment 22 2011-03-28 23:18:29 PDT

Created attachment 87266 [details] Patch

Kent Tamura

Comment 23 2011-03-28 23:19:34 PDT

Comment on attachment 87266 [details] Patch Great!

Naoki Takano

Comment 24 2011-03-28 23:19:44 PDT

Comment on attachment 87266 [details] Patch Sorry for bothering you again and again. Please review.

WebKit Commit Bot

Comment 25 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

Kent Tamura

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

Naoki Takano

Comment 27 2011-03-31 18:54:26 PDT

Created attachment 87815 [details] Patch

Naoki Takano

Comment 28 2011-03-31 18:55:42 PDT

Comment on attachment 87815 [details] Patch Fixed setDefaultValue() function. Please review again.

WebKit Commit Bot

Comment 29 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>

WebKit Commit Bot

Comment 30 2011-03-31 19:32:52 PDT

All reviewed patches have been landed. Closing bug.

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