Fix the issue to allow a custom color palette for the popup color picker....

Fix the issue to allow a custom color palette for the popup color picker.

If we pass a color picker through the popup color picker's constructor, it doesn't add itself as a child to the popup nor renders itself. I have no idea if this is the right way to do this, since first time contributing to closure-library. But it works in the use case I explained in the relevant thread. http://goo.gl/lsDo

BUG=none
TEST=ran closure/goog/ui/popupcolorpicker_test.html

Patch by Mohamed Mansour <hello@mohamedmansour.com>

[MohamedMansour, 2010-06-09 05:28:18]

Hi Nathan, its Mohamed from Google I/O. Let me know if this patch is correct.
Thanks!

[nnaze, 2010-06-09 18:57:42]

On 2010/06/09 05:28:18, MohamedMansour wrote:
> Hi Nathan, its Mohamed from Google I/O. Let me know if this patch is correct.
> Thanks!

Relevant thread:
https://groups.google.com/group/closure-library-discuss/browse_thread/thread/...

[dan.pupius, 2010-06-09 19:21:54]

Thanks for picking this up.

http://codereview.appspot.com/1630041/diff/1/3
File closure/goog/ui/popupcolorpicker.js (right):

http://codereview.appspot.com/1630041/diff/1/3#newcode45
closure/goog/ui/popupcolorpicker.js:45: goog.ui.PopupColorPicker =
function(opt_domHelper, opt_colorPicker) {
Would be great if you could add a test case exercising this.

http://codereview.appspot.com/1630041/diff/1/3#newcode49
closure/goog/ui/popupcolorpicker.js:49: this.customColorPicker_ =
opt_colorPicker;
Can you revert this line.  I'd prefer to keep an initialized_ flag.

http://codereview.appspot.com/1630041/diff/1/3#newcode59
closure/goog/ui/popupcolorpicker.js:59:
goog.ui.PopupColorPicker.prototype.customColorPicker_ = null;
change to boolean initialized_

http://codereview.appspot.com/1630041/diff/1/3#newcode392
closure/goog/ui/popupcolorpicker.js:392: if (!this.colorPicker_) {
How about something like this:

if (!this.initialized_) {
  this.colorPicker_ = this.colorPicker_ ||
      goog.ui.ColorPicker.createSimpleColorGrid(this.getDomHelper());
  this.colorPicker_.setFocusable(this.focusable_);
  this.addChild(this.colorPicker_, true);
  this.getHandler().listen(this.colorPicker_,
      goog.ui.ColorPicker.EventType.CHANGE, this.onColorPicked_);
  this.initialized_ = true;
}

[MohamedMansour, 2010-06-09 20:01:31]

Thanks Dan, I have updated the patch, I have no idea if that is the way we do
the tests. Please be picky on the comments as well.

http://codereview.appspot.com/1630041/diff/1/3
File closure/goog/ui/popupcolorpicker.js (right):

http://codereview.appspot.com/1630041/diff/1/3#newcode45
closure/goog/ui/popupcolorpicker.js:45: goog.ui.PopupColorPicker =
function(opt_domHelper, opt_colorPicker) {
On 2010/06/09 19:21:54, dan.pupius wrote:
> Would be great if you could add a test case exercising this.

Done.

http://codereview.appspot.com/1630041/diff/1/3#newcode49
closure/goog/ui/popupcolorpicker.js:49: this.customColorPicker_ =
opt_colorPicker;
On 2010/06/09 19:21:54, dan.pupius wrote:
> Can you revert this line.  I'd prefer to keep an initialized_ flag.

Done.

http://codereview.appspot.com/1630041/diff/1/3#newcode59
closure/goog/ui/popupcolorpicker.js:59:
goog.ui.PopupColorPicker.prototype.customColorPicker_ = null;
On 2010/06/09 19:21:54, dan.pupius wrote:
> change to boolean initialized_

Done.

http://codereview.appspot.com/1630041/diff/1/3#newcode392
closure/goog/ui/popupcolorpicker.js:392: if (!this.colorPicker_) {
On 2010/06/09 19:21:54, dan.pupius wrote:
> How about something like this:
> 
> if (!this.initialized_) {
>   this.colorPicker_ = this.colorPicker_ ||
>       goog.ui.ColorPicker.createSimpleColorGrid(this.getDomHelper());
>   this.colorPicker_.setFocusable(this.focusable_);
>   this.addChild(this.colorPicker_, true);
>   this.getHandler().listen(this.colorPicker_,
>       goog.ui.ColorPicker.EventType.CHANGE, this.onColorPicked_);
>   this.initialized_ = true;
> }

Done.

[dan.pupius, 2010-06-09 20:09:26]

Thanks for making those changes.

http://codereview.appspot.com/1630041/diff/11001/12003
File closure/goog/ui/popupcolorpicker.js (right):

http://codereview.appspot.com/1630041/diff/11001/12003#newcode54
closure/goog/ui/popupcolorpicker.js:54: /**
2-line breaks between top level blocks.

http://codereview.appspot.com/1630041/diff/11001/12003#newcode397
closure/goog/ui/popupcolorpicker.js:397:
this.colorPicker_.render(this.getElement());
remove this line, add a 2nd argument 'true' to addChild.

It's a convenience that tells addChild to render.

http://codereview.appspot.com/1630041/diff/11001/12002
File closure/goog/ui/popupcolorpicker_test.html (right):

http://codereview.appspot.com/1630041/diff/11001/12002#newcode47
closure/goog/ui/popupcolorpicker_test.html:47:
assertNotNull(picker.getPopup().getElement());
You'll want to attach it to an element and simulate a click, see
goog.testing.events.  This will cause 'show_' to get called.  You can then
verify that your custom color picker was rendered and is a child of the picker.

[MohamedMansour, 2010-06-09 20:14:20]

Done, and tested. Thanks!

http://codereview.appspot.com/1630041/diff/11001/12003
File closure/goog/ui/popupcolorpicker.js (right):

http://codereview.appspot.com/1630041/diff/11001/12003#newcode54
closure/goog/ui/popupcolorpicker.js:54: /**
On 2010/06/09 20:09:26, dan.pupius wrote:
> 2-line breaks between top level blocks.

Done.

http://codereview.appspot.com/1630041/diff/11001/12003#newcode397
closure/goog/ui/popupcolorpicker.js:397:
this.colorPicker_.render(this.getElement());
On 2010/06/09 20:09:26, dan.pupius wrote:
> remove this line, add a 2nd argument 'true' to addChild.
> 
> It's a convenience that tells addChild to render.

Done. Cool, didn't know about that :x

[dan.pupius, 2010-06-09 20:48:12]

http://codereview.appspot.com/1630041/diff/15001/16002
File closure/goog/ui/popupcolorpicker_test.html (right):

http://codereview.appspot.com/1630041/diff/15001/16002#newcode43
closure/goog/ui/popupcolorpicker_test.html:43: colorPicker.setColors(new
Array("#ffffff", "#000000"));
colorPicker.setColors(['#ffffff', '#000000']);

http://codereview.appspot.com/1630041/diff/15001/16002#newcode47
closure/goog/ui/popupcolorpicker_test.html:47:
assertNotNull(picker.getPopup().getElement());
This comment seemed to get lost before, so I'm fine if this comes in a
subsequent change:

What would be great is if you attach the picker to an element and simulate a
click on it, see goog.testing.events.  This will call show_ and you can then
test that the picker has been correctly added to the DOM.

[MohamedMansour, 2010-06-09 22:45:48]

Thanks for the tips Dan! Is this more acceptable?

http://codereview.appspot.com/1630041/diff/15001/16002
File closure/goog/ui/popupcolorpicker_test.html (right):

http://codereview.appspot.com/1630041/diff/15001/16002#newcode43
closure/goog/ui/popupcolorpicker_test.html:43: colorPicker.setColors(new
Array("#ffffff", "#000000"));
On 2010/06/09 20:48:13, dan.pupius wrote:
> colorPicker.setColors(['#ffffff', '#000000']);

Done.

http://codereview.appspot.com/1630041/diff/15001/16002#newcode47
closure/goog/ui/popupcolorpicker_test.html:47:
assertNotNull(picker.getPopup().getElement());
On 2010/06/09 20:48:13, dan.pupius wrote:
> This comment seemed to get lost before, so I'm fine if this comes in a
> subsequent change:
> 
> What would be great is if you attach the picker to an element and simulate a
> click on it, see goog.testing.events.  This will call show_ and you can then
> test that the picker has been correctly added to the DOM.

Done.

[dan.pupius, 2010-06-09 23:06:28]

Sweet.  Looks Good To Me.

[MohamedMansour, 2010-06-10 01:40:09]

Cool! Nathan or Dan, feel free to commit it =)

[nnaze, 2010-06-10 22:26:14]

On 2010/06/10 01:40:09, MohamedMansour wrote:
> Cool! Nathan or Dan, feel free to commit it =)

Patch ingested:
http://code.google.com/p/closure-library/source/detail?r=148

Congrats! First one (of many)!

Please close this issue.

[erik.arvidsson, 2010-06-11 06:27:21]

http://codereview.appspot.com/1630041/diff/20001/21002
File closure/goog/ui/popupcolorpicker_test.html (right):

http://codereview.appspot.com/1630041/diff/20001/21002#newcode21
closure/goog/ui/popupcolorpicker_test.html:21: <button href="javascript:void(0)"
id="button1">color picker</button>
button does not have an href attribute.

[MohamedMansour, 2010-06-11 11:18:43]

errr :< sorry! I wanted it to be a <a> instead of <button> If someone could make
rename it to a link that would be great. I am leaving for three days
camping/worldcup.

[nick santos, 2010-06-14 22:51:34]

I have to rollback this change, because it is breaking the tests on IE.

Monsour, do you have IE available in order to compose a fix?

[MohamedMansour, 2010-06-14 22:57:35]

Yes I have IE, but which test is it breaking? When I tested this, the popup
tests were working fine on IE.

[nicholas.j.santos_gmail.com, 2010-06-14 23:00:03]

Our build farm was using ie7/vista. I haven't verified locally yet
with other versions of IE.

I'll also try to find some time to look at why it broke, but just
wanted to see if something obvious popped out at you.

[nicholas.j.santos_gmail.com, 2010-06-14 23:01:10]

for what it's worth, here was the error message:

IE7_VISTA  [FAILED]
    Test was retried 2 times
      Retried On:

        Run 1 took 3.9s  State: ERROR
        Ip: 192.168.11.100
        Closure Unit Tests - goog.ui.Popup [FAILED]
       
javascript/closure/ui/popupcolorpicker_test.html?jsunitfarmurl=http%3A%2F%2Fclosure-cb.hot.corp.google.com%3A50051%2Fgoogle3%2Fjavascript%2Fclosure%2Fui%2Fpopupcolorpicker_test.html&browser=ie7-vista&tid=293ea8fa-cddf-4820-94bc-8c6da79071dd
        3 of 3 tests run in 73ms.
        2 passed, 1 failed.
        24 ms/test. 64 files loaded.
        ERROR in testCustomColorPicker
        Can't move focus to the control because it is invisible, not
enabled, or of a type that does not accept the focus.

        Please RETRY

[MohamedMansour, 2010-06-14 23:08:01]

Thanks, strange, I will look into it right now.

[MohamedMansour, 2010-06-14 23:56:30]

While debugging, it works just fine. Without debugging, sometimes it works,
sometimes it doesn't. 

When the second color is selected is where it fails (without debug mode).

Something like this:
http://codereview.appspot.com/1676044/show