387097 - Color picker in options window doesn't work after being selected once

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

From bug 279703 comment #95:
In Tools/Options/Contents/Colors, click on a colorpicker to open the colors
panel (with updated Walnut theme this looks nice!), and the click again on the
same colorpicker button to close it again. So far so good. Click again, and you
would expect to pop it again, but no ...  For other popups this works, so may
be the colorpicker needs some specific attention?

I can confirm this with XPSP2.

Comment 1

[Marco Bonardo [:mak]]

i get a slightly different behaviour...

clicking on the colorpicker button mantains it open, while it should close
1. click on color picker button
2. click again on it
the color picker should close instead it reopens on itself

clicking away closes it and it can be reopened without any problem

Comment 2

[Neil Deakin]

Attachment: Remove custom popup handling

The colorpicker uses some odd and hacky custom handling of the popup. Let's just use the builtin menu behaviour.

Comment 3

[Asaf Romano (gone)]

Comment on attachment 271246 [details] [diff] [review]
Remove custom popup handling

Having to put a <panel> inside a xul:menu element (meaningfully forces creating a binding for simple panels) seems sub-optimal to me :-/


>Index: toolkit/content/widgets/colorpicker.xml
>===================================================================

>-      <property name="open" onget="return this.mOpen"/>
>+      <property name="open" onget="return this.getAttribute('open') == 'true'"/>

while you're at it, either define a setter or mark this readonly.

>     <handlers>
>       <handler event="keydown"><![CDATA[
>         // open popup if key is space/up/left/right/down and popup is closed
>-        if ( (event.keyCode == 32 || (event.keyCode > 36 && event.keyCode < 41)) && !this.mOpen)
>-          
>+        if ( (event.keyCode == 32 || (event.keyCode > 36 && event.keyCode < 41)) && !open)
>           this.showPopup();
>+        else if ( (event.keyCode == 27) && open)

for readability, please make this this.open/!this.open.

r=mano otherwise.

Comment 4

[Carsten Book [:Tomcat]]

verified using  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122705 Minefield/3.0b3pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122704 Minefield/3.0b3pre with the steps to reproduce from this bug

- Verified fixed