<input type=color> has just shipped, we should consider using this as our color picker for the calendar and category colors.

Hi ,I am interested in solving this bug could you please help me out, As I am a newbie to mozilla

Hello saianirudh196. I have another contributor, lviknesh@gmail.com, lined up who would also like to work on this bug, maybe you could send each other email and figure out who will be providing the patch to avoid double work. There are definitely a few other nice bugs to look at if you don't mind switching.

Oh , Sorry for adding comment as patch name :(

Comment on attachment 8417377 [details] [diff] [review] Please tell me anymore changes are needed Review of attachment 8417377 [details] [diff] [review]: ----------------------------------------------------------------- Did you actually test the patch? It doesn't work for me, the color is not prefilled in the calendar properties dialog, nor is it updated when selected. Also, I am missing some CSS to make the button look a little more like it did before.

The prefill fails because in calendar-properties-dialog.js, |document.getElementById("calendar-color").color| has to be replaced with |document.getElementById("calendar-color").value|

This should take care :)

Comment on attachment 8561465 [details] [diff] [review] Fix - v2 Review of attachment 8561465 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/preferences/editCategory.js @@ +61,4 @@ > // Pretend the category name changed, this selects the color > categoryNameChanged(); > } else { > + document.getElementById("categoryColor").value = "transparent"; I don't know anything about Calendar's code but I worked on the implementation of input type color on Gecko. And I remember the specs says the value should be a 6digit hexa RGB (no alpha), and we implemented it this way, so I'm afraid this will not work. As "transparent" is an invalid value, sanitization algorithm will fail to recognize it and set value to default (i.e. "#000000") Also, as I said, this is no support for transparency in current definition of input type color, so I'm not sure how you can achieve the same effect, if you really need to.

Thanks for the comment. I believe we were just using this as a "null" value before. I'll check this again and just use #000000 or maybe a gray color instead, which should be fine.

I'm just using white now, which looks slightly than black better on mac. It is indeed just a placeholder.

Comment on attachment 8563309 [details] [diff] [review] Fix - v3 The patch works fine, the only little flaw I've found is that when opening the properties dialog about the default calendar ("Home"), the button that allows to change the color shows the picker's default color "Black" instead of the color of the Home calendar (#A8C2E1). This happens only with the default calendar and not with new calendars created after the first run. Is it possible to make picker's default color different than black? > I'm just using white now, which looks slightly than black better on mac. It > is indeed just a placeholder Yes, white is better that black for the purpose, another possibility might be to hide the button when the checkbox is not checked but I don't think it worths the effort in a case like this. It's a pity that the new color picker doesn't maintain the customized colors through the sessions.

(In reply to Decathlon from comment #11) > Comment on attachment 8563309 [details] [diff] [review] > Fix - v3 > > The patch works fine, the only little flaw I've found is that when opening > the properties dialog about the default calendar ("Home"), the button that > allows to change the color shows the picker's default color "Black" instead > of the color of the Home calendar (#A8C2E1). This happens only with the > default calendar and not with new calendars created after the first run. > > Is it possible to make picker's default color different than black? Ah yes, this happens when the calendar color is null. I just have to check for that and set #A8C2E1 instead. > > > I'm just using white now, which looks slightly than black better on mac. It > > is indeed just a placeholder > > Yes, white is better that black for the purpose, another possibility might > be to hide the button when the checkbox is not checked but I don't think it > worths the effort in a case like this. > > It's a pity that the new color picker doesn't maintain the customized colors > through the sessions. That color is only shown when the checkbox is disabled, aside from that it should be retaining the colors?

(In reply to Philipp Kewisch [:Fallen] from comment #12) > That color is only shown when the checkbox is disabled, aside from that it > should be retaining the colors? Currently it doesn't indeed. But it's unclear to me what it should do. I initially though this part (custom colors) might be filled with colors from list or autocomplete attribute (bug 960984 and bug 960989 which haven't been implemented yet). But indeed, it's not filled currently: it is just set to black in static array, so it will be all black again after restarting Firefox [1]. If you have an idea how this "custom colors" section can be used, please let me know. Maybe we should just forget about using it from "autocomplete" or "list", as not all native color pickers have a "custom colors" section. And in platforms which have one, save/restore the last values added by the user instead (i.e. will be global to all color pickers opened within Firefox). [1]: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsColorPicker.cpp#109

(In reply to Arnaud Bienner from comment #13) > (In reply to Philipp Kewisch [:Fallen] from comment #12) > > That color is only shown when the checkbox is disabled, aside from that it > > should be retaining the colors? > > Currently it doesn't indeed. > But it's unclear to me what it should do. I think I'm just misunderstanding something here. With the null color issue fixed, the patch should be behaving as expected, no more work needed. The dialog looks like this, where [ ##### ] is the html:colorpicker: [ ] Use Color: [ ##### ] * When the checkbox is ticked, the color is set to whatever color the category should be * When the checkbox is unticked, the color is reset to white and the color isn't used when accepting * When ticking the checkbox after it was previously unticked, the previously used color is restored. * When clicking on the colorpicker when the checkbox is unticked, the checkbox is ticked and the colorpicker is opened (this is the reason I didn't just disable the colorpicker) We don't need a list of colors right now, I think we are fine with the user picking any color he likes.

Ok, so I've improved the behavior in the edit category dialog a bit. The code isn't as simple as before, but it does give us kind of what we had before, but with the much more advanced color picker. The color picker is turned into a button with the same style. Clicking it turns it into a colorpicker again and enables the checkbox. For the calendar properties dialog, the color should now be set correctly when opening the dialog on a calendar without a defined color. For the new calendar wizard, I am setting our good ol' default #a8c2e1 on the calendar, essentially fixing bug 392173. We could eventually generalize the color generation UI from the edit category dialog into a binding or mixin so its usable from the calendar creation dialog and maybe other places, but this bug has already had enough iterations :) Let me know how this feels.

Comment on attachment 8564401 [details] [diff] [review] Fix - v4 Review of attachment 8564401 [details] [diff] [review]: ----------------------------------------------------------------- Sorry Philipp, I found a pair of issues and at least the first of them must be solved. ::: calendar/base/content/preferences/editCategory.js @@ +51,2 @@ > // Color is wanted, choose the color based on the category name's hash. > + document.getElementById("categoryColor").value = cal.hashColor(newValue); This causes a problem when you add a *new* category: when typing the category's name, in the picker appears a label with the colors code (#xxyyzz) instead of the real color. The problem disappear if you set the attribute "type" to "color" before give the picker a color. Another issue is that when you finish to type the name of the *new* category, the color in the picker (visible after the correction mentioned above) becomes always orange just after ticking the checkbox. It seems the variable "lastColor" gets the value #FF6600 (I'm not able to figure out why) in the first run and never changes. As far as I can see the problem could be solved by avoided the assignment of "toggleColor.lastColor" inside the "function toggleColor()" when this function is being called by the Load Handler "editCategoryLoad()" (i.e. on the dialog opening). I passed, only from there, to the function a boolean parameter that allow to skip the follow line toggleColor.lastColor = categoryColor.value; and it seems working well, but maybe there are others solutions as well.

(In reply to Decathlon from comment #17) > Sorry Philipp, I found a pair of issues and at least the first of them must > be solved. No need to feel sorry, better to find these issues before they are pushed :) > ::: calendar/base/content/preferences/editCategory.js > @@ +51,2 @@ > > // Color is wanted, choose the color based on the category name's hash. > > + document.getElementById("categoryColor").value = cal.hashColor(newValue); > > This causes a problem when you add a *new* category: when typing the > category's name, in the picker appears a label with the colors code > (#xxyyzz) instead of the real color. > The problem disappear if you set the attribute "type" to "color" before give > the picker a color. Good catch. I've fixed this by making sure the color value is only set when useColor is checked in the categoryNameChanged function. > Another issue is that when you finish to type the name of the *new* > category, the color in the picker (visible after the correction mentioned > above) becomes always orange just after ticking the checkbox. That orange is the color that hashColor gets out of an empty string. I've fixed this by calling categoryNameChanged() after the useColor checkbox is checked. Here is a new patch, hope it works better.

Comment on attachment 8564943 [details] [diff] [review] Fix - v5 It works fine for me. r+ There are two missing semicolon, here: >+function toggleColor() { >+ let useColor = document.getElementById('useColor').checked; >+ let categoryColor = document.getElementById('categoryColor') and here: >+function clickColor() { >+ let categoryColor = document.getElementById('categoryColor')

Thanks, here is an updated patch with those semicolons added.

Pushed as http://hg.mozilla.org/comm-central/rev/ab857f2c7a98