Steps to reproduce: 1. Install a Windows desktop theme that assigns sounds to menu open events. 2. Open Navigator and navigate its menus. 3. Open Mozilla and navigate its menus. Navigator plays menu sounds, but Mozilla does not. I understand why, but I still think this is a bug.

dear god.

As silly as this is, it's not following platform conventions... It should be done someday. ;)

As much as it pains me to say this, I use a theme on one of my machines that has menu sounds (hey, it's a Spiderman theme, gimme a break!). This should be pretty easy to do and, from his comment, I'm sure Mike won't mind me taking this from him.

do we need this on the Mac, too?

Preliminary patch coming, so I don't forget where I need to put the sound calls. I'd like to move the sound calls to somewhere central, so I don't pollute the code with #ifdef XP_WIN followed by #ifdef XP_MAC, etc.

Details of Mac system sounds are found here (that's out of my hands): http://developer.apple.com/techpubs/macos8/HumanInterfaceToolbox/AppManager/ProgWithAppearanceMgr/Appearance.ab.html

Thinking about this, that should probably be SND_ASYNC not SND_SYNC, and add SND_NODEFAULT as well to suppress the default beep.

The patch looks good to me, but I'd rather there be nsILookAndFeel methods for this. That way people can easily implement this on OS X or BeOS (or whatever) without caring about who uses the functionality.

Oh, whoops, I thought I mentioned my idea in my first 08-08 post. I originally thought about nsILookAndFeel, and initially went down that path, but then I realized that the functions in there are more for retrieving setting values. nsISound already has a playsound() function, and it seems like a good fit to add a PlaySystemSound() function which would use platform-independent application event constants.

Hyatt mentioned something about sounds on IRC tonight, so I might as well cc: him.

I have been meaning for some time to hook up to skins the ability to play sounds via event handlers. So you could do something like this for example: <handler event="popupshowing" sound="system-sound:popupshowing"/> we could have some convention of system sound names and also support .wavs etc. <handler event="popupshowing" sound="chrome://global/skin/sounds/mycustomsound.wav"/> This would all presumably be done using the nsISound interface behind the scenes, which IMO should be enhanced to treat certain strings as builtin system sounds, e.g., "system-sound:popupshowing" or some such.

This patch adds nsISound::PlaySystemSound() on Windows. The |strcpy (systemSound, "\0")| is extraneous, I now realize, but you get the idea. Hyatt, is this what you were looking for? If so, I'll go ahead and add blank methods on the other platforms' nsSound.cpp.

yes. go for it.

Since you're changing the IDL for the sound interface you need to change it on all of the platforms or find a base implementation file so that you don't get undefined symbols.

That's what I meant when I said I'd add blank methods on the other platforms, or am I missing something. I guess the safest thing to do is to add it for all platforms in the widget directory?

We also need to determine what system sounds PlaySystemSound will recognize. I think a good start would be: menushowing or popupshowing menucommand or popupcommand windowmaximize windowminimize windowrestoreup windowrestoredown windowopen windowclose defaultbeep (?) There are compatible events in both Windows and Mac, and I'm assuming that the Linux theme managers can handle these basic sounds. After we get these in we can start adding sounds for alerts, etc., which are a little different between Windows and Mac.

This patch adds PlayDefaultSound() on all the platforms that I could find an nsSound.cpp for. It also adds support for the system sounds that I mentioned earlier on Windows. All other platforms only have support for "defaultbeep", which just calls nsSound::Beep().

In my last comment I meant PlaySystemSound(), not PlayDefaultSound().

dean, we think alike! I just landed something very close to this (r=pavlov) for #64462. craft another patch, and since I accidentally obsoleted this patch, I'll gladly do the review.

Differences between this and the patch for bug 64462: 1) the 64462 version didn't add SND_NODEFAULT on Windows 2) This version uses an nsAReadableString instead of const char 3) The parameter is named differently in the .idl (aSoundAlias vs. soundAlias) 4) PlaySystemSound() on other platforms doesn't always beep, it only beeps if aSoundAlias is "defaultbeep" 5) I hard-coded a lot of sound aliases in, instead of defaulting to what Windows names them, to try to give some of them more xp names. I only added "_moz_" to "mailbeep", because I don't think we need it elsewhere.

if these are internal sounds, then prefix them with _moz_* sfraser asked me to add that for mailbeep, on the off chance someone already had that system sound. if they did, then they couldn't select that sound for something else, say on window open. for review, you'll want pavlov or sfraser.

I don't understand why we need to prefix them. From hyatt's example of how he sees this working in skins: <handler event="popupshowing" sound="system-sound:popupshowing"/> So these would be prefixed with "system-sound:". If in code you want to hard-code a sound, you're going to call PlaySound() if you want to play a sound file, and PlaySystemSound() if you want to translate the sound event to a system sound.

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 (you can query for this string to delete spam or retrieve the list of bugs I've moved)

I've done all I can on this. --> hyatt

*** Bug 160078 has been marked as a duplicate of this bug. ***

would this cover alerts and such as well (bug 162749)? (mac: bug 74628)

Tuukka: yes, my approach in attachment 53481 [details] [diff] [review] lays the framework for playing of standard Windows sounds. The theme support, as described in comment 12, would also need to be added. Of course, I'm sure that patch is incredibly out of date now.

*** Bug 162749 has been marked as a duplicate of this bug. ***

*** Bug 183155 has been marked as a duplicate of this bug. ***

retargeting

*** Bug 240861 has been marked as a duplicate of this bug. ***

*** Bug 286725 has been marked as a duplicate of this bug. ***

What is going on with this bug? Anything? Or is it just sitting? The last useful status I see is in 2002...

(In reply to comment #38) > What is going on with this bug? Anything? Or is it just sitting? The last > useful status I see is in 2002... If you don't see any comments, then nothing is happening. When bugs get started /usually/ someone will comment on the bug to that effect. If you personally want something to happen, either start something yourself or hire someone to do it, because nothing's happening now.

*** Bug 289176 has been marked as a duplicate of this bug. ***

It is not just menus that do not cause Windows sound events. A window restore up also fails to trigger the appropriate event. This is an accesibility issue - people may depend on sound events for feedback.

While you're at it, it'd be nice to put in the start navigation sound as described in bug #98674.

simple fix on win32.

Comment on attachment 351791 [details] [diff] [review] Patch v1.0 This patch added to support menu popup sound and menu execute sound. It's using system setting sounds. Of course, if users don't set sound to them in Control Panel, this patch doesn't play sounds.

Comment on attachment 351791 [details] [diff] [review] Patch v1.0 may god have mercy on our souls. ui+

Comment on attachment 351791 [details] [diff] [review] Patch v1.0 Looks OK. It would probably be better to play the popup sound when the popup actually appears rather than when the request to have it appear is made, assuming that triggering sounds during a layout is safe. But I'd go with this for now.

Comment on attachment 351791 [details] [diff] [review] Patch v1.0 Thank you for the reviewers! The risk is low. But the sounds when menu popup and menu execute may help some users who need the sounds for accessibility.

http://hg.mozilla.org/mozilla-central/rev/8602d55fe61f pushed to trunk.

Comment on attachment 351791 [details] [diff] [review] Patch v1.0 a191=beltzner

i object to the statement that the risk is low. you're introducing code in a critical layout path. code which will call out to system libraries and may lock for some period of time.

we call PlaySoundW with the ASYNC flag for a reason.

I'm not sure which case is locked by PlaySound. Should I add SND_NOWAIT flag??

I'm not saying the code is definitely wrong. I'm just saying that such a thing isn't truly low risk, especially given the state of sound systems on Linux. And if the implementor isn't certain about such characteristics then it's not something to say "risk is low". This is the kind of thing that I believe should be baked a bit longer (and preferably with some advertising to ensure it's tested).

I understood that timeless knows such cases. We are calling PlaySoundW with ASYNC as Stuart said. In such case, PlaySound API doesn't wait until to success to play sounds, I think (especially, at system sounds). Therefore, I said "risk is low". But I can understand timeless's objection too. But I still think the risk is low.

Verified. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre