875747 - nsIColorPicker widget

Status: RESOLVED FIXED

(Core :: Widget, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 766269 [details] [diff] [review]
Patch

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

Why is this an XPIDL interface? Can't we make it just a C++ abstract base class?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 766269 [details] [diff] [review]
> Patch
> 
> Review of attachment 766269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why is this an XPIDL interface? Can't we make it just a C++ abstract base
> class?

We want to be able to create JS-based implementations for Firefox OS or create some glue for a Firefox Android implementation too. An XPCOM component sounds like the best way to do that.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 766269 [details] [diff] [review]
Patch

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

::: widget/nsIColorPicker.idl
@@ +49,5 @@
> +  *
> +  * @param      parent       nsIDOMWindow parent.  This dialog will be dependent
> +  *                          on this parent. parent must be non-null.
> +  * @param      title        The title for the color widget
> +  * @param      initialColor The initial color to show when the picker is opened.

Document the format of colors here. Should all CSS color values be supported?

@@ +58,5 @@
> +  * The string representing the selected color.
> +  * While the update() or done() methods are not called, this will return the
> +  * color passed in the init() function.
> +  */
> +  readonly attribute AString color;

Wouldn't it be better to get rid of this and just pass the color to  nsIColorPickerShownCallback::done and update?

Comment 5

[Arnaud Bienner]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Document the format of colors here. Should all CSS color values be supported?

No.
As we would like to use it for <input type=color>, we need a "valid simple color", as stated here: http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#color-state-%28type=color%29
definition is here: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#valid-simple-color
I don't know if just mentioning "valid simple color" is enough, or if it would be better to add a reference to the spec or to describe it briefly.

> Wouldn't it be better to get rid of this and just pass the color to 
> nsIColorPickerShownCallback::done and update?

When I wrote the first nsIColorPickerShownCallback, before Mounir work on it, I did it this way because it's how it works for nsIFilePickerShownCallback.
But I'm not sure what is better.
Passing "color" to "done" and "update" sounds simpler but this extra parameter might be superfluous (and maybe confusing) when "result == returnCancel" because in that case we shouldn't care about the color.
But I have no strong opinion about this.
Mounir: is there any special reason why you keep it like this?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Comment on attachment 766269 [details] [diff] [review]
> Patch
> 
> Review of attachment 766269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsIColorPicker.idl
> @@ +49,5 @@
> > +  *
> > +  * @param      parent       nsIDOMWindow parent.  This dialog will be dependent
> > +  *                          on this parent. parent must be non-null.
> > +  * @param      title        The title for the color widget
> > +  * @param      initialColor The initial color to show when the picker is opened.
> 
> Document the format of colors here. Should all CSS color values be supported?

The format is what Arnaud described in comment 5. I actually explain that this class (nsIColorPicker) is using the HTML <input type='color'> at the beginning of the file. Isn't that enough?

> @@ +58,5 @@
> > +  * The string representing the selected color.
> > +  * While the update() or done() methods are not called, this will return the
> > +  * color passed in the init() function.
> > +  */
> > +  readonly attribute AString color;
> 
> Wouldn't it be better to get rid of this and just pass the color to 
> nsIColorPickerShownCallback::done and update?

There are two reasons behind this choice. First of all, nsIFilePicker does it like that. Second, .done() already have a parameter (ok/cancel) and having a second one that is not always relevant sounds odd. Between that and having .color in nsIColorPicker I tend to prefer having .color but I actually don't care that much. If you really prefer to pass the color value in the callback methods, I would be glad to change this.
Ideally, we should have a Promise mechanism for this but the Promise code still lives behind a pref.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mounir Lamouri (:mounir) from comment #6)
> The format is what Arnaud described in comment 5. I actually explain that
> this class (nsIColorPicker) is using the HTML <input type='color'> at the
> beginning of the file. Isn't that enough?

I guess it is, but you could repeat it in init() for clarity, and specify what happens if an invalid color value is passed.

> > @@ +58,5 @@
> > > +  * The string representing the selected color.
> > > +  * While the update() or done() methods are not called, this will return the
> > > +  * color passed in the init() function.
> > > +  */
> > > +  readonly attribute AString color;
> > 
> > Wouldn't it be better to get rid of this and just pass the color to 
> > nsIColorPickerShownCallback::done and update?
> 
> There are two reasons behind this choice. First of all, nsIFilePicker does
> it like that. Second, .done() already have a parameter (ok/cancel) and
> having a second one that is not always relevant sounds odd. Between that and
> having .color in nsIColorPicker I tend to prefer having .color but I
> actually don't care that much. If you really prefer to pass the color value
> in the callback methods, I would be glad to change this.

I think it's better to pass the colors as parameters. Not a big deal though.

Comment 8

[Arnaud Bienner]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> and specify what happens if an invalid color value is passed.

Currently in the color widgets patches we started to implement, we've added assert to check the color value string is correct.
In bug 875753, the idea of using return code instead has been proposed.
Or another solution would be to do some best effort and set a default color (#00000 as the spec says for input type color) if the passed color isn't valid. But this sounds a bit more complicated to have this implemented in very widget, and we probably want to warn the user in some way if something is wrong, instead of defaulting to another value.

Anyway, I think this point should be clarified indeed, as once we will agree on the IDL, we will be able to continue to implement the widgets.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 766269 [details] [diff] [review]
Patch

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

I'm waiting for a new patch here.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Changes are:
- "color" parameters are now ACString instead of AString;
- when there is a "color" taken as a parameter, a pointer to the format description is added;
- the behaviour of intialColor not being of the expected format is mentioned;
- .color in nsIColorPicker is removed;
- returnValue is removed in favour of empty string vs non-empty string.

WDYT?

Comment 11

[Arnaud Bienner]

Comment on attachment 770171 [details] [diff] [review]
Patch v2

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

Looks fine to me.

::: widget/nsIColorPicker.idl
@@ +48,5 @@
> + /**
> +  * Initialize the color picker widget. The color picker will not be shown until
> +  * open() is called.
> +  * If the backend doesn't support setting a title to the native color picker
> +  * widget, the title parameter might be ignored.

Nice idea to specify this, as we've seen we can't set the window's title on Windows.

Comment 12

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3153ceaba0

Now we can start updating the backends ;)

Comment 13

[Arnaud Bienner]

(In reply to Mounir Lamouri (:mounir) from comment #12)
> Now we can start updating the backends ;)

Great!
And we should also update bug 885996.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f3153ceaba0