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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wladimir Palant]

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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Shilpan - Wes can help you out

Comment 2

[Chris Kitching [:ckitching]]

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?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 4

[Wladimir Palant]

I think <xul:menulist> will be used regardless, e.g. on XUL pages loaded as content. And fixing this is trivial.

Comment 5

[Shilpan Bhagat]

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

Comment 6

[Wladimir Palant]

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.

Comment 7

[Wladimir Palant]

For reference, http://mxr.mozilla.org/mobile-browser/source/chrome/content/MenuListHelperUI.js and http://mxr.mozilla.org/mobile-browser/source/chrome/content/SelectHelperUI.js do the right thing.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

in particular:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/MenuListHelperUI.js#78

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 10

[Shilpan Bhagat]

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

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 12

[Shilpan Bhagat]

Attachment: Patch: Xul menulist now fires command event

Updated patch with the nit addressed

Comment 13

[Shilpan Bhagat]

Attachment: Patch: Xul menulist now fires command event

Sorry, forgot to obsolete the previous patch. Please check this one in.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf27f19ee5ce

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf27f19ee5ce