No event is fired after selecting an item in <xul:menulist>

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org), Assigned: Shilpan Bhagat)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 25
Points:
---

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The way SelectHelper is currently implemented it won't fire any event after selection in <xul:menulist> is finished. Steps to reproduce:

1. Install Adblock Plus 2.2.4 from https://addons.mozilla.org/en-US/firefox/addon/adblock-plus/versions/
2. Go to Adblock Plus options
3. Change selected filter subscription in the list.

Expected results:

Selected subscription changes and is remembered.

Actual results:

Selected subscription doesn't change, reloading the page sets it to the old value again.

The offending code is this: http://hg.mozilla.org/mozilla-central/file/04d8c309fe72/mobile/android/chrome/content/SelectHelper.js#l40. For nsIDOMXULMenuListElement it will merely set selectedIndex and not fire any event. XUL code expects a command event upon selection.

Current work-around is watching the selectedIndex property which is everything but great: https://hg.adblockplus.org/adblockplus/rev/0cc5310ed634

Updated

5 years ago
tracking-fennec: --- → ?
Shilpan - Wes can help you out
Assignee: nobody → sbhagat
tracking-fennec: ? → +
I may be wrong, but doesn't the work I'm doing in Bug 891115 render this bug obsolete? In particular, the preferences for the addons will no longer be rendered by aboutAddons.js (Indeed, that file is going away) and will instead be interpreted as Preference objects inside Java.
If so, it doesn't seem pointful to fix a bug that is going to be overwritten by the upcoming overhaul anyway.

Or is xul:menulist used elsewhere in a way that needs this bug fixing?
You are correct, but I think this might illustrate one of the "hard things" we'll need to do to get Add-ons moved to settings. The <setting> elements will be tricky since they can have event handlers and call JS.
I think <xul:menulist> will be used regardless, e.g. on XUL pages loaded as content. And fixing this is trivial.
(Assignee)

Comment 5

5 years ago
Created attachment 778878 [details] [diff] [review]
Patch: Xul menulist now fires events

You're right, the problem exists with the XUL menulist. Hence this will pose a problem wherever a xul menulist is being used. This patch enables firing events on when a new option is selected in xul menulist.
Attachment #778878 - Flags: review?(mark.finkle)
Comment on attachment 778878 [details] [diff] [review]
Patch: Xul menulist now fires events

This will fire a "change" event - this is correct for <html:select> but <xul:menulist> needs a "command" event.
Attachment #778878 - Flags: feedback-
Comment on attachment 778878 [details] [diff] [review]
Patch: Xul menulist now fires events

>diff --git a/mobile/android/chrome/content/SelectHelper.js b/mobile/android/chrome/content/SelectHelper.js

>     p.show((function(data) {
>       let selected = data.button;
>       if (selected == -1)
>           return;
> 
>       let changed = false;

Since this is only used for the HTMLSelectElement, let's move it and the | if (changed) | block into the "else if" for HTMLSelectElement

>       if (aElement instanceof Ci.nsIDOMXULMenuListElement) {
>-        aElement.selectedIndex = selected;
>+        if(aElement.selectedIndex != selected) {
>+          aElement.selectedIndex = selected;
>+          changed = true;

Instead of using the "changed" flag here (since we are moving it into the HTMLSelectElement block), just fire a "command" event right here
Attachment #778878 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 778969 [details] [diff] [review]
Patch: Xul menulist now fires command event

Oops, forgive my utter noobishness here. It's pretty evident I did not work with XUL before. This patch does the right thing.
Attachment #778878 - Attachment is obsolete: true
Attachment #778969 - Flags: review?(mark.finkle)
Comment on attachment 778969 [details] [diff] [review]
Patch: Xul menulist now fires command event

>diff --git a/mobile/android/chrome/content/SelectHelper.js b/mobile/android/chrome/content/SelectHelper.js

>+        if(aElement.selectedIndex != selected) {

Sorry I missed this nit before: if (

r+, but put up a new patch for landing
Attachment #778969 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 783843 [details] [diff] [review]
Patch: Xul menulist now fires command event

Updated patch with the nit addressed
(Assignee)

Comment 13

5 years ago
Created attachment 783846 [details] [diff] [review]
Patch: Xul menulist now fires command event

Sorry, forgot to obsolete the previous patch. Please check this one in.
Attachment #778969 - Attachment is obsolete: true
Attachment #783843 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bf27f19ee5ce
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.