Closed Bug 875754 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Widget: Win32, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
Assignee: nobody → arnaud.bienner
Blocks: 547004
Depends on: 875747
Attached patch Color input: windows widget (obsolete) — Splinter Review
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.
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.
Attachment #775385 - Flags: review?(jmathies)
Status: NEW → ASSIGNED
awesome, async! will take this for a spin today.
Do I have to flip any prefs to turn html5 form support on?
Attachment #766457 - Attachment is obsolete: true
(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 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.
Attachment #775385 - Flags: review?(jmathies) → review-
Attached file test case
(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?
(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.
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.
Attachment #775385 - Attachment is obsolete: true
Attachment #777727 - Flags: review?(jmathies)
And here is the inter-diff.
Attachment #777727 - Flags: review?(jmathies) → review+
Attachment #777727 - Flags: checkin?
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 on attachment 777727 [details] [diff] [review]
Color input: Windows widget (2nd)

You can just use the checkin-needed keyword in the future.
Attachment #777727 - Flags: checkin? → checkin+
Blocks: 895464
https://hg.mozilla.org/mozilla-central/rev/de1f0de356a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Ah yes, indeed. That's strange. I don't know why we end up with a big arrow.
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.
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Depends on: 972896
You need to log in before you can comment on or make changes to this bug.