Bug 81660

Summary:

line break inserted into input field

Product:

WebKit

Reporter:

jochen

Component:

UI Events

Assignee:

jochen

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, rniwa, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
layout test	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

jochen

Reported 2012-03-20 06:57:38 PDT

Created attachment 132818 [details] layout test The attached layout test will insert a linebreak into the text field, even though it should not. I tested this with Safari and Chrome, both insert a line break, Firefox and IE9 don't insert a linebreak here

Attachments
layout test (768 bytes, patch) 2012-03-20 06:57 PDT, jochen	no flags	Details Formatted Diff Diff
Patch (34.90 KB, patch) 2012-03-30 02:28 PDT, jochen	no flags	Details Formatted Diff Diff
Patch (4.28 KB, patch) 2012-03-30 02:55 PDT, jochen	no flags	Details Formatted Diff Diff
Patch (3.76 KB, patch) 2012-03-30 03:13 PDT, jochen	no flags	Details Formatted Diff Diff
Patch (3.87 KB, patch) 2012-03-31 12:14 PDT, jochen	no flags	Details Formatted Diff Diff
Patch (4.00 KB, patch) 2012-03-31 12:40 PDT, jochen	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

jochen

Comment 1 2012-03-30 02:28:46 PDT

Created attachment 134753 [details] Patch

Kent Tamura

Comment 2 2012-03-30 02:41:18 PDT

Comment on attachment 134753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134753&action=review > Source/WebCore/html/TextFieldInputType.cpp:375 > + // Do not insert single newline characters, as this will result in a linebreak being inserted. > + if (event->text() == "\n") { > + event->setText(String()); > + return; > + } Wrong indentation. Why this change fixes the bug? We have eventText.replace('\n', ' '); below. > LayoutTests/ChangeLog:12 > + * fast/forms/textfield-no-linebreak.html: Added. > + * platform/chromium-mac/fast/forms/textfield-no-linebreak-expected.png: Added. > + * platform/chromium-mac/fast/forms/textfield-no-linebreak-expected.txt: Added. > + * platform/mac/fast/forms/textfield-no-linebreak-expected.png: Added. > + * platform/mac/fast/forms/textfield-no-linebreak-expected.txt: Added. I think this test can be a ref test.

jochen

Comment 3 2012-03-30 02:45:04 PDT

(In reply to comment #2) > Why this change fixes the bug? > We have eventText.replace('\n', ' '); below. For linebreaks, the event text is ignored, it's just checked whether the event still has any text: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp#L51

Kent Tamura

Comment 4 2012-03-30 02:54:01 PDT

(In reply to comment #3) > (In reply to comment #2) > > Why this change fixes the bug? > > We have eventText.replace('\n', ' '); below. > > For linebreaks, the event text is ignored, it's just checked whether the event still has any text: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp#L51 Thanks. I understand. I think changing TextFieldInputType::handleBeforeTextInsertedEvent is not reasonable. It makes the code confusing. We should fix canAppendNewLineFeed() in TypingCommand.cpp like: return event->text() == "\n";

jochen

Comment 5 2012-03-30 02:55:07 PDT

Created attachment 134759 [details] Patch

jochen

Comment 6 2012-03-30 03:13:58 PDT

Created attachment 134764 [details] Patch

jochen

Comment 7 2012-03-30 03:14:23 PDT

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Why this change fixes the bug? > > > We have eventText.replace('\n', ' '); below. > > > > For linebreaks, the event text is ignored, it's just checked whether the event still has any text: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp#L51 > > Thanks. I understand. > I think changing TextFieldInputType::handleBeforeTextInsertedEvent is not reasonable. It makes the code confusing. We should fix canAppendNewLineFeed() in TypingCommand.cpp like: > return event->text() == "\n"; ok, I updated the patch (and made the test a reftest)

Ryosuke Niwa

Comment 8 2012-03-30 11:05:10 PDT

Comment on attachment 134764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134764&action=review > Source/WebCore/editing/TypingCommand.cpp:60 > RefPtr<BeforeTextInsertedEvent> event = BeforeTextInsertedEvent::create(String("\n")); > ExceptionCode ec = 0; > node->dispatchEvent(event, ec); > - return event->text().length(); > + return event->text() == "\n"; Please allocate "\n" once (e.g. local variable lineFeed) and share it. > LayoutTests/fast/forms/textfield-no-linebreak-expected.html:6 > +<input id="tf" type="text"></input> Why don't you just add autofocus?

jochen

Comment 9 2012-03-31 12:14:18 PDT

Created attachment 134971 [details] Patch

Ryosuke Niwa

Comment 10 2012-03-31 12:31:11 PDT

Comment on attachment 134971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134971&action=review > LayoutTests/fast/forms/textfield-no-linebreak-expected.html:1 > +<html> No DOCTYPE? > LayoutTests/fast/forms/textfield-no-linebreak.html:1 > +<html> Ditto.

jochen

Comment 11 2012-03-31 12:40:17 PDT

Created attachment 134974 [details] Patch

WebKit Review Bot

Comment 12 2012-03-31 14:18:08 PDT

Comment on attachment 134974 [details] Patch Clearing flags on attachment: 134974 Committed r112803: <http://trac.webkit.org/changeset/112803>

WebKit Review Bot

Comment 13 2012-03-31 14:18:13 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.