Bug 63092 - input/textarea onchange doesn't fire if value is set in key listener
Summary: input/textarea onchange doesn't fire if value is set in key listener
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-21 13:54 PDT by Emil A Eklund
Modified: 2011-06-23 15:13 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 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
Comment 1 Emil A Eklund 2011-06-21 14:29:52 PDT
Created attachment 98057 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 WebKit Review Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 WebKit Review Bot 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
Comment 6 Emil A Eklund 2011-06-21 17:37:54 PDT
Created attachment 98086 [details]
Patch
Comment 7 Emil A Eklund 2011-06-21 17:38:14 PDT
PTAL
Comment 8 Darin Adler 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?
Comment 9 Emil A Eklund 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.
Comment 10 Alexey Proskuryakov 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?
Comment 11 Emil A Eklund 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!
Comment 12 Emil A Eklund 2011-06-22 13:45:21 PDT
Created attachment 98233 [details]
Patch for landing
Comment 13 Alexey Proskuryakov 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.
Comment 14 Emil A Eklund 2011-06-22 14:14:05 PDT
Created attachment 98240 [details]
Patch

You are right, keypress can be canceled. Updated the test case accordingly.
Comment 15 Emil A Eklund 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.
Comment 16 Alexey Proskuryakov 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+.
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2011-06-23 15:13:52 PDT
All reviewed patches have been landed.  Closing bug.