Dispatch change event for input type=color when colorpicker is closed

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

[Habib Virji, 2014-08-27 14:11:55]

Please take a look.

No tests added as change event tests are passing. All color tests rely on
selectColorInColorChooser JS call, but this CL requires value set by the user.

[Habib Virji, 2014-09-08 08:33:01]

On 2014/08/27 14:11:55, Habib Virji wrote:
> Please take a look.
> 
> No tests added as change event tests are passing. All color tests rely on
> selectColorInColorChooser JS call, but this CL requires value set by the user.

@tkent: can you please have a look.

[keishi, 2014-09-09 11:55:51]

Sorry for being late to review.

https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:1495:
static_cast<ColorInputType*>(m_inputType.get())->didEndChooser();
You could add a separate method like Internals::endColorChooser and use it to
write a test for this CL.

https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
File Source/core/html/forms/ColorInputType.cpp (left):

https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
Source/core/html/forms/ColorInputType.cpp:191:
element().dispatchFormControlChangeEvent();
On Mac the color picker is a non-modal floating dialog so closing the closing
the color picker should not be required to set the value. We want to dispatch a
change event just on Mac.

I think we should add a method like RenderTheme::isModalColorChooser() and
return false for RenderThemeChromiumMac

[Habib Virji, 2014-09-09 12:09:35]

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/HTMLInputEl...
> File Source/core/html/HTMLInputElement.cpp (right):
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
> Source/core/html/HTMLInputElement.cpp:1495:
> static_cast<ColorInputType*>(m_inputType.get())->didEndChooser();
> You could add a separate method like Internals::endColorChooser and use it to
> write a test for this CL.
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
> File Source/core/html/forms/ColorInputType.cpp (left):
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
> Source/core/html/forms/ColorInputType.cpp:191:
> element().dispatchFormControlChangeEvent();
> On Mac the color picker is a non-modal floating dialog so closing the closing
> the color picker should not be required to set the value. We want to dispatch
a
> change event just on Mac.
> 
> I think we should add a method like RenderTheme::isModalColorChooser() and
> return false for RenderThemeChromiumMac

Thanks keishi for reviewing. Will update the changes accordingly and push in new
patch.

[Habib Virji, 2014-09-10 16:30:55]

Thanks Keishi, sorry for delay in submission. Code updated as suggested, new
method in internals to check change event dispatch. Also updated code with
isModalColorChooser to differentiate mac and win/linux platform.

https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:1495:
static_cast<ColorInputType*>(m_inputType.get())->didEndChooser();
On 2014/09/09 11:55:51, keishi wrote:
> You could add a separate method like Internals::endColorChooser and use it to
> write a test for this CL.

Done.

https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
File Source/core/html/forms/ColorInputType.cpp (left):

https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
Source/core/html/forms/ColorInputType.cpp:191:
element().dispatchFormControlChangeEvent();
On 2014/09/09 11:55:51, keishi wrote:
> On Mac the color picker is a non-modal floating dialog so closing the closing
> the color picker should not be required to set the value. We want to dispatch
a
> change event just on Mac.
> 
> I think we should add a method like RenderTheme::isModalColorChooser() and
> return false for RenderThemeChromiumMac

Done.

[Habib Virji, 2014-09-12 12:42:59]

Patchset #2 (id:20001) has been deleted

[Habib Virji, 2014-09-12 13:43:55]

Patchset #2 (id:40001) has been deleted

[Habib Virji, 2014-09-12 16:00:12]

Patchset #2 (id:60001) has been deleted

[Habib Virji, 2014-09-17 05:52:31]

On 2014/09/10 16:30:55, Habib Virji wrote:
> Thanks Keishi, sorry for delay in submission. Code updated as suggested, new
> method in internals to check change event dispatch. Also updated code with
> isModalColorChooser to differentiate mac and win/linux platform.
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
> File Source/core/html/HTMLInputElement.cpp (right):
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/HTMLInputEl...
> Source/core/html/HTMLInputElement.cpp:1495:
> static_cast<ColorInputType*>(m_inputType.get())->didEndChooser();
> On 2014/09/09 11:55:51, keishi wrote:
> > You could add a separate method like Internals::endColorChooser and use it
to
> > write a test for this CL.
> 
> Done.
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
> File Source/core/html/forms/ColorInputType.cpp (left):
> 
>
https://codereview.chromium.org/513783002/diff/1/Source/core/html/forms/Color...
> Source/core/html/forms/ColorInputType.cpp:191:
> element().dispatchFormControlChangeEvent();
> On 2014/09/09 11:55:51, keishi wrote:
> > On Mac the color picker is a non-modal floating dialog so closing the
closing
> > the color picker should not be required to set the value. We want to
dispatch
> a
> > change event just on Mac.
> > 
> > I think we should add a method like RenderTheme::isModalColorChooser() and
> > return false for RenderThemeChromiumMac
> 
> Done.

@keishi, are the changes OK

[keishi, 2014-09-17 08:15:54]

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
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/...
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. The input.value should change right after the
user selects a a color in the color chooser and so we should be testing that.

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/color/input-color-onchange-event.html (right):

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/color/input-color-onchange-event.html:40:
internals.endColorChooser(input);
Ditto. No need to add this.

https://codereview.chromium.org/513783002/diff/100001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/100001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1488: if (type() != InputTypeNames::color)
We decided not to add any more if statements that check the type to
HTMLInputElement, and instead use InputType/InputTypeView.
Could you add a method like InputType::endColorChooser() and a comment saying
that it is for testing purposes only.

[Habib Virji, 2014-09-17 08:30:42]

Thanks keishi, will update HTMLInputElement. Can you only confirm about tests as
change event otherwise will not be generated. Should I check for change event in
those test?

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
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/...
LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html:24:
internals.endColorChooser(input);
On 2014/09/17 08:15:54, keishi wrote:
> I don't think we need to add this. The input.value should change right after
the
> user selects a a color in the color chooser and so we should be testing that.

Actually, line 17 above is printed in the test, as it is also testing change
event generated. Should I remove change event generation?

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/color/input-color-onchange-event.html (right):

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/color/input-color-onchange-event.html:40:
internals.endColorChooser(input);
On 2014/09/17 08:15:54, keishi wrote:
> Ditto. No need to add this.

Added as line 26 above, will not be printed about change event generation.

https://codereview.chromium.org/513783002/diff/100001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/100001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1488: if (type() != InputTypeNames::color)
On 2014/09/17 08:15:54, keishi wrote:
> We decided not to add any more if statements that check the type to
> HTMLInputElement, and instead use InputType/InputTypeView.
> Could you add a method like InputType::endColorChooser() and a comment saying
> that it is for testing purposes only.

Ok, will make a change and push in new patch.

[keishi, 2014-09-18 03:50:56]

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
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/...
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 2014/09/17 08:15:54, keishi wrote:
> > I don't think we need to add this. The input.value should change right after
> the
> > user selects a a color in the color chooser and so we should be testing
that.
> 
> Actually, line 17 above is printed in the test, as it is also testing change
> event generated. Should I remove change event generation?
You're right, this test is about change event generation so we shouldn't remove
it. This line should go after line 25.

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/color/input-color-onchange-event.html (right):

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/color/input-color-onchange-event.html:40:
internals.endColorChooser(input);
On 2014/09/17 08:30:42, Habib Virji wrote:
> On 2014/09/17 08:15:54, keishi wrote:
> > Ditto. No need to add this.
> 
> Added as line 26 above, will not be printed about change event generation.

We don't have a test for input events so I think we could add that here.

input.oninput = function() {
  debug("input event dispatched: " + input.value);
};

On non-mac, an input event should only be fired for the first
internals.selectColorInColorChooser but not for the second one.

[Habib Virji, 2014-09-18 12:42:05]

Thanks updated as suggested. Please have a look.

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
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/...
LayoutTests/fast/forms/color/input-color-choose-default-value-after-set-value.html:24:
internals.endColorChooser(input);
On 2014/09/18 03:50:55, keishi wrote:
> On 2014/09/17 08:30:42, Habib Virji wrote:
> > On 2014/09/17 08:15:54, keishi wrote:
> > > I don't think we need to add this. The input.value should change right
after
> > the
> > > user selects a a color in the color chooser and so we should be testing
> that.
> > 
> > Actually, line 17 above is printed in the test, as it is also testing change
> > event generated. Should I remove change event generation?
> You're right, this test is about change event generation so we shouldn't
remove
> it. This line should go after line 25.

I will but then expected files will need to be changed too, as previously when
selectColorInColorChooser is called, it generated change event and then color
input value was checked. As per your suggested change, it will be first input
value and then change event,

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/color/input-color-onchange-event.html (right):

https://codereview.chromium.org/513783002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/color/input-color-onchange-event.html:40:
internals.endColorChooser(input);
On 2014/09/18 03:50:55, keishi wrote:
> On 2014/09/17 08:30:42, Habib Virji wrote:
> > On 2014/09/17 08:15:54, keishi wrote:
> > > Ditto. No need to add this.
> > 
> > Added as line 26 above, will not be printed about change event generation.
> 
> We don't have a test for input events so I think we could add that here.
> 
> input.oninput = function() {
>   debug("input event dispatched: " + input.value);
> };
> 
> On non-mac, an input event should only be fired for the first
> internals.selectColorInColorChooser but not for the second one.

Done.

https://codereview.chromium.org/513783002/diff/100001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/100001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1488: if (type() != InputTypeNames::color)
On 2014/09/17 08:15:54, keishi wrote:
> We decided not to add any more if statements that check the type to
> HTMLInputElement, and instead use InputType/InputTypeView.
> Could you add a method like InputType::endColorChooser() and a comment saying
> that it is for testing purposes only.

Done.

[Habib Virji, 2014-09-18 12:42:26]

Patchset #4 (id:120001) has been deleted

[keishi, 2014-09-18 13:16:01]

lgtm

https://codereview.chromium.org/513783002/diff/140001/LayoutTests/platform/ma...
File
LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt
(right):

https://codereview.chromium.org/513783002/diff/140001/LayoutTests/platform/ma...
LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt:15:
FAIL onChange should be 0. Was 1.`
nit: Why is there a backtick at the end??

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser();
nit: Now I realize this seems unfortunate.
I see the name endColorChooser is already used for the method that closes the
chooser from blink,
and didEndChooser is collides with the method in ColorChooserClient.

Maybe it makes sense to call the methods on ColorChooserClient directly, because
we are trying to act as a fake ColorChooser.
if (ColorChooserClient* client = m_inputType->colorChooserClient())
  client->didEndChooser();

[Habib Virji, 2014-09-18 14:54:26]

@keishi: one last question regarding change in HTMLINputType. It appears your
suggestion does not work. Is it okay to keep my submitted solution.

https://codereview.chromium.org/513783002/diff/140001/LayoutTests/platform/ma...
File
LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt
(right):

https://codereview.chromium.org/513783002/diff/140001/LayoutTests/platform/ma...
LayoutTests/platform/mac/fast/forms/color/input-color-onchange-event-expected.txt:15:
FAIL onChange should be 0. Was 1.`
On 2014/09/18 13:16:00, keishi wrote:
> nit: Why is there a backtick at the end??
It was typo. Corrected.

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser();
This does not hold true, " ColorChooserClient* client =
m_inputType->colorChooserClient()" as inputType does not hold reference to
colorChooserClient().  

I suppose it is better to either use static_cast solution or use inputType
solution.

[Habib Virji, 2014-09-18 15:00:58]

Patchset #5 (id:160001) has been deleted

[keishi, 2014-09-19 03:04:12]

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser();
On 2014/09/18 14:54:26, Habib Virji wrote:
> This does not hold true, " ColorChooserClient* client =
> m_inputType->colorChooserClient()" as inputType does not hold reference to
> colorChooserClient().  
> 
> I suppose it is better to either use static_cast solution or use inputType
> solution. 

I was thinking, ColorInputType is a ColorChooserClient, so
InputType::colorChooserClient could return null and
ColorInputType::colorChooserClient could return this.

[Habib Virji, 2014-09-19 09:15:56]

Sorry keishi, for keeping on popping question back at you. 

Cannot understand actually suggested solution. As colorChooserClient cannot be
accessed in HTMLInputElement. ColorInputType is singleton class with instance
created from InputType.

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser();
Sorry, still not clear. 

Yes ColorInputType is derived from ColorChooserClient. 

--> ColorInputType::colorChooserClient 
This does not hold as there is no member function with colorChooserClient. 

--> InputType::colorChooserClient
Does not hold as this is an independent client associated only with
ColorInputType. It will not compile.

[keishi, 2014-09-22 09:58:36]

On 2014/09/19 09:15:56, Habib Virji wrote:
> Sorry keishi, for keeping on popping question back at you. 
> 
> Cannot understand actually suggested solution. As colorChooserClient cannot be
> accessed in HTMLInputElement. ColorInputType is singleton class with instance
> created from InputType.
ColorInputType is not a singleton, we create one for every element.

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser();
On 2014/09/19 09:15:56, Habib Virji wrote:
> Sorry, still not clear. 
> 
> Yes ColorInputType is derived from ColorChooserClient. 
> 
> --> ColorInputType::colorChooserClient 
> This does not hold as there is no member function with colorChooserClient. 
Yes I am proposing that you add it.
> --> InputType::colorChooserClient
> Does not hold as this is an independent client associated only with
> ColorInputType. It will not compile.

InputType::colorChooserClient will be a virtual method that will be overridden
in ColorInputType.

Something like this:
ColorChooserClient* InputType::colorChooserClient() { return 0; }
ColorChooserClient* ColorInputType::colorChooserClient() OVERRIDE { return this;
}

[Habib Virji, 2014-09-22 13:44:40]

Thanks Keishi, implemented as suggested. Can you please have a look, if all
changes are okay now.

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/513783002/diff/140001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.cpp:1487: m_inputType->didEndChooser();
On 2014/09/22 09:58:36, keishi wrote:
> On 2014/09/19 09:15:56, Habib Virji wrote:
> > Sorry, still not clear. 
> > 
> > Yes ColorInputType is derived from ColorChooserClient. 
> > 
> > --> ColorInputType::colorChooserClient 
> > This does not hold as there is no member function with colorChooserClient. 
> Yes I am proposing that you add it.
> > --> InputType::colorChooserClient
> > Does not hold as this is an independent client associated only with
> > ColorInputType. It will not compile.
> 
> InputType::colorChooserClient will be a virtual method that will be overridden
> in ColorInputType.
> 
> Something like this:
> ColorChooserClient* InputType::colorChooserClient() { return 0; }
> ColorChooserClient* ColorInputType::colorChooserClient() OVERRIDE { return
this;
> }
> 

Done.

[keishi, 2014-09-22 14:25:05]

Thanks. LGTM

[Habib Virji, 2014-09-22 14:59:38]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-09-22 15:00:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/513783002/200001

[commit-bot: I haz the power, 2014-09-22 16:13:56]

Committed patchset #6 (id:200001) as 182433

[javan, 2017-06-29 12:54:38]

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