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

RESOLVED FIXED in mozilla9

Status

()

Core
XUL
--
major
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: ffbr, Assigned: Neil Deakin (mostly unavailable until September))

Tracking

unspecified
mozilla9
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.

Updated

8 years ago
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets

Comment 1

6 years ago
I also am experiencing this problem on a linux machine as well.  Firefox 7.0 Linux 2.6.32-5-686

Comment 2

6 years ago
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

6 years ago
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
Attachment #555008 - Flags: checkin+
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.
Attachment #555008 - Flags: checkin+ → feedback?(enndeakin)

Comment 5

6 years ago
*bump*

I read on slashdot the other day that mozilla is overrun with UNCONFIRMED bugs.  I hope I'm not stuck in this mess.
Olli, what side effects does nsStyledElement::PreHandleEvent have?

Essentially what happens here is that the command event is being refired on the command element.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All

Comment 7

6 years ago
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 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.
Attachment #555008 - Flags: feedback?(enndeakin) → feedback-
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.
Assignee: nobody → enndeakin
Attachment #555008 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Created attachment 557958 [details] [diff] [review]
patch with test
Attachment #557841 - Attachment is obsolete: true
Attachment #557958 - Flags: review?(neil)
(From update of attachment 557958 [details] [diff] [review])
>+        if (event instanceof XULCommandEvent && event.sourceEvent)
When would a command event not be a XULCommandEvent?
I guess it wouldn't be. Ignore that condition.
Someone can manually dispatch a command event, and in that case the event can implement some other
event interface.
(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 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.)
Attachment #557958 - Flags: review?(neil) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/db2ec1f8df57
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.