875750 - Implement <input type="color">: Android widget/color picker

Status: RESOLVED MOVED

(GeckoView :: IME, enhancement, P3)

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Dupe of bug 792457?

Comment 3

[Mounir Lamouri (:mounir)]

For information, Chrome Android is going to ship this:
https://code.google.com/p/chromium/issues/detail?id=135771

Comment 4

[Wesley Johnston (:wesj)]

I was working on a couple implementations here: https://github.com/wesj/Pickers
apk: http://people.mozilla.com/~wjohnston/ColorPicker.apk

Screenshots:
http://dl.dropboxusercontent.com/u/72157/picker1.png
http://dl.dropboxusercontent.com/u/72157/picker2.png

Haven't gotten much UX feedback though, and I know they're swamped.

Comment 5

[Tim Nguyen :ntim]

(In reply to Wesley Johnston (:wesj) from comment #4)
> I was working on a couple implementations here:
> https://github.com/wesj/Pickers
> apk: http://people.mozilla.com/~wjohnston/ColorPicker.apk
> 
> Screenshots:
> http://dl.dropboxusercontent.com/u/72157/picker1.png
> http://dl.dropboxusercontent.com/u/72157/picker2.png
> 
> Haven't gotten much UX feedback though, and I know they're swamped.

Try this look : https://chromium.googlecode.com/issues/attachment?aid=1357710031000&name=color_picker_landscape.png&token=SvIeUdTnuas2xVuxTzUvfm4yPx8%3A1379105722557&inline=1
It's the same as Chrome's so it should fit Android guidelines well.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

More helpfully

https://chromiumcodereview.appspot.com/11316153
https://code.google.com/p/chromium/issues/detail?id=135771

The code is BSD licensed, https://www.mozilla.org/MPL/license-policy.html#Licenses_Compatible_with_the_MPL

Comment 7

[Wesley Johnston (:wesj)]

Chrome's look like (pardon my crazy fonts):

http://dl.dropboxusercontent.com/u/72157/colors1.png
and if you hit "more":
http://dl.dropboxusercontent.com/u/72157/colors2.png

which seems a bit silly to me. I don't know that you'd ever want ANY of the colors in that first box. The second one exposes hsv to users, which I also was hoping to avoid.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: android-color-picker.patch

For the color picking dialog, I copied in from the android API demo and modified it slightly.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Brad - Got a screenshot of this for Ian to look at?

Comment 10

[Wesley Johnston (:wesj)]

We already have a pretty easy to use system for this that we use all over the place using prompts (and that's extendable for extensions). Please don't do this in widget.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 821745 [details] [diff] [review]
android-color-picker.patch

throwing to Wes for a first pass?

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 821745 [details] [diff] [review]
android-color-picker.patch

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

This looks like:

http://dl.dropboxusercontent.com/u/72157/colorpicker.png

i.e. it only allows changing the hue. Pretty biased, but I think I would rather write this in js/java so that its easier to maintain. I can imagine two ways?

1.) Implement nsIColorPicker as a js component. The component can load Prompts.jsm. We can add a method to it
Prompt addColorPicker({
  value: 0xFFAABBCC,
  options: ["red", "green", "blue"]
});
In Java its pretty simple now to write a PromptInput (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/PromptInput.java) that will provide a picker to show on the dialog. The "click in the middle to close" bit is tougher (but I'm also happy to throw that out).

2.) Stub nsIColorPicker to do nothing for us, and instead do this in InputWidgetHelper.js, like we currently do for date pickers. http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/InputWidgetHelper.js That gives us a bit more freedom to inspect the DOM and customize the picker, at the expense being a little less xpcomy.

Objections? Opinions?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

The UI is in Java.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: colorpicker.patch

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: color_picker.png

This will do all the colors

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Chromium patch

I don't think we can move this forward without UI from ian. This UI isn't good enough to do anything with.

This patch just copies over all the chromium color picker code. i.e. it looks and behaves exactly like the widget in comment 7. Build at http://people.mozilla.com/~wjohnston/colorpicker.apk if you want to try it. Not my favorite thing ever, but maybe a good first step?

I've updated the stuff I was playing with on github over time too, if there's any desire to try some varient of that. http://wesj.github.io/Pickers/screenshots.html

Comment 17

[Ian Barlow (:ibarlow)]

Adding Anthony for some visual design love.

Anthony, we're looking for some help here in locking down a design for a pop-up color picker. We would display this for websites that use an html5 <input type="color"> control.

Take a look at comment 7 for how Chrome handles this -- they start with a grid of (gross looking) defaults, and offer a 'more' button to go into a full on HSL editor. This could be one approach. Another could be something like the 'color wheel' designs Brad and Wes have been exploring in this bug.

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I'm on it. 

Just have a question about functionality that I couldn't really find answers to from just briefly scanning the log here.

1) Do we want to add the ability to be able to type in specific CMYK, RGB, or HEX values?
2) Do we want to have presets/ ability to add a fixed/arbitrary amount of colors? 

I have seen some of Wes's implementations though so I will use them as a great reference point!

Comment 19

[Wesley Johnston (:wesj)]

(In reply to Anthony Lam from comment #18)
> 1) Do we want to add the ability to be able to type in specific CMYK, RGB,
> or HEX values?
I think that's a UX question. Will people interacting with these want super precision to say "These are the exact RGB/CMYK values I want", or are we focusing on a more basic "I think this is pretty" interation.

> 2) Do we want to have presets/ ability to add a fixed/arbitrary amount of
> colors? 
I'm fine with some presets, and will probably try to make the implementation generic (i.e. easy to change/add/remove), assuming the design doesn't make that really really hard :) The "grid" that I was playing with in my github repo is smart about redistributing rows/cols based on the number of colors given to it.

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

From a UX standpoint I would say that specific values are typically very useful on mobile as touch sensitivity makes it hard to pinpoint a specific color. But with that being said, there should also be some sensible "pretty" presets that will help answer those "I think this is pretty" use cases.

Also digging that smart presets grid you have going on in that repo. Will look at offering both in my design!

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Gave this initial design a go. I tried to keep most (if not all) the issues we've discussed in mind. That being said, I would very much like to get feedback and see how feasible this solution is, especially with the array of controls and side-by-side input fields on an already "popped up" prompt.

Please find the design attached! :) http://cl.ly/image/0p073J3D0k0X

Comment 22

[Wesley Johnston (:wesj)]

(In reply to Anthony Lam from comment #21)
> Gave this initial design a go. I tried to keep most (if not all) the issues
> we've discussed in mind. That being said, I would very much like to get
> feedback and see how feasible this solution is, especially with the array of
> controls and side-by-side input fields on an already "popped up" prompt.
> 
> Please find the design attached! :) http://cl.ly/image/0p073J3D0k0X

We'll also need to make sure these are designed for landscape mode (and tablets? although they can just use this...)

Comment 23

[Ian Barlow (:ibarlow)]

(In reply to Anthony Lam from comment #21)
> Gave this initial design a go. I tried to keep most (if not all) the issues
> we've discussed in mind. That being said, I would very much like to get
> feedback and see how feasible this solution is, especially with the array of
> controls and side-by-side input fields on an already "popped up" prompt.
> 
> Please find the design attached! :) http://cl.ly/image/0p073J3D0k0X

Thanks for sharing this, Anthony. Some feedback:

I like the circle colour swatches, in addition to having a separate way to handle colour/tint/saturation. 

I wonder if there is a way to make the colour sliders a little more interesting and modern, they feel a little generic here. Maybe the sliders should be a different shape, or are wider, or something. Not sure.

I also worry a little about exposing the entire set of controls at once -- allowing users to see inputs for percentages and hex values right away might be a bit of decision overload. Maybe a 2-stage menu, where the first piece is just about picking a colour or doing sliders, and a "more" or "advanced" button that exposes the more fine grain text input controls?

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hey guys, as per the feedback given, we've revised the design a bit and came up with the following. I think that in the interest of time, we might be better off implementing the first screen and then adding the "Advanced" one later on. 

I hope the interactions for the first one are clear enough, the colors are chosen as either direct replicas of Ian's design values wheel or a slight variation of it.

Let me know if you have any questions!

http://cl.ly/image/2P1N0e1I0W1J

Comment 25

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Updated "Advanced" screen with subtle shadows :)

http://cl.ly/image/392j0d242B0v

please use this one!

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 27

[Wesley Johnston (:wesj)]

This is just the basic version of this (i.e. a list of colors):

https://www.dropbox.com/s/yxuwyfptczq8fp0/colorpicker1.png

build at:

http://people.mozilla.org/~wjohnston/colorpicker.apk

If the color picker is already set to a value that's not in our list of defaults, I add that color to the bottom of our list (hence, the pink row shown). Everything else is... pretty self explanatory.

We could do a different message here. ColorPicker:Show? Even if we do that, I'd still like to use Prompt.java for Fennec's implementation. Not a huge change (but a bit more work and we might want to take something like the async stuff in bug 946344 to make it a little cleaner). This seemed most straightforward to me though.

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

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

>diff --git a/mobile/android/base/prompts/Prompt.java b/mobile/android/base/prompts/Prompt.java

>-    private View applyInputStyle(View view) {
>+    private View applyInputStyle(View view, PromptInput input) {
>+        // Don't add padding to color picker views
>+        if (input instanceof ColorPickerInput)
>+            return view;
>+

Can you add a method to PromptInput that makes it easy to skip this without casting?

PromptInput.canApplyInputStyle()

>diff --git a/mobile/android/base/widget/BasicColorPicker.java b/mobile/android/base/widget/BasicColorPicker.java
>+    private Drawable getCheckDrawable() {

>+            getContext().getTheme().resolveAttribute(android.R.attr.listPreferredItemHeigââht, typedValue, true);

Something funky in there

>diff --git a/mobile/android/modules/Prompt.jsm b/mobile/android/modules/Prompt.jsm

>+  addColorPicker: function(aOptions) {
>+    return this._addInput({
>+      type: "color",
>+      value: aOptions.value,
>+      id: aOptions.id
>+    });
>+  },

If we feel like adding more options to this later we are already setup to handle it :)

Nice code. Turned out looking pretty nice.

Screenshot?

Comment 29

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/9101f35b7733

Landed, but happy to adjust if UX has changes. leaving-open for the advanced work.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9101f35b7733

Comment 31

[Arnaud Bienner]

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

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

::: widget/android/nsLookAndFeel.cpp
@@ +397,5 @@
>              aResult = 1;
>              break;
>  
> +        case eIntID_ColorPickerAvailable:
> +            aResult = 1;

As you're activating input type color styling, the fails-if(Android) in layout/reftests/forms/input/color/reftest.list will not be valid anymore.
As the tests should not fail anymore, I'm afraid they will not be green with this patch. And even if they are, we should reactivate them now we have color picker for Android.

Comment 32

[Aaron Train [:aaronmt]]

(In reply to Wesley Johnston (:wesj) from comment #29)
> https://hg.mozilla.org/integration/fx-team/rev/9101f35b7733
> 
> Landed, but happy to adjust if UX has changes. leaving-open for the advanced
> work.

Comment 33

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sorry bout the late reply here. I've actually had a chance to check it out and it looks awesome - Great work! I'm finding jumping around from color to color very whimsical indeed.

Comment 34

[Wesley Johnston (:wesj)]

Comment on attachment 829598 [details] [diff] [review]
colorpicker.patch

Clearing

Comment 35

[Wesley Johnston (:wesj)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature. Wanted because this is also shipping on Desktop in 28.
User impact if declined: No color picker
Testing completed (on m-c, etc.): Been on central for a week now
Risk to taking this patch (and alternatives if risky): Pretty low risk. New feature. Rarely seen on the web. Alternative is to not ship it. Easy to pref off though.
String or IDL/UUID changes made by this patch: None!

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Since it's easy to pref off, we can uplift and let it bake on Aurora.

Comment 37

[Wesley Johnston (:wesj)]

I've been playing with the advanced version of this in my spare time:

https://github.com/wesj/WheelColorPicker/
http://people.mozilla.com/~wjohnston/color.apk

Comment 38

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #37)
> I've been playing with the advanced version of this in my spare time:
> 
> https://github.com/wesj/WheelColorPicker/
> http://people.mozilla.com/~wjohnston/color.apk

Maybe you can publish this widget as an open source library :)

Comment 39

[Wesley Johnston (:wesj)]

Noticed that I never landed the basic version on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/84c1ccd31057

Comment 40

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Polished up the UI a little bit. Nothing major, removed the unnecessarily large Hue bar and equally unnecessary shading on the wheel.

https://people.mozilla.org/~alam/Shots/fx_android_htmlcolorp4.png

Comment 41

[Wesley Johnston (:wesj)]

This is back burner for now, so if some volunteer wants to do it, I'd be happy to let you take it. The github repo is up to date:

https://github.com/wesj/WheelColorPicker/

if you want somewhere to start (but feel free to do your own thing :) ).

Comment 42

[Wesley Johnston (:wesj)]

Attachment: forms.html

The basic color picker is turned on in FF29 and will ship there. Here's a quick test page for it.

The advanced one is still WIP looking for someone with time to finish it up.

Comment 43

[Arnaud Bienner]

I will try to have a look when I will have some time.
I think we should really provide an advanced option for the color picker: otherwise it is unnecessarily restrictive.

Comment 44

[Arnaud Bienner]

Thanks Wesley for updating your Github project! It works like a charm now, and, wow! it looks awesome :)
So awesome that I'm wondering what remains to be done actually?
I just noticed a few things:
- the first slider isn't displayed correctly when dialog is opened.
- The HSV text fields are hard to read.
Apart from that (+ probably some polishing), and integration into Firefox, is there anything else to do I'm missing?

Comment 45

[Wesley Johnston (:wesj)]

Heh. Great!

The biggest thing UX noticed when I last showed them was that the wheel didn't pan kinetically. we should fix that (i.e. you should be able to fling the wheel) I think I forced off hardware acceleration in the github project as well, because it broke things, so it would be good to test that. The designs have changed slightly since as well (I think the HUE slider on the HSV one now looks like the others (although I liked the old one). There's some other little things too. Localizing. The initial variables. Etc. Polishing takes time :(

BUT, if you just wanted to get things integrated into our build system (you can either put them in as a third party resources in the mobile/android/thirdparty folder or you can change their namespaces and put them directly in mobile/android/base/widget and just change all the packages, I don't mind). Once you have the files in the tree even (even if it doesn't build), we can have someone who is not me give feedback (lucasr would have a lot of good feedback I'm sure).

Comment 46

[Arnaud Bienner]

OK, got the global picture more or less now, thanks :)
I will start to fix the few things you mentioned, + the ones I noticed. That would give me the opportunity to play a bit with it and to check everything is working well.

About, integrating it into the build system, I believe I can leave this for the end, when it will be ready (and don't see advantages of doing it now).

Comment 47

[Wesley Johnston (:wesj)]

I agree. Mostly worried that dumping a large amount of code to review into someone's lap will be tough on them. So the earlier you can let people look, the better :) If there are ways to break the patch up into smaller pieces, that also helps sometimes :)

Comment 48

[Arnaud Bienner]

(In reply to Wesley Johnston (:wesj) from comment #45)
> BUT, if you just wanted to get things integrated into our build system (you
> can either put them in as a third party resources in the
> mobile/android/thirdparty folder or you can change their namespaces and put
> them directly in mobile/android/base/widget and just change all the
> packages, I don't mind).

How can I easily do this?
I would prefer to put everything in 3rd party (so hopefully the code can be reused for other Android applications, as we keep it open source).
But in addition of simply copying the java code to thirdparty dir and updating moz.build etc I believe I should also need to copy the layout files (and maybe some other resources files) somewhere; but I'm not sure where?

Ideally, I hope I just need to copy files from the Github repo to Mozilla source tree (no need to edit many existing files), so I can integrate everything into the source tree and safely continue working on the github repo with regular, easy files sync between both repo.

Comment 49

[Wesley Johnston (:wesj)]

Sharing resources is a bit hard. I actually tried to write this so that we didn't have to share ANY resources initially, and we could try to continue that here. I think we might need a few strings for "Hue" "Sat" etc. There may be a few integers describing some font sizes. We can hardcode those in the files (but we'll have to be careful to convert them from dip to pixels in code).

Alternatively, we are pulling in resources for the v7 via the patches in:

https://bugzilla.mozilla.org/show_bug.cgi?id=1006158

if you want an example how that can be done. Or we can just put everything in our normal mobile/android/base/widget and mobile/android/base/res directories and sidestep thirdparty (but that will require changes to the files, which isn't great :()

Comment 50

[Arnaud Bienner]

(In reply to Wesley Johnston (:wesj) from comment #45)
> I think I forced off hardware acceleration in the github project
> as well, because it broke things, so it would be good to test that.

As hardware acceleration is activated on Firefox, I faced this problem when trying to integrate everything into the source tree.

Do you remember what was wrong? (maybe how you figured out this disabling hardware acceleration would help) Or do you have any clue why it's not working?

If you don't remember, nevermind. I'm asking just in case, as this will probably save me some time :)

Comment 51

[Arnaud Bienner]

Looks like the multiples calls to setLayerType(View.LAYER_TYPE_SOFTWARE/HARWARE) creates the problem. Mainly this one [1].
Without these calls to setLayer, everything works as expected (with both hardware acceleration enabled or disabled), so I'm wondering why you needed this.

Not clearing the needinfo request in case you did this for a specific reason I'm missing.
If that doesn't ring a bell, I believe it would be safe to remove this setLayer thing.

[1]: https://github.com/ArnaudBienner/WheelColorPicker/blob/master/MyApplication2/src/main/java/com/example/myapplication2/Wheel.java#L315

Comment 52

[Wesley Johnston (:wesj)]

Great! Thanks for looking into it. The multiple calls were probably just me trying to optimize performance at some point. Removing them seems like a good idea :)

Comment 53

[Arnaud Bienner]

Attachment: android_advanced_colorpicker.patch

Advanced color picker from Github integrated into the tree.
Accessible from a new "Advanced" button in the basic color picker, like in http://cl.ly/image/392j0d242B0v/o

I will probably try to polish the code a bit more, but I believe it is functional now, so it's probably the time to ask for some feedback.

TODO:
- I'm not sure if the "android:text" field in layout resources are i18n. If not, some work needed here (I believe we might want to make "HSV:" and "%" translatable)
- We propose "HSV" to the user, but the code contains references to "RGB" as well. I guess this might have been done in a first step. I'm wondering if we should clean this, or we should keep this, in case we want to propose different selection mode to the user (i.e. HSV or RGB, not only HSV).

Next things are non blocking things IMHO, which can be done in future steps and that shouldn't prevent patch to be integrated:
- Change Hue slider as proposed in https://people.mozilla.org/~alam/Shots/fx_android_htmlcolorp4.png (even if I'm not sure it's better
- Wheel that didn't pan kinetically

Comment 54

[Arnaud Bienner]

Comment on attachment 8440289 [details] [diff] [review]
android_advanced_colorpicker.patch

Lucas, could you please have a look to this patch? Wesley suggested you might be interested in reviewing this in comment 45.
As mentioned in my previous comment, everything isn't perfect yet, but I would like to have some feedback before continuing. That being said, everything builds and works well for me, so you can try to play with it if you want.

Comment 55

[Wesley Johnston (:wesj)]

In my code (I think) tapping the "HSV" label would flip to "RGB" mode.

Comment 56

[Arnaud Bienner]

(In reply to Wesley Johnston (:wesj) from comment #55)
> In my code (I think) tapping the "HSV" label would flip to "RGB" mode.

Ah indeed, you're right.
I didn't touched that part too much.
Just tested in my fennec build and it's still working.
This is not very obvious though IMHO. I don't know how to make this more visible. Will think about it (this can be improved in the second step anyway).

Comment 57

[Lucas Rocha (:lucasr)]

Comment on attachment 8440289 [details] [diff] [review]
android_advanced_colorpicker.patch

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

Had a quick look. Didn't see any serious issues with the patch. It needs a thorough round of cleanups and general tidying up.

::: mobile/android/base/resources/layout/advanced_color_picker.xml
@@ +29,5 @@
> +                    android:paddingRight="0dp"/>
> +
> +                <View style="@style/SeekSpacer"/>
> +
> +                <org.mozilla.gecko.widget.SeekBar android:id="@+id/satSeekbar"

nit: align attributes consistently.

::: mobile/android/base/resources/values/styles.xml
@@ +719,5 @@
> +        <item name="android:layout_weight">1</item>
> +        <item name="android:layout_width">match_parent</item>
> +    </style>
> +
> +    <style name="Edit">

Use more Wheel-specific names for these styles. They look a bit too generic.

::: mobile/android/base/widget/Animator.java
@@ +7,5 @@
> +
> +/**
> + * Abstract class to animate a particular set of values on a view.
> + */
> +abstract class Animator<T> {

We already have code for doing animations. Have a look at PropertyAnimator.java.

::: mobile/android/base/widget/SeekBar.java
@@ +9,5 @@
> +import android.graphics.drawable.Drawable;
> +import android.graphics.drawable.ShapeDrawable;
> +import android.util.AttributeSet;
> +
> +public class SeekBar extends android.widget.SeekBar {

nit: import SeekBar directly.

@@ +50,5 @@
> +    public void setGradientColors(int[] colors) {
> +        mGradientColors = colors;
> +        updateGradient();
> +    }
> +    

nit: remove trailing spaces.

@@ +70,5 @@
> +        setGrad(getBackground(), mBackgroundColors);
> +        invalidate();
> +    }
> +
> +    private void setGrad(Drawable d, int[] colors) {

setGradient?

::: mobile/android/base/widget/Slice.java
@@ +23,5 @@
> + */
> +public class Slice extends View {
> +
> +    private static final String LOGTAG = "Slice";
> +    private static Paint mPaint = new Paint();

Where is mPaint actually used?

@@ +80,5 @@
> +
> +        mPaint.setColor(color);
> +    }
> +
> +    public void setParams(float angle, float r, float offset) {

angle and r are not used. "setParams" is a bit too cryptic. Be more specific.

::: mobile/android/base/widget/Wheel.java
@@ +16,5 @@
> +
> +/**
> + * View group that lays out items in a circle. Dragging on the view will rotate the items around the circle
> + */
> [e-mail]

Why RemoteView?

@@ +216,5 @@
> +        setupRadius(w, h);
> +
> +        mSelectedAngle = (int) (Math.atan((mCy-0.5)/(mCx-0.5))*-180/Math.PI);
> +        while (mSelectedAngle < 0) mSelectedAngle += 360;
> +        if (mCx > 0.5) mSelectedAngle += 180;

nit: brace all if's, even if they have only one line.

Comment 58

[Arnaud Bienner]

Thanks for your feedback :)
I made the changes you suggested.
However, few things aren't clear to me (please see my comments below).

(In reply to Lucas Rocha (:lucasr) from comment #57)
> ::: mobile/android/base/resources/layout/advanced_color_picker.xml
> @@ +29,5 @@
> > +                    android:paddingRight="0dp"/>
> > +
> > +                <View style="@style/SeekSpacer"/>
> > +
> > +                <org.mozilla.gecko.widget.SeekBar android:id="@+id/satSeekbar"
> 
> nit: align attributes consistently.

Not sure what you meant: everything looks aligned to me here.

> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ +719,5 @@
> > +        <item name="android:layout_weight">1</item>
> > +        <item name="android:layout_width">match_parent</item>
> > +    </style>
> > +
> > +    <style name="Edit">
> 
> Use more Wheel-specific names for these styles. They look a bit too generic.

Actually, this style isn't used in the Wheel, but in the textfields to select the RGB or HSV values directly.
If we want something less generic, maybe we should name this "AdvancedColorPickerEdit" (probably too long) or (simpler) "ColorValueEdit" (but except the fact it is used in the AdvancedColorPicker widget, it has nothing specific to color selection).
So not sure what can be a better name.

> We already have code for doing animations. Have a look at
> PropertyAnimator.java. 

OK, I will :)

> > +public class SeekBar extends android.widget.SeekBar {
> 
> nit: import SeekBar directly.

So we use a mSeekBar attribute instead? Is there any benefits about this/anything wrong with creating a new class that inherits android.widget.SeekBar?
Or you worried about name conflicts? In that case we might rename this class to something like "GradientSeekBar"?


> > +
> > +    public void setParams(float angle, float r, float offset) {
> 
> angle and r are not used. "setParams" is a bit too cryptic. Be more specific.

I had a look and it looks like we don't need this, as well as other things in this class, so I made some cleanup.

Comment 59

[Arnaud Bienner]

(In reply to Lucas Rocha (:lucasr) from comment #57)
> ::: mobile/android/base/widget/Wheel.java
> @@ +16,5 @@
> > +
> > +/**
> > + * View group that lays out items in a circle. Dragging on the view will rotate the items around the circle
> > + */
> > [e-mail]
> 
> Why RemoteView?

Wesley, do you remember why we need RemoteView here?

Comment 60

[Wesley Johnston (:wesj)]

Not needed. I think I just copy-pasted from ViewGroup to start and forgot to remove that:

http://developer.android.com/reference/android/view/ViewGroup.html

Comment 61

[Arnaud Bienner]

needinfo as I'm not sure so you saw my questions in comment 58, and so you don't forgot to have a look at them, when you have time.

Comment 62

[Lucas Rocha (:lucasr)]

My apologies for letting this slip for so long! I'll let wesj handle the review here.

Comment 63

[Arnaud Bienner]

I haven't worked on this again since a long time.
wesj, any chance you can finish this?
I tried to fix as many things as I can, but it's a bit hard for me to polish everything as I'm not used to write Android code. Also I don't have a working dev environment atm.
But still, I would be glad to see this very good color picker landed in Firefox for Android :)
I've submitted a pull request on github, to be sure what I've done isn't lost.

Comment 64

[Wesley Johnston (:wesj)]

No problem :) I'll try to get through your PR today regardless. No need to let such awesome things rot, and thanks for the work! I think we could land this without fixing everything (i.e. without kinetic scrolling or something...) but maybe I'll get a build up for UX to try.

I'm also a bit torn on this because Material design has some (really basic looking, which fits with the design [1]) color pickers screenshots at least (I don't think they ship). They seem more in line with the platform and a whole lot easier to ship. I think we might be better doing that. Anthony, you have any opinions?

[1] http://material-design.storage.googleapis.com/publish/v_2/material_ext_publish/0Bx4BSt6jniD7dDFpcFo0V2JLS2c/components_sliders_continuous5.png

Comment 65

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I agree - I think this design has since become outdated. I think the one provided by Android now actually covers most use cases for a default color picker quite well. Let's stick to what the platform offers for this.

Comment 66

[David Lawrence [:dkl]]

Moving all open Core::Widget: Android bugs to GeckoView::General (then the triage owner of GeckoView will decide which ones are valuable and which ones should be closed).

Comment 67

[Andrey Bienkowski]

https://bugzilla.mozilla.org/show_bug.cgi?id=1548706

Comment 68

[Makoto Kato [:m_kato]]

This issue should be closed since GV doesn't implement color picker. GV delegates color picker to Web Browser (https://github.com/mozilla-mobile/android-components/issues/1469). If UI issue, please file it to https://github.com/mozilla-mobile/fenix

Comment 69

[Chris Peterson [:cpeterson]]

Moving some input bugs to the new GeckoView::IME component.