1009243 - input type="color" doesn't fire "input" events on Windows when the user modifies the color in the picker

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Yohan Boniface [:ybon]]

In FF 29, according to my tests, both "input" and "change" events are fired on color commit, and on commit only (i.e. when hitting "OK").
This doesn't allow to track the changes in real time, for example to create a live preview.
Maybe this should be handled by a input/change events more specialized, having "input" fired at each color change (and then updating the input value) and "change" fired on commit.

On a side note, Chromium seems to have implemented it the other way around: every color change made by the user updates the input value, and the "change" event is fired. And there is no "OK" nor "Cancel" button.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1005206 for a real life use case.

Comment 1

[Anthony Ricaud (:rik)]

From http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#event-input-input : 
> The input event fires whenever the user has modified the data of the control. The change event fires when
> the value is committed, if that makes sense for the control, or else when the control loses focus. In all
> cases, the input event comes before the corresponding change event (if any).

This sounds like a bug in our implementation.

Comment 2

[Arnaud Bienner]

On OS X and Linux, we fire an "update" event when the user selection changes.
Maybe we should fire an "input" event instead so?

However, for Windows, I didn't find an easy way to get the color while the user is editing it (the ChooseColor function is always synchronous IIRC, and I don't think there are native mechanism to be notified if the user changed the color until he pressed "OK").

Comment 3

[Yohan Boniface [:ybon]]

(In reply to Arnaud Bienner from comment #2)
> On OS X and Linux, we fire an "update" event when the user selection changes.

I've tested "update" also before opening the issue (it was mentioned in the long reference bug), but it doesn't seem to be fired at all on my FF 29.0 on Ubuntu.
Here is the minimal test case I'm using: https://gist.github.com/yohanboniface/fe8f43fe2f5c497b9579
(It's worth testing also on Chromium, where both "change" and "input" are fired at every move in the palette :/)

Also note that, IMHO, firing "input" at every move in the palette is not enough: the input value must also be updated, for the listener to be able to read it.

Comment 4

[Arnaud Bienner]

(In reply to Yohan Boniface [:ybon] from comment #3) 
> I've tested "update" also before opening the issue (it was mentioned in the
> long reference bug), but it doesn't seem to be fired at all on my FF 29.0 on
> Ubuntu.

...because this was done in a second step (bug 966417) and will be in Firefox 30.
On OS X, this was part of the initial implementation, so should work in Firefox 29.

> Also note that, IMHO, firing "input" at every move in the palette is not
> enough: the input value must also be updated, for the listener to be able to
> read it.

IIRC it is: value and the input button's color are both updated.

Comment 5

[Arnaud Bienner]

(In reply to Arnaud Bienner from comment #2)
> On OS X and Linux, we fire an "update" event when the user selection changes.
> Maybe we should fire an "input" event instead so?

Sorry for the confusion :(
We fire an "input" event, as expected, when user changes its selection (but the color picker is still opened).
Internally, the method we used is called "Update", so that's where my confusion came from.

So we correctly fire "input" events, as expected, and "change" event when user click OK (or simply close the color picker on OS X) for Linux and OS X.

We should rename that bug to specify the user's input isn't tracked real time for Windows only (or open a new one).

But as I previously said, I'm afraid there is no easy way to do this on Windows.

Comment 6

[Arnaud Bienner]

IMHO this bug should be closed and we should open a new bug (or rename this one) for Windows only, as, as I said, it's working as expected on OS X and Gtk.

I don't think we fire "input" events when choosing color in Android but, as we provide our own color picker, this should be easy to change if we think we should change the behavior for Android as well.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Arnaud Bienner from comment #6)
> IMHO this bug should be closed and we should open a new bug (or rename this
> one) for Windows only

Renamed this one.

I wonder if we should just not use the native picker on Windows if this isn't fixable... :-(

What do other browsers do here? Do IE and Chrome just reimplement the picker themselves?

Comment 8

[Arnaud Bienner]

(In reply to :Gijs Kruitbosch from comment #7)
> I wonder if we should just not use the native picker on Windows if this
> isn't fixable... :-(

I wonder the same. Moreover, Windows native color picker isn't very nice and useful. The only good thing about using it is consistency: Windows users are used to see this color picker, so they will not be disappointed.
Except this, it's pretty bad IMO.
An interesting thing about using a native color picker is that it usually provides a pipette to select a color on the screen, which is pretty useful. Windows' color picker doesn't have this.
The color picker we use in the web developer tools is pretty nice. Maybe we should use that one on Windows (I believe this should be feasible, but I don't know how hard to implement it will be).

> What do other browsers do here? Do IE and Chrome just reimplement the picker
> themselves?

IIRC IE doesn't implement input type=color and Chrome uses the native color picker, with the same drawback as us.

Comment 9

[Biru [:poiru]]

(In reply to :Gijs Kruitbosch from comment #7)
> I wonder if we should just not use the native picker on Windows if this
> isn't fixable... :-(

It should be fixable. I'll take a look.

Comment 10

[Arnaud Bienner]

(In reply to Birunthan Mohanathas [:poiru] from comment #9)
> It should be fixable. I'll take a look.

Good luck ;) Let us know if you find something.

I'm not a Windows expert, but I tried to find a solution for this.
But I didn't find anything in MSDN documentation [1] [2] about a callback which can be called when the user change its selection.
I found only the color picker dialog which run synchronously and returns the color selected by the user when closed.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms646912%28v=vs.85%29.aspx
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/ms646830%28v=vs.85%29.aspx

Comment 11

[Biru [:poiru]]

Attachment: Make <input type="color"> fire "input" events on Windows

(In reply to Arnaud Bienner from comment #10)
> (In reply to Birunthan Mohanathas [:poiru] from comment #9)
> > It should be fixable. I'll take a look.
> 
> Good luck ;) Let us know if you find something.
> 
> I'm not a Windows expert, but I tried to find a solution for this.
> But I didn't find anything in MSDN documentation [1] [2] about a callback
> which can be called when the user change its selection.

This should do it. Not pretty, but it works.

Comment 12

[Arnaud Bienner]

Comment on attachment 8476793 [details] [diff] [review]
Make <input type="color"> fire "input" events on Windows

Review of attachment 8476793 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like a solution :)
Unfortunately I can't test your patch as I don't have a Windows dev environment anymore.
I'm wondering how your hack is "hacky" i.e. can WM_CTLCOLORSTATIC be emitted for other reasons than user changing the color? (if yes, this might be problematic)

::: widget/windows/nsColorPicker.cpp
@@ -93,4 @@
>  AsyncColorChooser::AsyncColorChooser(const nsAString& aInitialColor,
>                                       nsIWidget* aParentWidget,
>                                       nsIColorPickerShownCallback* aCallback)
> -  : mInitialColor(aInitialColor)

I believe you should keep thi initial color because...

@@ +130,1 @@
>      }

...here, we should now have an "else" to restore the initial color, in case user clicked "Cancel".
Otherwise, as mColor was updated through Update, it will be passed to the callback after (if I haven't missed anything), so "Cancel" will not ready cancel the color selection.

Comment 13

[Arnaud Bienner]

(In reply to Arnaud Bienner from comment #12)
> callback after (if I haven't missed anything), so "Cancel" will not ready
*really

Comment 14

[Biru [:poiru]]

Attachment: Part 1: Store color as a COLORREF rather than a nsString in nsColorPicker

Comment 15

[Biru [:poiru]]

Attachment: Part 2: Make <input type="color"> fire "input" events on Windows

(In reply to Arnaud Bienner from comment #12)
> I'm wondering how your hack is "hacky" i.e. can WM_CTLCOLORSTATIC be emitted
> for other reasons than user changing the color? (if yes, this might be
> problematic)

Controls send WM_CTLCOLORSTATIC to their parent window when the control is about to be drawn, so we should be fine. Even if the control displaying the color were to send it for some other reason, the GetPixel() call should return the correct result.

> ...here, we should now have an "else" to restore the initial color, in case
> user clicked "Cancel".

Good catch, fixed.

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8476922 [details] [diff] [review]
Part 2: Make <input type="color"> fire "input" events on Windows

Review of attachment 8476922 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent work, thanks!

::: widget/windows/nsColorPicker.cpp
@@ +90,5 @@
>    aResult.Append(ToHexString(b));
>  }
>  } // anonymous namespace
>  
> +static AsyncColorChooser* gColorChooser = nullptr;

nit, you don't have to set this to nullptr, this is the default for static globals.

@@ +165,5 @@
> +  if (aMsg == WM_CTLCOLORSTATIC) {
> +    // The color picker does not expose a proper way to retrieve the current
> +    // color, so we need to obtain it from the static control displaying the
> +    // current color instead.
> +    const int kCurrentColorBoxID = 709;

fairly common thing to do actually, I wouldn't consider this a hack. Plus in an ancient dialog like this, it's unlikely the id will change.

Comment 17

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4725899adffa
https://hg.mozilla.org/integration/mozilla-inbound/rev/47ecfa3c421f

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ed7967e9724d

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4725899adffa
https://hg.mozilla.org/mozilla-central/rev/47ecfa3c421f