604451 - Provide a way to play the "ding" system sound used to indicate typing too many characters in a text field on Windows

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[(no longer active)]

In order to fix bug 603190, we'll need a new value for nsISound::PlayEventSound for the "ding" sound played when someone types more characters than the maximum allowed value inside a text field.

Comment 1

[Jan]

nsISound::beep() seems to play that sound. Tested by setting the specific sound ("Standardton Warnsignal" in my german Windows) to a distinct sound, then evaluating Components.classes["@mozilla.org/sound;1"].createInstance(Components.interfaces.nsISound).beep() in the error console and with a mspaint dialog box that has a length-limited input field. Entering too much into the mspaint dialog created the distinct sound and beep() did too, so beep() should make the correct sound for typing too many characters even if a user has custom sound settings. Additionally, it may already be cross-plattform, so using it may be the best solution.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

No, we should use nsISound::PlayEventSound. And nsSound for Windows should call beep() if it's correct behavior.

It seems that Windows plays the sound for each discarded character. I mean, when IME commits a couple of characters, Windows plays the sound a couple of times.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch for new constant in nsISound and Win32 nsSound for max length reached

The default beep is the one that is used when max length is reached in an edit box in windows.  I verified by changing it to a custom sound and reaching the max length.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 558374 [details] [diff] [review]
Patch for new constant in nsISound and Win32 nsSound for max length reached

> +#define NS_SYSSOUND_EDITOR_MAX_LEN  NS_LITERAL_STRING("_moz_editormaxlen")

> +  else if (aSoundAlias.Equals(NS_SYSSOUND_EDITOR_MAX_LEN))
> +    eventId = EVENT_EDITOR_MAX_LEN;

You don't need to support the new event sound by PlaySystemSound(). All callers should call PlayEventSound().

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch for new constant in nsISound and Win32 nsSound for max length reached. v2

Removed PlaySystemSound support for the new event as per Masayuki Nakano's suggestion.

Comment 6

[Brian R. Bondy [:bbondy]]

Bug 603190 for which this bug was originally created is going to go with a visual cue instead when max length is reached.  I will still push this patch though for other XUL apps such as Thunderbird that may want to support it to get platform default behaviour.

Comment 7

[(no longer active)]

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> Bug 603190 for which this bug was originally created is going to go with a
> visual cue instead when max length is reached.  I will still push this patch
> though for other XUL apps such as Thunderbird that may want to support it to
> get platform default behaviour.

They're not going to get that behavior by merely having this patch in, since the editor code won't be using it... :/

Comment 8

[Brian R. Bondy [:bbondy]]

I know they wouldn't without specifically implementing it themselves :)

Would it be better for me to mark this task as RESOLVED | INVALID ?

Comment 9

[(no longer active)]

No, I think you should definitely get this landed.  I just wanted to mention that it won't automatically give those applications what they want...

Comment 10

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4a73b5bc2681

I selected all platforms instead of only just Windows because there is an interface which gets compiled on all platforms.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 558458 [details] [diff] [review]
Patch for new constant in nsISound and Win32 nsSound for max length reached. v2

Super review requested for the minor interface change.

Comment 12

[(no longer active)]

Why does this affect add-on compatibility?

Comment 13

[Brian R. Bondy [:bbondy]]

I'm not sure which action items come from marking that keyword, but I was told on IRC to put it and request super review for any and all interface changes.  Pls advise if the keyword is only for modifications and removals.

Comment 14

[(no longer active)]

The addon-compat keyword is used for changes which can potentially break existing add-ons.  On a closer look, this patch changes the IID of nsISound, so it's possible for it to break binary add-ons which use that interface.  Therefore, you can ignore my comment, I was wrong!  :-)

Comment 15

[Brian R. Bondy [:bbondy]]

That brings me to another question... I was told to always update the IID of every interface I change. Is that right? If so that implies addon-compat for all interface changes :)

Comment 16

[(no longer active)]

(In reply to Brian R. Bondy [:bbondy] from comment #15)
> That brings me to another question... I was told to always update the IID of
> every interface I change. Is that right? If so that implies addon-compat for
> all interface changes :)

Yes.  However, adding a constant doesn't change the vtable layout, so maybe we shouldn't change the IID in this case?  Benjamin would know best.

Comment 17

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a5eb5183ed9

Comment 18

[Benjamin Smedberg]

It is not necessary to rev the IID for just a new constant, but I don't think it would hurt anyone.

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4a5eb5183ed9

Comment 20

[Eric Shepherd [:sheppy]]

This documentation has already been updated.