875754 - Implement <input type="color">: Windows widget/color picker

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Comment 1

[Arnaud Bienner]

Attachment: Color input: windows widget

Work in progress:
- something should be done for Windows8 (but this should probably done in another bug).
- we need to make nsColorPicker async by moving work to another thread.

Comment 2

[Arnaud Bienner]

Attachment: Color input: Windows widget (1st)

Hi Jim,

Could you please review this patch? Its aim is to use the Windows native color picker for input type='color'.
All the important patches related to this (content, IDL, etc.) have already been landed in central, except the layout part, but a working patch is available in bug 875275, if you want to test this patch.

Comment 3

[Jim Mathies [:jimm]]

awesome, async! will take this for a spin today.

Comment 4

[Jim Mathies [:jimm]]

Do I have to flip any prefs to turn html5 form support on?

Comment 5

[Arnaud Bienner]

(In reply to Jim Mathies [:jimm] from comment #4)
> Do I have to flip any prefs to turn html5 form support on?
Oh, sorry, I forgot to mention this: dom.forms.color (disabled by default currently).

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 775385 [details] [diff] [review]
Color input: Windows widget (1st)

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

Is there supposed to be some sort of in-content UI for this? What I see is a ~5px text input I have to click in to bring up the picker.

r-ing for now, mostly due to nits and some cleanup. Generally looks good.

::: widget/windows/moz.build
@@ +40,5 @@
>      'WindowHook.cpp',
>      'nsAppShell.cpp',
>      'nsBidiKeyboard.cpp',
>      'nsClipboard.cpp',
> +    'nsColorPicker.cpp',

We don't require the 'ns' prefix anymore, just use 'ColorPicker'.

::: widget/windows/nsColorPicker.cpp
@@ +99,5 @@
> +NS_IMETHODIMP
> +AsyncColorChooser::Run()
> +{
> +  CHOOSECOLOR options;
> +  static COLORREF  customColors[16] = {0} ;

nit - extraneous spacing here.

@@ +110,5 @@
> +  options.Flags         = CC_RGBINIT | CC_FULLOPEN;
> +  options.rgbResult     = mInitialColor;
> +  options.lpCustColors  = customColors;
> +
> +  if (ChooseColor(&options) == TRUE) {

nit - you don't need the '== TRUE' here.

@@ +126,5 @@
> +// nsIColorPicker
> +
> +nsColorPicker::nsColorPicker()
> +{
> +  CoInitialize(NULL);

Why call this? I don't think you need it.

@@ +131,5 @@
> +}
> +
> +nsColorPicker::~nsColorPicker()
> +{
> +  CoUninitialize();

ditto.

::: widget/windows/nsWidgetFactory.cpp
@@ +138,5 @@
> +
> +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) {
> +#ifdef MOZ_METRO
> +    // TODO
> +    // picker = new nsMetroColorPicker;

Lets return an unsupported failure result here and maybe an assert that will show up in debug builds.

Comment 7

[Jim Mathies [:jimm]]

Attachment: test case

Comment 8

[Arnaud Bienner]

(In reply to Jim Mathies [:jimm] from comment #6)
> Is there supposed to be some sort of in-content UI for this? What I see is a
> ~5px text input I have to click in to bring up the picker.

As I wrote in comment 2, the layout part is missing currently (bug 875275) but a working patch is attached to the bug (attachment 773223 [details] [diff] [review]) that you can use for test: you will have colored button representing the chosen color instead of a small text input.

> r-ing for now, mostly due to nits and some cleanup. Generally looks good.

OK. Great :)

> 
> ::: widget/windows/moz.build
> @@ +40,5 @@
> >      'WindowHook.cpp',
> >      'nsAppShell.cpp',
> >      'nsBidiKeyboard.cpp',
> >      'nsClipboard.cpp',
> > +    'nsColorPicker.cpp',
> 
> We don't require the 'ns' prefix anymore, just use 'ColorPicker'.

I used this name because the IDL file is named like this, to keep coherency. Do you think the IDL file should also be changed?

Comment 9

[Jim Mathies [:jimm]]

(In reply to Arnaud from comment #8)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > Is there supposed to be some sort of in-content UI for this? What I see is a
> > ~5px text input I have to click in to bring up the picker.
> 
> As I wrote in comment 2, the layout part is missing currently (bug 875275)
> but a working patch is attached to the bug (attachment 773223 [details] [diff] [review]
> [diff] [review]) that you can use for test: you will have colored button
> representing the chosen color instead of a small text input.
> 
> > r-ing for now, mostly due to nits and some cleanup. Generally looks good.
> 
> OK. Great :)
> 
> > 
> > ::: widget/windows/moz.build
> > @@ +40,5 @@
> > >      'WindowHook.cpp',
> > >      'nsAppShell.cpp',
> > >      'nsBidiKeyboard.cpp',
> > >      'nsClipboard.cpp',
> > > +    'nsColorPicker.cpp',
> > 
> > We don't require the 'ns' prefix anymore, just use 'ColorPicker'.
> 
> I used this name because the IDL file is named like this, to keep coherency.
> Do you think the IDL file should also be changed?

I wouldn't worry about that file, I'd just update the file name of the src file you're checking in in widget/windows.

Comment 10

[Arnaud Bienner]

Attachment: Color input: Windows widget (2nd)

Here is an updated version of the patch, with your comments applied.
The only thing I haven't changed is the name of the file: IMHO it's better to keep coherency with the IDL file, whose name starts with "ns".
Also, this makes the interdiff easier to read.
But I you really think this should be changed, I will update the patch to change this.

Comment 11

[Arnaud Bienner]

Attachment: Color input: Windows widget (2nd): interdiff

And here is the inter-diff.

Comment 12

[Arnaud Bienner]

Thanks Jim for the review :)

There is still the color picker for Metro to implement, but unfortunately, I don't have Windows 8 so I can't do it myself.
Do you know someone who might be interested in implementing it?

Also, maybe we should open a new bug for this?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 777727 [details] [diff] [review]
Color input: Windows widget (2nd)

You can just use the checkin-needed keyword in the future.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de1f0de356a1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de1f0de356a1

Comment 16

[Sebastian Zartner [:sebo]]

Attachment: Color picker dialog under Windows 7

The arrow for setting the luminance is too big. It should normally be much smaller.[1]

Sebastian

[1] http://support.prentrom.com/images/eco/Colors.jpg

Comment 17

[Arnaud Bienner]

Ah yes, indeed. That's strange. I don't know why we end up with a big arrow.

Comment 18

[Arnaud Bienner]

I tested with Paint, and indeed, the narrow is smaller.
But with in the Appearance menu, from Control Panel, the arrow is as big as the one we have.

I'm not sure what is the best.
I'm not sure we need a such bigger arrow but, on the other hand it doesn't hurt IMHO and it probably helps to see more precisely the luminance selected.