507186 - checkbox inside a prefwindow doesn't affect the preference when it has a command

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[ffbr]

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]

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]]

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]

Attachment: 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]

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]

*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]

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][[e-mail]]]

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]

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]

Attachment: 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]

Attachment: patch with test

Comment 11

[[e-mail]]

(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]

I guess it wouldn't be. Ignore that condition.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Someone can manually dispatch a command event, and in that case the event can implement some other
event interface.

Comment 14

[[e-mail]]

(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

[[e-mail]]

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][[e-mail]]]

(In reply to [e-mail] 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

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/db2ec1f8df57