63092 – input/textarea onchange doesn't fire if value is set in key listener

RESOLVED FIXED Bug 63092

input/textarea onchange doesn't fire if value is set in key listener

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

Summary input/textarea onchange doesn't fire if value is set in key listener

Emil A Eklund

Reported 2011-06-21 13:54:36 PDT

Reported downstream: http://code.google.com/p/chromium/issues/detail?id=86302 Reported by project member glider, Jun 16 (5 days ago) Chrome Version : 12.0.742.91 and 12.0.742.105 for Windows URLs (if applicable) : http://bpilot.xmlcan.ca/test/test_onkeyup.html Other browsers tested: Firefox 4.0.1 on Mac: OK Chrome 12.0.742.100 beta on Linux -- OK Chrome 11.0.696.71 on Mac -- OK What steps will reproduce the problem? 1. Go to http://bpilot.xmlcan.ca/test/test_onkeyup.html or open the following file locally: ============================================ <html> ....<input id='blabla' value='blabla' size='10' onkeyup='this.value=this.value.replace("d","")' onchange='alert(this.value)'/> </html> ============================================ 2. Type something into the input box 3. Click somewhere outside the input box What is the expected result? An alert with the input box contents shall fire What happens instead? Nothing

Attachments
Patch (6.17 KB, patch) 2011-06-21 14:29 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cq-01 (1.30 MB, application/zip) 2011-06-21 15:34 PDT, WebKit Review Bot	no flags	Details
Archive of layout-test-results from ec2-cr-linux-03 (1.22 MB, application/zip) 2011-06-21 15:48 PDT, WebKit Review Bot	no flags	Details
Patch (6.42 KB, patch) 2011-06-21 17:37 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch for landing (7.38 KB, patch) 2011-06-22 13:45 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (8.00 KB, patch) 2011-06-22 14:14 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Emil A Eklund

Comment 1 2011-06-21 14:29:52 PDT

Created attachment 98057 [details] Patch

WebKit Review Bot

Comment 2 2011-06-21 15:33:58 PDT

Comment on attachment 98057 [details] Patch Rejecting attachment 98057 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: ents-text.svg = TEXT Failed to run "['xvfb-run', 'Tools/Scripts/new-run-webkit-tests', '--chromium', '--no-pixel-tests', '--results-d..." exit_code: 2 sociated-element-crash.html = TEXT PASS fast/forms/input-number-events.html = TEXT PASS fast/forms/input-number-large-padding.html = TEXT PASS fast/forms/input-spinbutton-capturing.html = TEXT PASS Regressions: Unexpected text diff mismatch : (2) fast/forms/range-set-attribute.html = TEXT svg/custom/pointer-events-text.svg = TEXT Full output: http://queues.webkit.org/results/8925050

WebKit Review Bot

Comment 3 2011-06-21 15:34:03 PDT

Created attachment 98066 [details] Archive of layout-test-results from ec2-cq-01 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

WebKit Review Bot

Comment 4 2011-06-21 15:48:02 PDT

Comment on attachment 98057 [details] Patch Attachment 98057 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8923080 New failing tests: fast/forms/range-set-attribute.html

WebKit Review Bot

Comment 5 2011-06-21 15:48:10 PDT

Created attachment 98071 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Emil A Eklund

Comment 6 2011-06-21 17:37:54 PDT

Created attachment 98086 [details] Patch

Emil A Eklund

Comment 7 2011-06-21 17:38:14 PDT

PTAL

Darin Adler

Comment 8 2011-06-21 17:44:08 PDT

Comment on attachment 98086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98086&action=review > Source/WebCore/ChangeLog:15 > + * html/HTMLInputElement.cpp: > + (WebCore::HTMLInputElement::setValue): > + * html/HTMLTextAreaElement.cpp: > + (WebCore::HTMLTextAreaElement::setValue): > + (WebCore::HTMLTextAreaElement::setNonDirtyValue): > + (WebCore::HTMLTextAreaElement::setValueCommon): Could you add something to the change log that explains why these changes are correct? > Source/WebCore/html/HTMLInputElement.cpp:982 > + String sanitizedValue = this->sanitizeValue(value); Why the this-> on this line?

Emil A Eklund

Comment 9 2011-06-21 17:45:52 PDT

> Could you add something to the change log that explains why these changes are correct? Will do. > > > Source/WebCore/html/HTMLInputElement.cpp:982 > > + String sanitizedValue = this->sanitizeValue(value); > > Why the this-> on this line? To avoid confusion as sanitizeValue looks a lot like sanitizedValue. Perhaps I should try to pick a better name for the local variable.

Alexey Proskuryakov

Comment 10 2011-06-21 21:30:27 PDT

The regression test has a disconnect from bug title. Does this only change the behavior for keyup? What if the value is set from keydown or keypress, and then default handling is prevented?

Emil A Eklund

Comment 11 2011-06-22 10:40:54 PDT

keypress fires before the value is updated and can't be prevented so its behavior is unaffected by this change. I've added tests for keydown though. Thanks Alexey!

Emil A Eklund

Comment 12 2011-06-22 13:45:21 PDT

Created attachment 98233 [details] Patch for landing

Alexey Proskuryakov

Comment 13 2011-06-22 14:03:30 PDT

(In reply to comment #11) > keypress fires before the value is updated and can't be prevented so its behavior is unaffected by this change. I don't follow what you are saying. Keydown is also fired before the value is updated, so what's the difference? And no, preventing default handling of keypress does work.

Emil A Eklund

Comment 14 2011-06-22 14:14:05 PDT

Created attachment 98240 [details] Patch You are right, keypress can be canceled. Updated the test case accordingly.

Emil A Eklund

Comment 15 2011-06-23 14:04:41 PDT

Alexey, is there anything else you'd like me to test or are you happy with the test for this patch now that I test keydown, keypress and keyup? Thanks.

Alexey Proskuryakov

Comment 16 2011-06-23 14:52:13 PDT

I don't have any other comment, thank you for adding the tests. Since the patch already has reviewers filled in, you can remove r? flag, and set cq? or cq+.

WebKit Review Bot

Comment 17 2011-06-23 15:13:43 PDT

Comment on attachment 98240 [details] Patch Clearing flags on attachment: 98240 Committed r89624: <http://trac.webkit.org/changeset/89624>

WebKit Review Bot

Comment 18 2011-06-23 15:13:52 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 Forms

Assignee

Emil A Eklund

Reported

2011-06-21 13:54 PDT

Modified

2011-06-23 15:13 PDT History

CC List

3 users Show

URL

Keywords

Depends on

Blocks