Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(429)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 842153002: Click event should be fired before change events for checkbox and radio button. (Closed)

Created:
5 years, 11 months ago by Paritosh Kumar
Modified:
5 years, 8 months ago
Reviewers:
keishi, tkent, philipj_slow
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, Savago
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Click event should be fired before change events for checkbox and radio button. This cl fixes this issue. Bug=178505 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193320

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 2

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -3 lines) Patch
A LayoutTests/fast/forms/radio-checkbox-events-order.html View 1 2 1 chunk +80 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/radio-checkbox-events-order-expected.txt View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
M LayoutTests/fast/forms/radio/radio-group-keyboard-change-event-expected.txt View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/html/HTMLInputElement.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLInputElement.cpp View 1 2 3 2 chunks +7 lines, -1 line 0 comments Download
M Source/core/html/forms/CheckboxInputType.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/forms/RadioInputType.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (6 generated)
Paritosh Kumar
@keishi: Please have a look.
5 years, 11 months ago (2015-01-09 10:02:20 UTC) #2
Paritosh Kumar
Please have a look.
5 years, 8 months ago (2015-04-01 10:42:01 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/842153002/1
5 years, 8 months ago (2015-04-01 23:59:07 UTC) #6
tkent
Did you investigate why CheckboxInputType and RadioInputType called setChecked() in willDispatchClick()?
5 years, 8 months ago (2015-04-02 00:09:38 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/30311)
5 years, 8 months ago (2015-04-02 00:20:23 UTC) #9
Paritosh Kumar
On 2015/04/02 00:09:38, tkent wrote: > Did you investigate why CheckboxInputType and RadioInputType called setChecked() ...
5 years, 8 months ago (2015-04-02 02:22:52 UTC) #10
tkent
So, the order should be: - mousedown event - (checkbox/radio value is not updated yet) ...
5 years, 8 months ago (2015-04-03 03:38:10 UTC) #11
Paritosh Kumar
On 2015/04/03 03:38:10, tkent wrote: > So, the order should be: > > - mousedown ...
5 years, 8 months ago (2015-04-03 04:16:32 UTC) #12
tkent
Thank you for the investigation. I suspected IE updated the value on mousedown, but it ...
5 years, 8 months ago (2015-04-03 04:23:03 UTC) #13
Paritosh Kumar
On 2015/04/03 04:23:03, tkent wrote: > Thank you for the investigation. > I suspected IE ...
5 years, 8 months ago (2015-04-03 05:06:49 UTC) #14
tkent
Thank you for the information. So, I think your code change does right thing. Do ...
5 years, 8 months ago (2015-04-07 00:47:40 UTC) #15
Paritosh Kumar
Thanks tkent > Do we have a test of the event order when preventDefault() is ...
5 years, 8 months ago (2015-04-07 05:06:57 UTC) #16
tkent
> > Do we have a test of the event order when a checkbox value ...
5 years, 8 months ago (2015-04-07 05:10:26 UTC) #17
Paritosh Kumar
On 2015/04/07 05:10:26, tkent wrote: > > > Do we have a test of the ...
5 years, 8 months ago (2015-04-07 06:10:09 UTC) #18
tkent
lgtm https://codereview.chromium.org/842153002/diff/40001/Source/core/html/HTMLInputElement.cpp File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/842153002/diff/40001/Source/core/html/HTMLInputElement.cpp#newcode900 Source/core/html/HTMLInputElement.cpp:900: void HTMLInputElement::dispatchChangeEventIfNeeded(TextFieldEventBehavior eventBehavior) The argument is always DispatchChangeEvent. ...
5 years, 8 months ago (2015-04-07 06:32:28 UTC) #19
Paritosh Kumar
Thanks tkent Removed unnecessary argument, from dispatchChangeEventIfNeeded. https://codereview.chromium.org/842153002/diff/40001/Source/core/html/HTMLInputElement.cpp File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/842153002/diff/40001/Source/core/html/HTMLInputElement.cpp#newcode900 Source/core/html/HTMLInputElement.cpp:900: void HTMLInputElement::dispatchChangeEventIfNeeded(TextFieldEventBehavior ...
5 years, 8 months ago (2015-04-07 08:53:58 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/842153002/60001
5 years, 8 months ago (2015-04-08 01:11:40 UTC) #23
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193320
5 years, 8 months ago (2015-04-08 02:52:32 UTC) #24
tkent
5 years, 6 months ago (2015-06-15 01:54:58 UTC) #25
Message was sent while issue was closed. 
A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1189533002/ by tkent@chromium.org.

The reason for reverting is: Caused a regression, crbug.com/500084..

Powered by Google App Engine
This is Rietveld 408576698