81660 – line break inserted into input field

RESOLVED FIXED 81660

line break inserted into input field

https://bugs.webkit.org/show_bug.cgi?id=81660

Summary line break inserted into input field

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component UI Events

Assignee

jochen

Reported

2012-03-20 06:57 PDT

Modified

2012-03-31 14:18 PDT History

CC List

4 users Show

URL

Keywords

Depends on

Blocks