Add support for windows sounds to Menus

VERIFIED FIXED in mozilla1.9.2a1



18 years ago
10 years ago


(Reporter: eem12, Assigned: masayuki)


(Blocks 2 bugs, {polish, verified1.9.1})

Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 5 obsolete attachments)



18 years ago
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.
Severity: normal → trivial
Ever confirmed: true
Target Milestone: --- → Future

Comment 2

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

Comment 3

18 years ago
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.
Assignee: pinkerton → dean_tessman

Comment 4

18 years ago
do we need this on the Mac, too?

Comment 5

18 years ago
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.
Target Milestone: Future → mozilla1.0

Comment 6

18 years ago
Posted patch cvs diff -u (obsolete) — Splinter Review

Comment 8

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
Hyatt mentioned something about sounds on IRC tonight, so I might as well cc: 

Comment 12

18 years ago
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

18 years ago
Posted patch cvs diff -u mozilla/widget (obsolete) — Splinter Review

Comment 14

18 years ago
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

18 years ago
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.

Comment 17

18 years ago
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

18 years ago
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

18 years ago
Posted patch cvs diff -u mozilla/widget (obsolete) — Splinter Review

Comment 20

18 years ago
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

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


18 years ago
Attachment #45052 - Attachment is obsolete: true


18 years ago
Attachment #48040 - Attachment is obsolete: true
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.


18 years ago
Attachment #48109 - Attachment is obsolete: true

Comment 24

18 years ago
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.

Comment 27

18 years ago
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
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 
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 29

18 years ago
I've done all I can on this.  --> hyatt
Assignee: dean_tessman → hyatt

Comment 30

17 years ago
*** Bug 160078 has been marked as a duplicate of this bug. ***
Blocks: 162749
would this cover alerts and such as well (bug 162749)? (mac: bug 74628)

Comment 32

17 years ago
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

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

Comment 34

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

Comment 35

16 years ago
Target Milestone: mozilla1.0.1 → Future


16 years ago
Severity: trivial → enhancement
Keywords: helpwanted
Summary: Menus don't play Windows sounds. → Add support for windows sounds to Menus
Target Milestone: Future → ---
*** Bug 240861 has been marked as a duplicate of this bug. ***


15 years ago
Keywords: polish
*** 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.

Comment 40

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

Comment 41

13 years ago
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

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


12 years ago
Assignee: hyatt → nobody
No longer blocks: 162749
OS: Windows NT → Windows XP
QA Contact: jrgmorrison → xptoolkit.menus


11 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets


11 years ago
Blocks: 265037
Posted patch Patch v1.0Splinter Review
simple fix on win32.
Assignee: nobody → masayuki
Attachment #53481 - Attachment is obsolete: true
Attachment #53501 - Attachment is obsolete: true
Attachment #351791 - Flags: superreview?(roc)
Attachment #351791 - Flags: review?(neil)
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.


11 years ago
Attachment #351791 - Flags: review?(neil) → review?(enndeakin)
Attachment #351791 - Flags: superreview?(roc) → superreview+
Attachment #351791 - Flags: ui-review?(jboriss) → ui-review+
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

may god have mercy on our souls. ui+

Comment 46

11 years ago
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.
Attachment #351791 - Flags: review?(enndeakin) → review+
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.

pushed to trunk.
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 351791 [details] [diff] [review]
Patch v1.0

Attachment #351791 - Flags: approval1.9.1? → approval1.9.1+
Keywords: fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]

Comment 50

11 years ago
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

11 years ago
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??

Comment 53

11 years ago
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.

Comment 55

11 years ago

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
You need to log in before you can comment on or make changes to this bug.