Add support for windows sounds to Menus

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
XUL
--
enhancement
VERIFIED FIXED
17 years ago
8 years ago

People

(Reporter: Ed McKenzie, Assigned: masayuki)

Tracking

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

Trunk
mozilla1.9.2a1
x86
Windows XP
polish, verified1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

17 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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Future

Comment 2

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

Comment 3

16 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
Status: ASSIGNED → NEW

Comment 4

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

Comment 5

16 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.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.0

Comment 6

16 years ago
Created attachment 45052 [details] [diff] [review]
cvs diff -u

Comment 7

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

Comment 8

16 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

16 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

16 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

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

Comment 12

16 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" 
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.

Comment 13

16 years ago
Created attachment 48040 [details] [diff] [review]
cvs diff -u mozilla/widget

Comment 14

16 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

16 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

16 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

16 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
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.

Comment 19

16 years ago
Created attachment 48109 [details] [diff] [review]
cvs diff -u mozilla/widget

Comment 20

16 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

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

Updated

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

Updated

16 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.

Updated

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

Comment 23

16 years ago
Created attachment 53481 [details] [diff] [review]
new patch  -  cvs diff -u mozilla/widget

Comment 24

16 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.

Comment 25

16 years ago
Created attachment 53501 [details] [diff] [review]
get nsStatusBarBiffManager.cpp working again
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

16 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
sound.

Comment 28

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

Comment 29

16 years ago
I've done all I can on this.  --> hyatt
Assignee: dean_tessman → hyatt
Status: ASSIGNED → NEW

Comment 30

15 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

15 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
now.

Comment 33

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

Comment 34

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

Comment 35

14 years ago
retargeting
Target Milestone: mozilla1.0.1 → Future

Updated

14 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. ***
Keywords: polish
*** Bug 286725 has been marked as a duplicate of this bug. ***

Comment 38

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

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

Comment 41

11 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

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

Updated

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

Updated

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

Updated

9 years ago
Blocks: 265037
Created attachment 351791 [details] [diff] [review]
Patch v1.0

simple fix on win32.
Assignee: nobody → masayuki
Attachment #53481 - Attachment is obsolete: true
Attachment #53501 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #351791 - Flags: superreview?(roc)
Attachment #351791 - Flags: review?(neil)
Attachment #351791 - Flags: ui-review?(jboriss)
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.
Keywords: helpwanted
Blocks: 461963

Updated

9 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 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+
Attachment #351791 - Flags: approval1.9.1?
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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

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

Comment 50

9 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

9 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

9 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

9 years ago
Verified.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Depends on: 498089
You need to log in before you can comment on or make changes to this bug.