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

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

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

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
Assignee: nobody → arnaud.bienner
Blocks: 547004
Depends on: 875747
(Assignee)

Comment 1

6 years ago
Created attachment 766457 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 775385 [details] [diff] [review]
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.
Attachment #775385 - Flags: review?(jmathies)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 3

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

Comment 4

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

Updated

5 years ago
Attachment #766457 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
(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

5 years ago
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-

Comment 7

5 years ago
Created attachment 776446 [details]
test case
(Assignee)

Comment 8

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 777727 [details] [diff] [review]
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.
Attachment #775385 - Attachment is obsolete: true
Attachment #777727 - Flags: review?(jmathies)
(Assignee)

Comment 11

5 years ago
Created attachment 777728 [details] [diff] [review]
Color input: Windows widget (2nd): interdiff

And here is the inter-diff.

Updated

5 years ago
Attachment #777727 - Flags: review?(jmathies) → review+
(Assignee)

Updated

5 years ago
Attachment #777727 - Flags: checkin?
(Assignee)

Comment 12

5 years ago
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+

Updated

5 years ago
Blocks: 895464
https://hg.mozilla.org/mozilla-central/rev/de1f0de356a1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Created attachment 823244 [details]
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
(Assignee)

Comment 17

5 years ago
Ah yes, indeed. That's strange. I don't know why we end up with a big arrow.
(Assignee)

Comment 18

5 years ago
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)

Updated

5 years ago
Flags: needinfo?(jmathies)

Updated

5 years ago
Depends on: 972896
You need to log in before you can comment on or make changes to this bug.