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

Issue 513783002: Dispatch change event for input type=color when colorpicker is closed (Closed)

Created:
6 years, 3 months ago by Habib Virji
Modified:
3 years, 5 months ago
Reviewers:
keishi, tkent
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Dispatch change event for input type=color when colorpicker is closed Change event should be trigger when colorpicker is closed. In current it dispatches for each value change in colorpicker. In current code, JS function selectColorInColorChooser(which in turn call didChooseColor) set the color and expects dispatch change event. SetValue also calls didChooseColor. Since both JS function and setValue are calling didChooseColor, it ends up firing change event for each value change. This CL dispatches change event from endChooserColor to solve change event not dispatched when value is set by the user. Since mac, uses non-modal window, it still dispatches change event. To test the functionality endColorChooser has been added to test value added, only on close of color chooser change event is dispatched. BUG=405059 R=tkent, keishi TEST=Updated tests to check changing color does not dispatch change event, only on closing of window it does. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182433

Patch Set 1 #

Total comments: 4

Patch Set 2 : Added endColorChooser in internals for testing purpose, added isModalColorChooser to differentiate … #

Patch Set 3 : Mac test fix #

Total comments: 11

Patch Set 4 : Added didEndChooser in InputType to avoid static cast #

Total comments: 8

Patch Set 5 : Added new expected file for mac #

Patch Set 6 : Implemented colorChooserClient support #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -9 lines) Patch
M LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value-expected.txt View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/color/input-color-onchange-event.html View 1 2 3 2 chunks +18 lines, -2 lines 0 comments Download
M LayoutTests/fast/forms/color/input-color-onchange-event-expected.txt View 1 2 3 1 chunk +7 lines, -1 line 0 comments Download
A + LayoutTests/platform/mac/fast/forms/color/input-color-choose-default-value-after-set-value-expected.txt View 1 2 3 4 0 chunks +-1 lines, --1 lines 0 comments Download
A + LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
M Source/core/html/HTMLInputElement.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLInputElement.cpp View 1 2 3 4 5 1 chunk +8 lines, -4 lines 0 comments Download
M Source/core/html/forms/ColorInputType.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/forms/ColorInputType.cpp View 1 2 3 4 5 3 chunks +10 lines, -1 line 0 comments Download
M Source/core/html/forms/InputType.h View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/html/forms/InputType.cpp View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderTheme.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderThemeChromiumMac.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/testing/Internals.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/testing/Internals.idl View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 26 (6 generated)
Habib Virji
Please take a look. No tests added as change event tests are passing. All color ...
6 years, 3 months ago (2014-08-27 14:11:55 UTC) #1
Habib Virji
On 2014/08/27 14:11:55, Habib Virji wrote: > Please take a look. > > No tests ...
6 years, 3 months ago (2014-09-08 08:33:01 UTC) #2
keishi
Sorry for being late to review. https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputElement.cpp File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputElement.cpp#newcode1495 Source/core/html/HTMLInputElement.cpp:1495: static_cast<ColorInputType*>(m_inputType.get())->didEndChooser(); You could ...
6 years, 3 months ago (2014-09-09 11:55:51 UTC) #3
Habib Virji
On 2014/09/09 11:55:51, keishi wrote: > Sorry for being late to review. > > https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputElement.cpp ...
6 years, 3 months ago (2014-09-09 12:09:35 UTC) #4
Habib Virji
Thanks Keishi, sorry for delay in submission. Code updated as suggested, new method in internals ...
6 years, 3 months ago (2014-09-10 16:30:55 UTC) #5
Habib Virji
On 2014/09/10 16:30:55, Habib Virji wrote: > Thanks Keishi, sorry for delay in submission. Code ...
6 years, 3 months ago (2014-09-17 05:52:31 UTC) #9
keishi
https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html File LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html (right): https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html#newcode24 LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html:24: internals.endColorChooser(input); I don't think we need to add this. ...
6 years, 3 months ago (2014-09-17 08:15:54 UTC) #10
Habib Virji
Thanks keishi, will update HTMLInputElement. Can you only confirm about tests as change event otherwise ...
6 years, 3 months ago (2014-09-17 08:30:42 UTC) #11
keishi
https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html File LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html (right): https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html#newcode24 LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html:24: internals.endColorChooser(input); On 2014/09/17 08:30:42, Habib Virji wrote: > On ...
6 years, 3 months ago (2014-09-18 03:50:56 UTC) #12
Habib Virji
Thanks updated as suggested. Please have a look. https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html File LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html (right): https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html#newcode24 LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html:24: internals.endColorChooser(input); ...
6 years, 3 months ago (2014-09-18 12:42:05 UTC) #13
keishi
lgtm https://codereview.chromium.org/513783002/diff/140001/LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt File LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt (right): https://codereview.chromium.org/513783002/diff/140001/LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt#newcode15 LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt:15: FAIL onChange should be 0. Was 1.` nit: ...
6 years, 3 months ago (2014-09-18 13:16:01 UTC) #15
Habib Virji
@keishi: one last question regarding change in HTMLINputType. It appears your suggestion does not work. ...
6 years, 3 months ago (2014-09-18 14:54:26 UTC) #16
keishi
https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLInputElement.cpp File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLInputElement.cpp#newcode1487 Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser(); On 2014/09/18 14:54:26, Habib Virji wrote: > This ...
6 years, 3 months ago (2014-09-19 03:04:12 UTC) #18
Habib Virji
Sorry keishi, for keeping on popping question back at you. Cannot understand actually suggested solution. ...
6 years, 3 months ago (2014-09-19 09:15:56 UTC) #19
keishi
On 2014/09/19 09:15:56, Habib Virji wrote: > Sorry keishi, for keeping on popping question back ...
6 years, 3 months ago (2014-09-22 09:58:36 UTC) #20
Habib Virji
Thanks Keishi, implemented as suggested. Can you please have a look, if all changes are ...
6 years, 3 months ago (2014-09-22 13:44:40 UTC) #21
keishi
Thanks. LGTM
6 years, 3 months ago (2014-09-22 14:25:05 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/513783002/200001
6 years, 3 months ago (2014-09-22 15:00:43 UTC) #24
commit-bot: I haz the power
Committed patchset #6 (id:200001) as 182433
6 years, 3 months ago (2014-09-22 16:13:56 UTC) #25
javan
3 years, 5 months ago (2017-06-29 12:54:38 UTC) #26
Message was sent while issue was closed.
On 2014/09/22 16:13:56, commit-bot: I haz the power wrote:
> Committed patchset #6 (id:200001) as 182433

FYI, I'm still seeing this issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=405059#c11

Powered by Google App Engine
This is Rietveld 408576698