Closed Bug 507186 Opened 10 years ago Closed 8 years ago
checkbox inside a prefwindow doesn't affect the preference when it has a command
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) Gecko/2009070818 Ubuntu/8.10 (intrepid) Firefox/3.0.12 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124) 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.
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets
I also am experiencing this problem on a linux machine as well. Firefox 7.0 Linux 2.6.32-5-686
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?
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)
*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
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-
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
(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 firstname.lastname@example.org 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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.