Closed Bug 891736 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25
Tracking Status
fennec + ---

People

(Reporter: gaubugzilla, Assigned: shilpanbhagat)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Blocks: abp
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.
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-
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+
Updated patch with the nit addressed
Sorry, forgot to obsolete the previous patch. Please check this one in.
Attachment #778969 - Attachment is obsolete: true
Attachment #783843 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bf27f19ee5ce
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.