298665 - (colopicker_popup) color picker is broken (popup doesn't close after selecting a color)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Julien Appert]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; fr; rv:1.8b2) Gecko/20050621 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Win98; fr; rv:1.8b2) Gecko/20050621 Firefox/1.0+

After selecting a color, colorpicker popup doesn't want to close...

Reproducible: Always

Steps to Reproduce:
1. click the colopicker button
2. select a color in the colorpicker popup
3. note the bug

Actual Results:  
nothing

Expected Results:  
Colorpicker popup should close

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This was due to Ben's new prefwindow changes.

Comment 2

[Alan Knowles]

Anyone got any hints on where to look in the code to fix this - it's kind of critical for any XUL apps using the colorpiker 1.5

Comment 3

[:Gavin Sharp [email: [e-mail]]]

http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/colorpicker.xml is the color picker implementation. Using Venkman and the JS Console to see what's going on would be a good start.

Comment 4

[Alan Knowles]

This seems to fix it..


--- toolkit.jar.unzipped/content/global/bindings/colorpicker.xml        2005-08-22 17:01:30.000000000 +0800
+++ /tmp/colorpicker.xml        2005-11-19 11:32:01.000000000 +0800
@@ -434,7 +434,9 @@
         <xul:popup class="colorpicker-button-menupopup" anonid="colorpopup"
                    onmousedown="event.stopPropagation()"
                    onpopupshowing="this._colorPicker.onPopupShowing()"
-                   onpopuphiding="this._colorPicker.onPopupHiding()">
+                   onselect="this._colorPicker.pickerChange();"
+                   onpopuphiding="this._colorPicker.onPopupHiding()"
+                  >
            <xul:colorpicker xbl:inherits="palettename,disabled" allowevents="true" anonid="colorpicker"/>
          </xul:popup>
        </xul:popupset>

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Sure, that looks right. Could you attach a cvs diff -up8 patch? Ask for review (r?) from [e-mail].

Comment 6

[Alan Knowles]

Attachment: Proposed Fix - removes select event, adds onselect attribute

Comment 7

[Alan Knowles]

how do we ask for review? - just add [e-mail] to the cc? or send them a direct email, with a link to the bug + patch.?

Regards
Alan

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Using the "Edit" link on the attachment, set the "first-review" flag to "?" and enter the address in the "Requestee" field.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 203763 [details] [diff] [review]
Proposed Fix - removes select event, adds onselect attribute

Any commnets on why doesn't the current handler work?

Comment 10

[Alan Knowles]

As far as I can tell,

the scoping is wrong on the handler
- the hander is added to the xbl bindings for the colorpicker button, however, the "select" event (from _fireEvent) is sent to the popup element inside the colorpicker button. 

I'm guessing that since popup already has a onselect event handler, by default, that it is not propegated to the parent (eg. the xbl bindings), hence never reaches the select handler for the xbl, and never fires off the pickerChange() call.

(sorry had to add  asaf to the cc's as I was not sure you would get a copy)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Ah, no, I see the reason this is failing. Events usually bubble up through the DOM, but the synthetic event from _fireEvent has canBubble set to false... see:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/colorpicker.xml&rev=1.11&mark=215#208
and:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/public/idl/events/nsIDOMEvent.idl&rev=1.8&mark=172#145

So just changing the second argument in that initEvent call should fix it... though I've not looked into whether that was intentional for some other reason.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(and that method issues other events, so other callers would have to be verified to ensure that changing it won't break anything)

Comment 13

[:Gavin Sharp [email: [e-mail]]]

for reference:
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF_FRAMESET&subdir=mozilla%2Ftoolkit%2Fcontent%2Fwidgets&file=colorpicker.xml&rev1=1.7&rev2=1.8

Comment 14

[Asaf Romano (gone)]

Comment on attachment 203763 [details] [diff] [review]
Proposed Fix - removes select event, adds onselect attribute

This isn't the only case where "fireEvent" doesn't bubble, I would like to sort this out separately (please file a bug and cc me), so this will do for now.

Comment 15

[Martijn Wargers (dead)]

*** Bug 323251 has been marked as a duplicate of this bug. ***

Comment 16

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/widgets/colorpicker.xml; new revision: 1.12;

Checked in on the trunk. Thanks for the patch Alan.

Comment 17

[Ian Neal]

Just wonder why it wasn't done the same way as the xpfe version of this file, with the onselect being on the actual colorpicker itself.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 203763 [details] [diff] [review]
Proposed Fix - removes select event, adds onselect attribute

(In reply to comment #17)
> Just wonder why it wasn't done the same way as the xpfe version of this file,
> with the onselect being on the actual colorpicker itself.

For what it's worth, I think the two colorpickers are pretty different with regard to event handling. If there is a better way of doing this, could you please file a bug and CC me?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Fixed on the 1.8 branch for Firefox 2.
mozilla/toolkit/content/widgets/colorpicker.xml; new revision: 1.10.4.2;