Closed
Bug 298665
(colopicker_popup)
Opened 19 years ago
Closed 19 years ago
color picker is broken (popup doesn't close after selecting a color)
Categories
(Toolkit :: XUL Widgets, defect)
Toolkit
XUL Widgets
Tracking
()
RESOLVED
FIXED
mozilla1.8.1
People
(Reporter: appertj, Assigned: alan)
References
()
Details
(Keywords: fixed1.8.1, regression)
Attachments
(1 file)
(deleted),
patch
|
asaf
:
first-review+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•19 years ago
|
Alias: colopicker_popup
Comment 1•19 years ago
|
||
This was due to Ben's new prefwindow changes.
Blocks: 297286
Status: UNCONFIRMED → NEW
Component: Disability Access → XUL Widgets
Ever confirmed: true
OS: Windows 98 → All
Product: Firefox → Toolkit
QA Contact: disability.access → xul.widgets
Hardware: PC → All
Summary: after selecting a color, colorpicker popup doesn't want to close... → color picker is broken (popup doesn't close after selecting a color)
Updated•19 years ago
|
Keywords: regression
Assignee | ||
Comment 2•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
||
Sure, that looks right. Could you attach a cvs diff -up8 patch? Ask for review (r?) from bugs.mano@sent.com.
Assignee | ||
Comment 6•19 years ago
|
||
Assignee | ||
Comment 7•19 years ago
|
||
how do we ask for review? - just add bugs.mano@sent.com to the cc? or send them a direct email, with a link to the bug + patch.? Regards Alan
Comment 8•19 years ago
|
||
Using the "Edit" link on the attachment, set the "first-review" flag to "?" and enter the address in the "Requestee" field.
Assignee | ||
Updated•19 years ago
|
Attachment #203763 -
Flags: first-review?(bugs.mano)
Comment 9•19 years ago
|
||
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?
Assignee | ||
Comment 10•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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
Version: unspecified → Trunk
Comment 14•19 years ago
|
||
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.
Attachment #203763 -
Flags: first-review?(bugs.mano) → first-review+
Updated•19 years ago
|
Whiteboard: [checkin needed?]
Comment 15•19 years ago
|
||
*** Bug 323251 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Assignee: nobody → alan
Comment 16•19 years ago
|
||
mozilla/toolkit/content/widgets/colorpicker.xml; new revision: 1.12; Checked in on the trunk. Thanks for the patch Alan.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed?]
Target Milestone: --- → mozilla1.9alpha1
Comment 17•19 years ago
|
||
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•19 years ago
|
||
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?
Attachment #203763 -
Flags: branch-1.8.1?(mconnor)
Updated•19 years ago
|
Attachment #203763 -
Flags: branch-1.8.1?(mconnor) → branch-1.8.1+
Comment 19•19 years ago
|
||
Fixed on the 1.8 branch for Firefox 2. mozilla/toolkit/content/widgets/colorpicker.xml; new revision: 1.10.4.2;
Keywords: fixed1.8.1
Target Milestone: mozilla1.9alpha1 → mozilla1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•