Bug 20461 – Collapse multiline values in inputs

• Component: Forms
• Status: RESOLVED FIXED

[Eric Roman]

If you paste a multiline address into maps.google.com, only the first line appears in the input.

IE/Firefox/Opera all collapse the multiline value to avoid this problem.
-IE and Opera strip the newline.
-Firefox replaces it with a space.

Webkit should similarly strip newlines so you can paste into inputs easily.

[Eric Roman]

Created attachment 22898 [details]
Either paste into the field, or click the button to repro multi-line issue.

[Glenn Wilson]

This is an interesting issue because all browsers seem to handle copying and pasting newlines, carriage returns, and non-displayable characters (like \0) differently.

Tested on WinXPSP2,
IE6/IE7 ==> cuts newlines and carriage returns out of the string, truncates everything after a null character
FF3 ==> replaces CRLF with one space, shows null character as a unicode "box with numbers"
Safari 3.1==> truncates after the first ASCII char 'below' a space (excluding '\t')

So when copying in multiple lines into a text field, should each CRLF be replaced with zero spaces like IE, one space like FF, or two spaces (one for each character)?  And should it accept the null character (\0) and other unexpected characters?

I'm leaning towards following FF's lead here...my thoughts:

1.  If a CR, LF, or CRLF were once between characters in a string, that string was intended to have some kind of whitespace there.  Otherwise, for example, "Our record is 10-5\n8 players scored goals" would become "Our record is 10-58 players scored goals".  So at least some whitespace should be retained to maintain some intention of the original text.  However, two spaces for a CRLF seems inconsistent with the way CRLF is displayed in most cases -- as one space.

2.  Retaining the null character is dependent on whether it can be rendered, and whether it makes sense.  Firefox shows a little box with "0000" in it, which looks to be a representation of the unicode character, U+0000.  So clearly, FF has a way of showing the character and the user can understand it's a funky char, and can decide what to do with it.  In Safari 3.1, when forced to show the null character, it doesn't display at all.  So truncating on it doesn't seem necessary.

[Glenn Wilson]

On second thought, letting non-visible characters live in input fields could cause a lot of potential problems for users.  I can imagine a situation wherein a user is tortured by trying to log in somewhere with a null character in their username/password.  Not to mention what kind of security concerns this might raise.

It's probably better to handle issue #1, and just collapse CR/LF into spaces when pasting into a text-like input, and handle the non-rendering of system characters as part of a separate bug.



 

[Glenn Wilson]

This bug looks to be associated with the fix to bug #8104.

The original problem seemed to be that input text fields' heights would change when newlines were pasted in.  So any input getting into a text field would just be truncated on a newline.

But this doesn't seem right: IE7, FF3 take multi-line inputs and "flatten" them to be put into single-line text inputs.

[Glenn Wilson]

Created attachment 23306 [details]
Possible fix to bug 20461

Here is a potential fix to this problem.

This bug is the result of an earlier issue wherein pasting text with multi-line inputs would result in text areas mis-sizing.  The solution was to simply truncate all input after the first non-tab system character had been found.

I believe that this behavior is not entirely correct.  Other browsers do not truncate after the first system character, but simply replace newlines with spaces.  This seems to be helpful to the user: for example, copy a two-line postal address and paste into Google Maps' one-line text field.  This change modified how the input filter replaced text: it now replaces newlines and carriage returns (and the combination, \r\n) with a single space.

However, not all behavior has been changed.  The input still truncates on non-tab, non-carriage return, non-newline system characters.

As a result, a layout test has been added and the old layout test has been removed.

[Maciej Stachowiak]

Cc'd Adele and Justin who might better understand the context of the original change.

[Darin Adler]

Comment on attachment 23306 [details]
Possible fix to bug 20461

Truncating strings at the first newline was an intentional design decision in the original Safari development cycle. The original check-in for this was:

    http://trac.webkit.org/changeset/7408

> IE/Firefox/Opera all collapse the multiline value to avoid this problem.
> -IE and Opera strip the newline.
> -Firefox replaces it with a space.

At the change of change 7408 that was not true of IE. Presumably IE changed their behavior at some point. We decided to match IE rather than Gecko back then.

+        string.replace(static_cast<WebCore::String>("\r\n"), static_cast<WebCore::String>(" "));

You should be able to just write:

    string.replace("\r\n", " ");

Those typecasts are unnecessary, and in fact they prevent us from later overloading String::replace to create a more efficient version that doesn't create intermediate String objects.

The patch looks fine otherwise. I'd prefer that someone remove the redundant static_cast calls.

r=me

[Darin Adler]

(In reply to comment #5)
> However, not all behavior has been changed. The input still truncates on
> non-tab, non-carriage return, non-newline system characters.

We need a test for this part of the behavior then! I missed that in my review.

[Glenn Wilson]

I think with the static casts, I was conforming to what my IDE was telling me...intellisense has led me astray.  Those casts can be removed if they are problematic.

The unit test included does test that inputs are truncated at the first non- tab/newline/CR as well as testing that multiline inputs are collapsed.  Admittedly, it only tests that the truncation happens with with a null character ('\0'), and in theory it *should* test every system character.

[Darin Adler]

Comment on attachment 23306 [details]
Possible fix to bug 20461

This patch breaks the test output from test fast/forms/8250.html, which fails now. That test needs to be updated.

[Darin Adler]

I took care of it. There were also tabs in the test file that I had to remove.

http://trac.webkit.org/changeset/37539