Bug 8104 - REGRESSION (NativeTextField): New text fields should not allow pasting newlines
Summary: REGRESSION (NativeTextField): New text fields should not allow pasting newlines
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Adele Peterson
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2006-03-31 09:33 PST by mitz
Modified: 2006-04-04 01:55 PDT (History)
1 user (show)

See Also:


Attachments
initial patch (2.75 KB, patch)
2006-03-31 17:10 PST, Adele Peterson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-03-31 09:33:45 PST
New text fields allow you to paste text containing newlines, and then their height changes to accommodate the multi-line text.

Old behavior was to paste the text only up to the first newline.
Comment 1 Adele Peterson 2006-03-31 13:00:10 PST
*** Bug 8107 has been marked as a duplicate of this bug. ***
Comment 2 Adele Peterson 2006-03-31 14:11:02 PST
ok- i'm working on this now.
Comment 3 Adele Peterson 2006-03-31 17:10:28 PST
Created attachment 7429 [details]
initial patch

posting an initial patch...

I ran into a problem while testing a simpler version of this patch.  If you select all at cnn.com, and then go to paste that content into the text field, we ended up pasting in a newline at the end.  I realized that if we did a test rendering of the plain-text conversion first, and then sent the BeforeTextInserted event (which causes the truncation).

I don't like doing another test rendering in ReplaceSelectionCommand though.  So I'm trying to find a better solution.  Feel free to jump in w/ ideas :)
Comment 4 Darin Adler 2006-04-01 08:50:11 PST
(In reply to comment #3)
> I don't like doing another test rendering in ReplaceSelectionCommand though. 
> So I'm trying to find a better solution.  Feel free to jump in w/ ideas :)

Well, this version seems fine to me; we shouldn't worry unduly about the extra text rendering for now. But if you are doing this, you should move more lines inside the if statement. Specifically, these three don't need to be done twice if you aren't in the plain text case:

+    range->selectNodeContents(holder.get(), ec);
+    ASSERT(ec == 0);
+    text = plainText(range.get());

The use of min/max here is a bit confusing because of the special case for -1. You should probably just write it out like this:

        if (newLinePosition >= 0 && (truncateLength < 0 || truncateLength > newLinePosition))
            truncateLength = newLinePosition;
 
I think that's a little clearer. Another possibility would be to write a local function that is a version of min that handles the -1 value as a special case.

    static int minPosition(int p1, int p2)
    {
        if (p1 < 0)
            return p2;
        if (p2 < 0)
            return p1;
        return min(p1, p2);
    }

        truncateLength = minPosition(truncateLength, newLinePosition);
Comment 5 Darin Adler 2006-04-01 08:53:12 PST
(In reply to comment #4)
> I think that's a little clearer. Another possibility would be to write a local
> function that is a version of min that handles the -1 value as a special case.

Or you could initialize truncateLength to text.length() to simplify the code a bit. You're stuck with the "-1" for the find result, but not for your own code. You could even cover find with something that returns the string length instead of -1 when not finding anything.
Comment 6 Maciej Stachowiak 2006-04-02 23:25:42 PDT
These are all text field regressions so they should all be P1.
Comment 7 Adele Peterson 2006-04-03 14:11:13 PDT
With further testing, and talking w/ Justin, I don't think we actually need to change how ReplaceSelectionCommand works for this fix.

Justin reviewed a simpler patch that just does the truncation in HTMLInputElement.