Bug 67697 - Change event is not fired for input[type=number] when the user reverts a change made by script
Summary: Change event is not fired for input[type=number] when the user reverts a chan...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 67360
  Show dependency treegraph
 
Reported: 2011-09-06 22:59 PDT by Ryosuke Niwa
Modified: 2011-09-07 03:06 PDT (History)
5 users (show)

See Also:


Attachments
fixes the bug (4.41 KB, patch)
2011-09-06 23:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed the test failure (16.62 KB, patch)
2011-09-07 02:00 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-09-06 22:59:49 PDT
Here's a test & fix for the following discussion on the bug 67360:

 Comment #2 From tkent@chromium.org (:tkent) (r) 2011-09-06 18:51:09 PST (-) [reply] 
(From update of attachment 106507 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=106507&action=review

> Source/WebCore/html/TextInputType.cpp:53
> +    // FIXME: should this be moved to TextFieldInputType?

I think so. It looks a bug of the current code.
 Comment #3 From rniwa@webkit.org (:rniwa) (r) 2011-09-06 18:58:57 PST (-) [reply] 
(From update of attachment 106507 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=106507&action=review

>> Source/WebCore/html/TextInputType.cpp:53
>> +    // FIXME: should this be moved to TextFieldInputType?
> 
> I think so. It looks a bug of the current code.

Okay. Let me see if I can come up with a test.
Comment 1 Ryosuke Niwa 2011-09-06 23:06:19 PDT
Created attachment 106546 [details]
fixes the bug
Comment 2 Kent Tamura 2011-09-06 23:17:05 PDT
Comment on attachment 106546 [details]
fixes the bug

Looks good.  Thank you!
Comment 3 WebKit Review Bot 2011-09-06 23:44:42 PDT
Comment on attachment 106546 [details]
fixes the bug

Attachment 106546 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9602458

New failing tests:
fast/forms/input-number-events.html
Comment 4 Kent Tamura 2011-09-07 00:56:02 PDT
(In reply to comment #3)
> New failing tests:
> fast/forms/input-number-events.html

The failure was:
@@ -15,7 +15,7 @@
 PASS changeEventCounter is 0
 PASS inputEventCounter is 1
 Focus on another field
-PASS changeEventCounter is 1
+FAIL changeEventCounter should be 1. Was 0.
 PASS inputEventCounter is 1
 PASS successfullyParsed is true

'change' was not fired when the number input lost focus after updating the value by the spin button.
HTMLInputElement::stepUpFromRenderer() uses setValueAsNumber() internally. So m_wasModifiedByUser and m_textAsOfLastFormControlChangeEvent are always reset.
Comment 5 Ryosuke Niwa 2011-09-07 02:00:20 PDT
Created attachment 106558 [details]
fixed the test failure
Comment 6 Ryosuke Niwa 2011-09-07 02:02:01 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > New failing tests:
> > fast/forms/input-number-events.html
> 
> The failure was:
> @@ -15,7 +15,7 @@
>  PASS changeEventCounter is 0
>  PASS inputEventCounter is 1
>  Focus on another field
> -PASS changeEventCounter is 1
> +FAIL changeEventCounter should be 1. Was 0.
>  PASS inputEventCounter is 1
>  PASS successfullyParsed is true
> 
> 'change' was not fired when the number input lost focus after updating the value by the spin button.
> HTMLInputElement::stepUpFromRenderer() uses setValueAsNumber() internally. So m_wasModifiedByUser and m_textAsOfLastFormControlChangeEvent are always reset.

Yeah, I completely overlooked this failure. New patch fixes this issue by propagating sendChangeEvent through various functions from stepUpFromRenderer.
Comment 7 Kent Tamura 2011-09-07 02:04:40 PDT
Comment on attachment 106558 [details]
fixed the test failure

Looks good.
Comment 8 Ryosuke Niwa 2011-09-07 02:11:10 PDT
(In reply to comment #7)
> (From update of attachment 106558 [details])
> Looks good.

Thanks for the review :D
Comment 9 WebKit Review Bot 2011-09-07 03:06:18 PDT
Comment on attachment 106558 [details]
fixed the test failure

Clearing flags on attachment: 106558

Committed r94658: <http://trac.webkit.org/changeset/94658>
Comment 10 WebKit Review Bot 2011-09-07 03:06:23 PDT
All reviewed patches have been landed.  Closing bug.