Bug 9358 - REGRESSION: Assertion failure in HTMLInputElement::setValueFromRenderer (value == constrainValue(value)) when deleting all text
Summary: REGRESSION: Assertion failure in HTMLInputElement::setValueFromRenderer (valu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: data:text/html,<input value="X">
Keywords: Regression
: 9429 9542 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-06-08 07:35 PDT by mitz
Modified: 2006-06-30 12:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (no changelog; no test) (690 bytes, patch)
2006-06-29 08:18 PDT, David Kilzer (:ddkilzer)
darin: review-
Details | Formatted Diff | Diff
Patch v2 (5.20 KB, patch)
2006-06-30 08:42 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-06-08 07:35:36 PDT
I hit this assert when I try to delete the X in the field using the delete key:

ASSERTION FAILED: value == constrainValue(value) (WebCore/html/HTMLInputElement.cpp:1068 void WebCore::HTMLInputElement::setValueFromRenderer(const WebCore::String&))
Comment 1 mitz 2006-06-08 07:55:08 PDT
The root cause seems to be that now when you delete the last character, a <br> placeholder is inserted.
Comment 2 Justin Garcia 2006-06-08 11:40:47 PDT
Look like textContent returns "\n" for a br even if its collapsed.  value = "\n" and constrainValue(value) = "".
Comment 3 David Kilzer (:ddkilzer) 2006-06-10 08:20:53 PDT
Grrr!  I Just ran into this on Bugzilla's simple search page, and it killed my browser!
Comment 4 David Kilzer (:ddkilzer) 2006-06-13 15:47:01 PDT
This is also a regression (so to speak).
Comment 5 David Kilzer (:ddkilzer) 2006-06-13 15:47:22 PDT
*** Bug 9429 has been marked as a duplicate of this bug. ***
Comment 6 mitz 2006-06-22 02:38:22 PDT
*** Bug 9542 has been marked as a duplicate of this bug. ***
Comment 7 David Kilzer (:ddkilzer) 2006-06-29 08:18:30 PDT
Created attachment 9089 [details]
Patch v1 (no changelog; no test)

I crash from this assertion daily on my debug build.  Tell me what's wrong with this patch so it can be fixed properly.  :)
Comment 8 Darin Adler 2006-06-29 08:30:09 PDT
Comment on attachment 9089 [details]
Patch v1 (no changelog; no test)

It's driving me crazy too!

This is an acceptable workaround for the problem, but not the right way to fix the problem.

I don't want an extra call to constrainValue here in the production code in the long term.

I'd prefer isEmpty() to length() == 0, and I'd like a comment here that explains exactly why we need a special case.

Perhaps a better way to do it would be to add code like this before the assert:

    // Workaround for bug where trailing \n is included in the result of textContent.
    // <URL of a new bug report> 
    if (value == "\n")
        value = "";
Comment 9 David Kilzer (:ddkilzer) 2006-06-29 23:50:52 PDT
(In reply to comment #8)
>     // Workaround for bug where trailing \n is included in the result of
> textContent.
>     // <URL of a new bug report>

Bug 9661 filed for the follow-up issue.

Comment 10 David Kilzer (:ddkilzer) 2006-06-30 08:42:26 PDT
Created attachment 9106 [details]
Patch v2

Patch v2 includes ChangeLogs, a layout test, and addresses Darin's comments.
Comment 11 David Kilzer (:ddkilzer) 2006-06-30 08:43:13 PDT
(In reply to comment #10)
> Patch v2 includes ChangeLogs, a layout test, and addresses Darin's comments.

It also passed all layout tests.

Comment 12 Darin Adler 2006-06-30 11:12:16 PDT
Comment on attachment 9106 [details]
Patch v2

OK. r=me
Comment 13 David Kilzer (:ddkilzer) 2006-06-30 12:04:31 PDT
Committed revision 15111!