520368 - <menulist> Setting onchange Handler Does Not Fire

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[David McNamara [:mackers]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3) Gecko/20090824 Ant.com Toolbar 1.4 Firefox/3.1b2, Ant.com Toolbar 1.2
Build Identifier: 

Split from bug 516122.

The onchange handler for a menulist in a settings window does not fire.

I've tested with the sample codes, but without success.

<setting type="control" title="Testing">
  <menulist onchange="Components.reportError('Hello');">
    <menupopup>
      <menulist label="AAA"/>
      <menulist label="BBB"/>
    </menupopup>
  </menulist>
</setting>

<setting title="Favorite Color" type="control">
  <menulist onchange="MyAddon.SetColor();">
    <menupopup>
      <menuitem value="red" label="Red"/>
      <menuitem value="blue" label="Blue"/>
      <menuitem value="green" label="Green"/>
    </menupopup>
  </menulist>
</setting>

Neither a components.reportError nor window.alert javascript fire.

I am also seeing javascript errors in the console:

Error: val.setAttribute is not a function
Source File: chrome://global/content/bindings/menulist.xml line 224

Error: oldval.removeAttribute is not a function
Source File: chrome://global/content/bindings/menulist.xml line 211

(FWIW Fabrice has confirmed this in person)


Reproducible: Always

Comment 1

[Brian King [:kinger]]

I wonder, does Fennec use menulist anywhere currently?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm already working on this, I have a WIP to fire oncommand on menulist but it sounds like the selectedIndex property is weird in our current implementation for menulist.

(In reply to comment #1)
> I wonder, does Fennec use menulist anywhere currently?

Probably not, and I think menulist should not fire any onChange event, that are specific to HTML elements, but a oncommand one instead (which is not working actually).

(In reply to comment #0)

> I've tested with the sample codes, but without success.
> 
> <setting type="control" title="Testing">
>   <menulist onchange="Components.reportError('Hello');">
>     <menupopup>
>       <menulist label="AAA"/>
>       <menulist label="BBB"/>
>     </menupopup>
>   </menulist>
> </setting>

These 2 nested menulist elements should be menuitem (this didn't resolved the error btw)

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

* Replace the onchange handler by a oncommand handler
* Remove the text nodes inserted cause of XMLHttpRequest (which are the cause of the errors that David saw in his console)
* Correct a few errors in our SelectHelper implem

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Get rid of the type getters

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

oups! Wrong patch!
Sorry for the spam.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

* handle command/oncommand/addEventListener/* for Menulist
* handle onchange/addEventlistener/* for html:select

This also resolved bug 519271 since we fire a onchange event now instead of manually calling the select.onchange method.

I'm wondering if that's bad to dispatch a command event to every item? (imho : no)

Comment 7

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/23fe24602080