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

RESOLVED FIXED in mozilla9

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: bbondy)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla9
x86
Windows 7
addon-compat, dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

7 years ago
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.
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.
OS: Mac OS X → Windows 7
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Comment 3

6 years ago
Created attachment 558374 [details] [diff] [review]
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.
Attachment #558374 - Flags: review?(tellrob)
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().
(Assignee)

Comment 5

6 years ago
Created attachment 558458 [details] [diff] [review]
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.
Attachment #558374 - Attachment is obsolete: true
Attachment #558458 - Flags: review?(tellrob)
Attachment #558374 - Flags: review?(tellrob)
Attachment #558458 - Flags: review?(tellrob) → review+
(Assignee)

Updated

6 years ago
No longer blocks: 603190
(Assignee)

Comment 6

6 years ago
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.
(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... :/
(Assignee)

Comment 8

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

Would it be better for me to mark this task as RESOLVED | INVALID ?
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...
Keywords: dev-doc-needed
(Assignee)

Comment 10

6 years ago
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.
(Assignee)

Updated

6 years ago
Keywords: addon-compat
(Assignee)

Comment 11

6 years ago
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.
Attachment #558458 - Flags: superreview?(roc)
Attachment #558458 - Flags: superreview?(roc) → superreview+
Why does this affect add-on compatibility?
(Assignee)

Comment 13

6 years ago
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.
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!  :-)
(Assignee)

Comment 15

6 years ago
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 :)
(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.
(Assignee)

Comment 17

6 years ago
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a5eb5183ed9
It is not necessary to rev the IID for just a new constant, but I don't think it would hurt anyone.
http://hg.mozilla.org/mozilla-central/rev/4a5eb5183ed9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
This documentation has already been updated.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.