Closed Bug 463209 Opened 11 years ago Closed 11 years ago

No sound when message box is opened (alert/confirm/prompt/etc..)

Categories

(Core :: Widget: Win32, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 10 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
When each message box is opened on Windows, the system sound is not played. We should play the sound.

# I searched this bug, but I couldn't find this bug... But it's strange...

The attached patch fixes only on Windows. I'm not sure the sounds are needed on other platforms too and how to implement it.
Attachment #346450 - Flags: superreview?(roc)
Attachment #346450 - Flags: review?(roc)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Sorry for the spam.
Attachment #346450 - Attachment is obsolete: true
Attachment #346458 - Flags: superreview?(roc)
Attachment #346458 - Flags: review?(roc)
Attachment #346450 - Flags: superreview?(roc)
Attachment #346450 - Flags: review?(roc)
Comment on attachment 346458 [details] [diff] [review]
Patch v1.1

oops, this patch has a bug.
Attachment #346458 - Attachment is obsolete: true
Attachment #346458 - Flags: superreview?(roc)
Attachment #346458 - Flags: review?(roc)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #346469 - Flags: superreview?(roc)
Attachment #346469 - Flags: review?(roc)
Comment on attachment 346469 [details] [diff] [review]
Patch v1.2

nsISound API sucks, but otherwise this is OK.

seeking ui-review from boriss to confirm that this is, in fact, what we want to do.
Attachment #346469 - Flags: ui-review?(jboriss)
Attachment #346469 - Flags: superreview?(roc)
Attachment #346469 - Flags: superreview+
Attachment #346469 - Flags: review?(roc)
Attachment #346469 - Flags: review+
I don't think alert/prompt/confirm from web pages should make sound.  What do other browsers do on Windows?
IE uses the sounds for them. I think we should use the sounds. Because we emulate the system message box. Why do you think we should not use the sounds?
Boriss: Would you check the patch?
Hey - sorry, I'm late here.  Masayuki, are you saying that it is standard for alerts from Firefox to make a sound?  Does "message box" refer to a notification, or alert, or popup?
Does this patch respect the system settings for no sounds? I have them all turned off and I'd rather Firefox doesn't break the silence or interrupt my music.
I think he's saying that currently, standard Windows apps do make a sound but Firefox doesn't. "message box" refers to any of the standard "prompt" dialog boxes --- the ones with a message, standard buttons such as "OK" and "Cancel", and an icon on the left.
Yes. Firefox doesn't respect the system sounds, Fx should respect them. My patch makes that the following dialogs respect the sounds:

message box (it only has ok button, e.g., javascript:alert('');):
  "Exclamation" sound.

confirming dialog (it has yes/no or ok/cancel, e.g., javascript:confirm('','');):
  "Question" sound.

prompt dialog (it has ok/cancel and input field, e.g, javascript:prompt('','');):
  "Question" sound.

username/password dialog (e.g., basic auth dialog):
  "Question" sound.

select dialog (it has a list, e.g., true/false selectable dialog on about:config when you create a new boolean value):
  "Question" sound.

# now, we don't use "Asterisk" and "Program error" sounds.
Attachment #346469 - Flags: ui-review?(jboriss) → ui-review+
Comment on attachment 346469 [details] [diff] [review]
Patch v1.2

As it's consistent with the operating system, this looks good.
Comment on attachment 346469 [details] [diff] [review]
Patch v1.2

This patch improves the emulation of native message boxes behavior. The risk is low.
Attachment #346469 - Flags: approval1.9.1b2?
Comment on attachment 346469 [details] [diff] [review]
Patch v1.2

Not for beta 2, but will evaluate afterwards.
Attachment #346469 - Flags: approval1.9.1b2?
Attachment #346469 - Flags: approval1.9.1b2-
Attachment #346469 - Flags: approval1.9.1?
Attachment #346469 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/mozilla-central/rev/26936f9168d2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
It looks like this caused SeaMonkey and also Thunderbird Tinderboxen to burn.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081126
Minefield/3.1b3pre
hmmm, I'm not getting any sound with this patch applied...
||javascript:alert("some text"); and javascript:confirm("some text");void 0;||
backed-out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So the reason this caused red is that nsISound is used from components that don't use internal strings, but it included internal strings.

Benjamin, is there a header that will "do the right thing" in terms of including something that will give a NS_LITERAL_STRING?

If not, probably better to not do NS_LITERAL_STRING in the C++ block here (and maybe even nix the C++ block).
bent pointed out nsStringGlue.h, which is exactly what's needed here.
Attached patch Patch v1.3 (obsolete) — Splinter Review
Thank you, Boris. I succeeded to build Seamonkey with this patch.
Attachment #346469 - Attachment is obsolete: true
Attachment #350282 - Flags: review?(bzbarsky)
Attached patch Patch v1.3 (obsolete) — Splinter Review
oops, sorry, I posted wrong file.
Attachment #350282 - Attachment is obsolete: true
Attachment #350283 - Flags: review?(bzbarsky)
Attachment #350282 - Flags: review?(bzbarsky)
(In reply to comment #18)
> Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081126
> Minefield/3.1b3pre
> hmmm, I'm not getting any sound with this patch applied...
> ||javascript:alert("some text"); and javascript:confirm("some text");void 0;||

Please confirm following conditions:

1. Make sure Question and Exclamation sounds are enabled on your system settings.
2. You don't play any sounds on other applications under sound resource is shared setting.
(In reply to comment #24)
The sound was working for the same script in IE (when the alert popped up the sound kicked in). I have to rebuild now anyhow, so I'll include the new patch and verify results tomorrow.
Comment on attachment 350283 [details] [diff] [review]
Patch v1.3

>+      var soundService = Components.classes["@mozilla.org/sound;1"].
>+                           createInstance(Components.interfaces.nsISound);
>+      soundService.playSystemSound(sound);

IMHO better style would be:

Components.classes["@mozilla.org/sound;1"]
          .createInstance(Components.interfaces.nsISound)
          .playSystemSound(sound);
Comment on attachment 350283 [details] [diff] [review]
Patch v1.3

>+#include "nsStringGlue.h"
>+
>+#define NS_SYSSOUND_PREFIX          NS_LITERAL_STRING("_moz_")
>+#define NS_SYSSOUND_MAIL_BEEP       NS_LITERAL_STRING("_moz_mailbeep")
>+#define NS_SYSSOUND_ERROR_DIALOG    NS_LITERAL_STRING("_moz_errordialog")
>+#define NS_SYSSOUND_INFO_DIALOG     NS_LITERAL_STRING("_moz_infodialog")
>+#define NS_SYSSOUND_QUESTION_DIALOG NS_LITERAL_STRING("_moz_questiondialog")
>+#define NS_SYSSOUND_WARNING_DIALOG  NS_LITERAL_STRING("_moz_warningdialog")
>+
>+static PRBool NS_IsMozAliasSound(const nsAString &aSoundAlias)
>+{
>+  return StringBeginsWith(aSoundAlias, NS_SYSSOUND_PREFIX);
>+}

This possible alternative might avoid the #include altogether:

#define NS_IsMozAliasSound(aSoundAlias) \
        StringBeginsWith(aSoundAlias, NS_SYSSOUND_PREFIX)
Attached patch Patch v1.4 (obsolete) — Splinter Review
It's clever way!
Attachment #350283 - Attachment is obsolete: true
Attachment #350330 - Flags: review?(neil)
Attachment #350283 - Flags: review?(bzbarsky)
Just FTR IE only makes a sound (seems like the Question?) on an alert dialog, not confirm or prompt. Also, native prompts like when closing multiple tabs at once don't get any sound. Chrome doesn't either make any sounds, not even for alert as doesn't Opera. Also that exclamation beep for every confirm to me doesn't sound right (i.e. when I close more than one tab or enter private browsing, I get exclamation), it sounds like I did something wrong, which is usually when that sound is played.

The latest patch does make sounds on every single dialog. Prompt/alert/confirm/native dialogs (like private browsing), etc.
I agree, sounds are OK for Alert* and Confirm* but I don't think they make much sense for Prompt* or Select* functions.
Attachment #350330 - Flags: review?(neil)
Comment on attachment 350330 [details] [diff] [review]
Patch v1.4

Cancelling review because I'm not keen on the beeps for the prompt/select. I'd be happy to review a patch that only beeped on alert/confirm.

>   paramBlock->SetInt(nsPIPromptService::eNumberButtons, 1);
>   paramBlock->SetString(nsPIPromptService::eIconClass, NS_LITERAL_STRING("alert-icon").get());
>   paramBlock->SetString(nsPIPromptService::eDialogTitle, aDialogTitle);
>   paramBlock->SetString(nsPIPromptService::eMsg, aText);
>+  paramBlock->SetString(eOpeningSound, NS_SYSSOUND_WARNING_DIALOG.get());
Strange that this version of the code uses an explicit nsPIPromptSerivce::
Attached patch Patch v1.5 (obsolete) — Splinter Review
ok, this is better. I redefined as:

> #define NS_SYSSOUND_ALERT_DIALOG    NS_LITERAL_STRING("_moz_alertdialog")
> #define NS_SYSSOUND_CONFIRM_DIALOG  NS_LITERAL_STRING("_moz_confirmdialog")
> #define NS_SYSSOUND_PROMPT_DIALOG   NS_LITERAL_STRING("_moz_promptdialog")
> #define NS_SYSSOUND_SELECT_DIALOG   NS_LITERAL_STRING("_moz_selectdialog")

And nsSound of Win32 only plays the sounds at NS_SYSSOUND_ALERT_DIALOG and NS_SYSSOUND_CONFIRM_DIALOG. So, nsSound can choose the sounds for each dialogs instead of nsPromptService.
Attachment #350330 - Attachment is obsolete: true
Attachment #350362 - Flags: review?(neil)
Comment on attachment 350362 [details] [diff] [review]
Patch v1.5

>diff --git a/toolkit/content/selectDialog.js b/toolkit/content/selectDialog.js
...
>+  // play sound
>+  try {
>+    var sound = param.GetString(2);
Isn't this always going to be _moz_selectdialog?
Attached patch Patch v1.6 (obsolete) — Splinter Review
Attachment #350362 - Attachment is obsolete: true
Attachment #350434 - Flags: review?(neil)
Attachment #350362 - Flags: review?(neil)
I think a sound would be especially usefull for the larry bar which users can entirely miss. IE plays an interesting sound by the bar and I think it's most usefull then, as opposed to alerts and confirms which are modal anyway. Sorry for all the late calls, I just think this is something worth addressing, as to date it is pretty easy to miss the bar. :-)
(In reply to comment #35)
> I think a sound would be especially usefull for the larry bar which users can
> entirely miss. IE plays an interesting sound by the bar and I think it's most
> usefull then, as opposed to alerts and confirms which are modal anyway. Sorry
> for all the late calls, I just think this is something worth addressing, as to
> date it is pretty easy to miss the bar. :-)

I think that other UI's sounds suggestions should be separated to other bugs.
Comment on attachment 350434 [details] [diff] [review]
Patch v1.6

>-	if (aSoundAlias.EqualsLiteral("_moz_mailbeep")) 
>-		return Beep();
>+	if (NS_IsMozAliasSound(aSoundAlias)) {
>+		if (aSoundAlias.Equals(NS_SYSSOUND_MAIL_BEEP)) 
You got 3 out of 4 trailing spaces, but you missed this one ;-)

>+  const wchar_t *sound = nsnull;
>+  if (aSoundAlias.Equals(NS_SYSSOUND_MAIL_BEEP))
>+    sound = L"MailBeep";
>+  else if (aSoundAlias.Equals(NS_SYSSOUND_CONFIRM_DIALOG))
>+    sound = L"SystemQuestion";
>+  else if (aSoundAlias.Equals(NS_SYSSOUND_ALERT_DIALOG))
>+    sound = L"SystemExclamation";
>+
>+  if (sound)
>+    ::PlaySoundW(sound, nsnull, SND_ALIAS | SND_ASYNC);
This turned out to be harder than it looks - when you have (None) defined as your sound in your sound scheme then this code still dings. Fortunately this is easily resolved by adding the SND_NODEFAULT flag. r=me with this fixed.

[I then defined a Question sound, but this seemed to trigger a Windows XP bug; the Sounds control panel only saved "chord.wav" in the registry but PlaySound played "W:chord.wav" because it was in my PATH. Further testing suggests that MessageBeep(MB_ICONQUESTION) gets it right either way, but of course that solution isn't applicable to the mail beep.]
Attachment #350434 - Flags: review?(neil) → review+
Attached patch Patch v1.7Splinter Review
Thank you, Neil.
Attachment #350434 - Attachment is obsolete: true
Attachment #350482 - Flags: approval1.9.1?
Comment on attachment 350482 [details] [diff] [review]
Patch v1.7

a191=beltzner
Attachment #350482 - Flags: approval1.9.1? → approval1.9.1+
Attached file bundle file (obsolete) —
Comment on attachment 350482 [details] [diff] [review]
Patch v1.7

>+  if (NS_IsMozAliasSound(aSoundAlias)) {
>
>+    // We don't have a default mail sound on OS/2, so just beep.
>
>+    if (aSoundAlias.Equals(NS_SYSSOUND_MAIL_BEEP))
>
>+      Beep();
>
>+    // XXX Should we call Beep() for NS_SYSSOUND_ALERT_DIALOG and
>
>+    // NS_SYSSOUND_CONFIRM_DIALOG?

Masayuki: the answer is no. Can you please file a follow-up bug if you don't want to remove this before checkin?
You're always welcome to CC me if you have OS/2 questions. :-)
Attached file bundle file #2 (obsolete) —
thank you, removed the XXX comment of OS/2.
Attachment #350549 - Attachment is obsolete: true
Attachment #350568 - Attachment is obsolete: true
relanded.
http://hg.mozilla.org/mozilla-central/rev/f5fa6e92cd13
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Whiteboard: [needs 191 landing] → [c-n: baking for 1.9.1]
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Keywords: fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Duplicate of this bug: 42212
You need to log in before you can comment on or make changes to this bug.