Closed Bug 520368 Opened 10 years ago Closed 10 years ago

<menulist> Setting onchange Handler Does Not Fire

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
fennec1.0b5

People

(Reporter: mackers, Assigned: vingtetun)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Blocks: 509081
Blocks: 492546
tracking-fennec: --- → ?
I wonder, does Fennec use menulist anywhere currently?
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)
Assignee: nobody → 21
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
* 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
Attachment #405245 - Flags: review?(mark.finkle)
Attached patch Patch v0.2 (obsolete) — Splinter Review
Get rid of the type getters
Attachment #405245 - Attachment is obsolete: true
Attachment #405254 - Flags: review?
Attachment #405245 - Flags: review?(mark.finkle)
Attached patch Patch v0.2 (obsolete) — Splinter Review
oups! Wrong patch!
Sorry for the spam.
Attachment #405254 - Attachment is obsolete: true
Attachment #405255 - Flags: review?(mark.finkle)
Attachment #405254 - Flags: review?
Attached patch Patch v0.3Splinter Review
* 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)
Attachment #405255 - Attachment is obsolete: true
Attachment #405270 - Flags: review?(mark.finkle)
Attachment #405255 - Flags: review?(mark.finkle)
Attachment #405270 - Flags: review?(mark.finkle) → review+
pushed:
https://hg.mozilla.org/mobile-browser/rev/23fe24602080
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → B5
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.