Last Comment Bug 507186 - checkbox inside a prefwindow doesn't affect the preference when it has a command
: checkbox inside a prefwindow doesn't affect the preference when it has a command
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla9
Assigned To: Neil Deakin
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-29 10:59 PDT by ffbr
Modified: 2011-09-07 07:28 PDT (History)
6 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
We need to take out the early return on line 1698 (1010 bytes, patch)
2011-08-22 18:30 PDT, Ed
enndeakin: feedback-
Details | Diff | Splinter Review
patch (1.16 KB, patch)
2011-09-02 08:31 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
patch with test (5.75 KB, patch)
2011-09-02 15:14 PDT, Neil Deakin
neil: review+
Details | Diff | Splinter Review

Description ffbr 2009-07-29 10:59:54 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.12) Gecko/2009070818 Ubuntu/8.10 (intrepid) Firefox/3.0.12
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.12) Gecko/2009070818 Ubuntu/8.10 (intrepid) Firefox/3.0.12

i have a prefwindow called with openDialog. When a checkbox is changed, the command fires as expected. however, it no longer affects the preference it's for. removing the command attribute restores functionality of changing the prference. this may be somewhat related to: 206497

    <commandset>

      <command id="cmd_foo" oncommand="alert('foo');"/>

    </commandset>


    <checkbox id="addonname-pref1" preference="pref_pref1" label="&addonname.check.label;" command="cmd_foo" />



Reproducible: Always

Steps to Reproduce:
1. create a simple xul file for a prefwindow, set up test preference
2. inside the xul file add a checkbox, with commandset and add command attribute to the checkbox
3. notice the checkbox value doesn't affect the preference. remove command to checkbox and it will now change the preference.
Comment 1 Ed 2011-08-21 13:12:31 PDT
I also am experiencing this problem on a linux machine as well.  Firefox 7.0 Linux 2.6.32-5-686
Comment 2 Josh Matthews [:jdm] 2011-08-21 21:38:29 PDT
I've suggested that Ed try removing the early-return from nsXULElement::PreHandleEvent in the case of an NS_XUL_COMMAND event, so we would fall through to nsStyledElement::PreHandleEvent after dispatching the XUL command. Neil, does this sound like a safe solution?
Comment 3 Ed 2011-08-22 18:30:45 PDT
Created attachment 555008 [details] [diff] [review]
We need to take out the early return on line 1698

This is my very first patch to an open source project.  Please be gentle!  I am not sure it is clear what file I am submitting this patch in.  For reference its: http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp
Comment 4 Neil Deakin 2011-08-22 18:38:41 PDT
Comment on attachment 555008 [details] [diff] [review]
We need to take out the early return on line 1698

Ed, I'll take a look at this patch tomorrow.

When submitting a patch that you'd like feedback on or to be reviewed, set the 'review' or 'feedback' flag to '?' then enter an email address of the person you'd like feedback from. Unfortunately, determining who that person is isn't easy as https://wiki.mozilla.org/Modules/All isn't particularly accurate.
Comment 5 Ed 2011-08-30 14:43:41 PDT
*bump*

I read on slashdot the other day that mozilla is overrun with UNCONFIRMED bugs.  I hope I'm not stuck in this mess.
Comment 6 Neil Deakin 2011-09-02 06:01:08 PDT
Olli, what side effects does nsStyledElement::PreHandleEvent have?

Essentially what happens here is that the command event is being refired on the command element.
Comment 7 Olli Pettay [:smaug] 2011-09-02 06:44:51 PDT
Well, there wouldn't be side effects but the event would be just dispatched to the original event
target right after dispatching command event to command element.

The current behavior is essentially mix of Bug 197152 and Bug 336696.
If the behavior was changed, I wouldn't be surprised if there were some regressions.
Comment 8 Neil Deakin 2011-09-02 08:24:37 PDT
Comment on attachment 555008 [details] [diff] [review]
We need to take out the early return on line 1698

OK, so if this fires the event on both elements, that isn't going to work.

What we seem to want is a way for the preference updating code to run before the event is redirected, or to run anyway, but I'm not sure what mechanism could be used for that.
Comment 9 Neil Deakin 2011-09-02 08:31:42 PDT
Created attachment 557841 [details] [diff] [review]
patch

What we can do though, is get the original target from the command event and use that to get the preference instead.
Comment 10 Neil Deakin 2011-09-02 15:14:27 PDT
Created attachment 557958 [details] [diff] [review]
patch with test
Comment 11 neil@parkwaycc.co.uk 2011-09-02 16:00:08 PDT
(From update of attachment 557958 [details] [diff] [review])
>+        if (event instanceof XULCommandEvent && event.sourceEvent)
When would a command event not be a XULCommandEvent?
Comment 12 Neil Deakin 2011-09-02 17:18:59 PDT
I guess it wouldn't be. Ignore that condition.
Comment 13 Olli Pettay [:smaug] 2011-09-03 03:01:17 PDT
Someone can manually dispatch a command event, and in that case the event can implement some other
event interface.
Comment 14 neil@parkwaycc.co.uk 2011-09-04 07:47:44 PDT
(In reply to Olli Pettay from comment #13)
> Someone can manually dispatch a command event, and in that case the event
> can implement some other event interface.
Well they'll have trouble getting a command event listener to listen to it; events created with document.createEvent("XULCommandEvent") will work, while events created with document.createEvent("Event") won't.
[Random side note: Why aren't event types constructors? new XULCommandEvent("command", ...) would me much saner.]
Comment 15 neil@parkwaycc.co.uk 2011-09-04 07:53:38 PDT
Comment on attachment 557958 [details] [diff] [review]
patch with test

r=me with the XULCommandEvent check removed. (You may or may not want to remove the CDATA lines too, now that there are no & characters to quote.)
Comment 16 Olli Pettay [:smaug] 2011-09-04 08:04:13 PDT
(In reply to neil@parkwaycc.co.uk from comment #14)
> [Random side note: Why aren't event types constructors? new
> XULCommandEvent("command", ...) would me much saner.]
They are per latest DOM Core draft.
Comment 17 :Ehsan Akhgari (away Aug 1-5) 2011-09-07 07:16:35 PDT
http://hg.mozilla.org/mozilla-central/rev/db2ec1f8df57

Note You need to log in before you can comment on or make changes to this bug.