Last Comment Bug 83056 - Add support for windows sounds to Menus
: Add support for windows sounds to Menus
: polish, verified1.9.1
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows XP
: -- enhancement with 3 votes (vote)
: mozilla1.9.2a1
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
: Neil Deakin
: 160078 162749 183155 240861 286725 (view as bug list)
Depends on: 498089
Blocks: 265037 461963
  Show dependency treegraph
Reported: 2001-05-28 18:17 PDT by Ed McKenzie
Modified: 2009-06-13 03:42 PDT (History)
20 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

cvs diff -u (2.02 KB, patch)
2001-08-08 00:49 PDT, Dean Tessman
no flags Details | Diff | Splinter Review
cvs diff -u mozilla/widget (1.69 KB, patch)
2001-09-02 17:21 PDT, Dean Tessman
no flags Details | Diff | Splinter Review
cvs diff -u mozilla/widget (8.96 KB, patch)
2001-09-03 18:55 PDT, Dean Tessman
no flags Details | Diff | Splinter Review
new patch - cvs diff -u mozilla/widget (10.17 KB, patch)
2001-10-14 10:27 PDT, Dean Tessman
no flags Details | Diff | Splinter Review
get nsStatusBarBiffManager.cpp working again (714 bytes, patch)
2001-10-14 16:34 PDT, Dean Tessman
no flags Details | Diff | Splinter Review
Patch v1.0 (4.35 KB, patch)
2008-12-07 08:37 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
enndeakin: review+
roc: superreview+
jboriss: ui‑review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description Ed McKenzie 2001-05-28 18:17:28 PDT
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.
Comment 1 Mike Pinkerton (not reading bugmail) 2001-05-29 09:50:06 PDT
dear god.
Comment 2 Chad Austin 2001-06-22 15:00:00 PDT
As silly as this is, it's not following platform conventions...  It should be
done someday.  ;)
Comment 3 Dean Tessman 2001-07-30 18:42:16 PDT
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.
Comment 4 Dean Tessman 2001-07-30 21:55:45 PDT
do we need this on the Mac, too?
Comment 5 Dean Tessman 2001-08-08 00:48:28 PDT
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.
Comment 6 Dean Tessman 2001-08-08 00:49:10 PDT
Created attachment 45052 [details] [diff] [review]
cvs diff -u
Comment 7 Dean Tessman 2001-08-08 00:56:24 PDT
Details of Mac system sounds are found here (that's out of my hands):
Comment 8 Dean Tessman 2001-08-08 02:02:46 PDT
Thinking about this, that should probably be SND_ASYNC not SND_SYNC, and add
SND_NODEFAULT as well to suppress the default beep.
Comment 9 Chad Austin 2001-08-08 16:44:50 PDT
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.
Comment 10 Dean Tessman 2001-08-08 23:11:02 PDT
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.
Comment 11 Dean Tessman 2001-08-08 23:23:15 PDT
Hyatt mentioned something about sounds on IRC tonight, so I might as well cc: 
Comment 12 David Hyatt 2001-08-08 23:29:40 PDT
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" 

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.
Comment 13 Dean Tessman 2001-09-02 17:21:24 PDT
Created attachment 48040 [details] [diff] [review]
cvs diff -u mozilla/widget
Comment 14 Dean Tessman 2001-09-02 17:23:10 PDT
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.
Comment 15 David Hyatt 2001-09-03 12:58:42 PDT
yes.  go for it.
Comment 16 Christopher Blizzard (:blizzard) 2001-09-03 16:52:55 PDT
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.
Comment 17 Dean Tessman 2001-09-03 17:13:34 PDT
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?
Comment 18 Dean Tessman 2001-09-03 17:22:21 PDT
We also need to determine what system sounds PlaySystemSound will recognize.  I
think a good start would be:

menushowing or popupshowing
menucommand or popupcommand
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.
Comment 19 Dean Tessman 2001-09-03 18:55:01 PDT
Created attachment 48109 [details] [diff] [review]
cvs diff -u mozilla/widget
Comment 20 Dean Tessman 2001-09-03 18:56:18 PDT
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().
Comment 21 Dean Tessman 2001-09-03 22:11:54 PDT
In my last comment I meant PlaySystemSound(), not PlayDefaultSound().
Comment 22 (not reading, please use instead) 2001-10-10 01:15:47 PDT
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.

Comment 23 Dean Tessman 2001-10-14 10:27:53 PDT
Created attachment 53481 [details] [diff] [review]
new patch  -  cvs diff -u mozilla/widget
Comment 24 Dean Tessman 2001-10-14 10:28:50 PDT
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.

Comment 25 Dean Tessman 2001-10-14 16:34:37 PDT
Created attachment 53501 [details] [diff] [review]
get nsStatusBarBiffManager.cpp working again
Comment 26 (not reading, please use instead) 2001-11-16 17:26:47 PST
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.

Comment 27 Dean Tessman 2001-11-16 17:48:06 PST
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
Comment 28 Asa Dotzler [:asa] 2001-12-03 10:28:17 PST
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 
Comment 29 Dean Tessman 2002-01-25 23:25:20 PST
I've done all I can on this.  --> hyatt
Comment 30 R.K.Aa. 2002-07-30 05:06:17 PDT
*** Bug 160078 has been marked as a duplicate of this bug. ***
Comment 31 Tuukka Tolvanen (sp3000) 2002-08-14 15:39:06 PDT
would this cover alerts and such as well (bug 162749)? (mac: bug 74628)
Comment 32 Dean Tessman 2002-08-14 16:39:39 PDT
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
Comment 33 John Levon 2002-11-10 13:24:37 PST
*** Bug 162749 has been marked as a duplicate of this bug. ***
Comment 34 R.K.Aa. 2002-12-03 00:28:47 PST
*** Bug 183155 has been marked as a duplicate of this bug. ***
Comment 35 Vedran Miletic 2003-10-05 08:10:39 PDT
Comment 36 Jeff Walden [:Waldo] (remove +bmo to email) 2004-04-18 19:56:58 PDT
*** Bug 240861 has been marked as a duplicate of this bug. ***
Comment 37 Frank Wein [:mcsmurf] 2005-03-18 15:42:34 PST
*** Bug 286725 has been marked as a duplicate of this bug. ***
Comment 38 Michael B. Trausch 2005-03-19 11:18:08 PST
What is going on with this bug?  Anything?  Or is it just sitting?  The last
useful status I see is in 2002...
Comment 39 Jeff Walden (gone starting June 8) 2005-03-19 11:29:22 PST
(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.
Comment 40 José Jeria 2005-04-05 13:05:34 PDT
*** Bug 289176 has been marked as a duplicate of this bug. ***
Comment 41 Kurt Fitzner 2007-01-23 08:33:37 PST
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.
Comment 42 Robin Lionheart 2007-02-19 10:14:39 PST
While you're at it, it'd be nice to put in the start navigation sound as described in bug #98674.
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-07 08:37:04 PST
Created attachment 351791 [details] [diff] [review]
Patch v1.0

simple fix on win32.
Comment 44 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-07 08:40:10 PST
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 45 Jennifer Morrow [:Boriss] (UX) 2008-12-08 10:44:57 PST
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

may god have mercy on our souls. ui+
Comment 46 Neil Deakin 2008-12-09 15:52:58 PST
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 47 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-09 23:46:28 PST
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.
Comment 48 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-10 09:30:22 PST

pushed to trunk.
Comment 49 Mike Beltzner [:beltzner, not reading bugmail] 2008-12-12 07:10:18 PST
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

Comment 50 timeless 2008-12-15 18:17:47 PST
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.
Comment 51 Stuart Parmenter 2008-12-15 18:26:02 PST
we call PlaySoundW with the ASYNC flag for a reason.
Comment 52 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-15 21:56:35 PST
I'm not sure which case is locked by PlaySound. Should I add SND_NOWAIT flag??
Comment 53 timeless 2008-12-16 01:51:53 PST
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).
Comment 54 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-16 09:24:22 PST
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.
Comment 55 Charles Fenwick 2009-01-27 17:37:36 PST

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

Note You need to log in before you can comment on or make changes to this bug.