473444 - Help opens, but with a beep with cmd+? in prefwindow without overlayed/own keys

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Stefan [:stefanh]]

Attachment: demo patch

I think this is a cocoa bug, but I thought it might be necessary to work around it at this stage...

STR:

1) Apply demo patch and build
2) Start Firefox and open the preference window
3) Hit cmd+?

What happens: A system "beep" and Help opens
What should happen: Help opens without any beep

This is not an issue in Firefox, but it will be an issue for any consumer that doesn't make use of an overlay with the help keys specified in it.

One possible solution is to use preventDefault on the handler - that seems to get rid of the beep. But then there won't be any beep in a pane that lacks a help button, of course.

Comment 1

[jag (Peter Annema)]

Related to bug 456311 / fall-out from bug 398514? Likely if you get the beep on 10.4 but not on 10.5.

Comment 2

[Stefan [:stefanh]]

(In reply to comment #1)
> Related to bug 456311 / fall-out from bug 398514? Likely if you get the beep on
> 10.4 but not on 10.5.

Sorry, forgot to say that I'm on 10.5.

Comment 3

[Stefan [:stefanh]]

fwiw, as it is now it doesn't works very well if you use a mac-specific overlay as Firefox does. It appears that opening preferences and hitting cmd+?, will open 2 Help windows (yep, one pane-specific and one general). Obviously, the key in the menu gets triggered as well.

jag suggested to use stopPropagation and preventDefault on the handler if the help button is visible and not disabled. I'll do that. preventDefault only will fix the issues, but it shouldn't hurt using stopPropagation as well since this whole thing feels a bit fragile.

Comment 4

[Stefan [:stefanh]]

Btw, jag - would you consider this (the beep) a cocoa bug?

Comment 5

[Stefan [:stefanh]]

Attachment: Fix beep and make only one instance of Help open with cmd+? in prefs (pushed)

OK, this should fix both the beeping and the 2 instances of Help. Not sure if nix/win wants/needs this. If not, I guess I could ifdef it to be mac-only.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 357370 [details] [diff] [review]
Fix beep and make only one instance of Help open with cmd+? in prefs (pushed)

>+++ b/toolkit/content/widgets/preferences.xml

>+        var helpButton = document.documentElement.getButton("help");
>+        if (helpButton.disabled || helpButton.hidden)
>+          return;

I'm not so sure about this change... what's the reasoning behind it? I don't think we should avoid firing the event just because the button isn't there, or is disabled. The other part looks good.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 357370 [details] [diff] [review]
Fix beep and make only one instance of Help open with cmd+? in prefs (pushed)

OK, I misunderstood how the help button actually worked. This is fine as is.

Comment 8

[Stefan [:stefanh]]

http://hg.mozilla.org/mozilla-central/rev/0689654a46ec (will ask for a191 after a short bake).

Comment 9

[[e-mail]]

Comment on attachment 357370 [details] [diff] [review]
Fix beep and make only one instance of Help open with cmd+? in prefs (pushed)

>+        var helpButton = document.documentElement.getButton("help");
>+        if (helpButton.disabled || helpButton.hidden)
>+          return;
>         this._fireEvent("dialoghelp", this);
this.getButton("help") also works.

Comment 10

[Stefan [:stefanh]]

Attachment: Use 'this' instead per comment #9 (pushed)

Sorry, should have thought about it in the first place...

Comment 11

[Stefan [:stefanh]]

Comment on attachment 357370 [details] [diff] [review]
Fix beep and make only one instance of Help open with cmd+? in prefs (pushed)

I'm going to land attachment #357715 [details] [diff] [review] tomorrow.

Comment 12

[Stefan [:stefanh]]

Comment on attachment 357715 [details] [diff] [review]
Use 'this' instead per comment #9 (pushed)

http://hg.mozilla.org/mozilla-central/rev/2cda622c9bd9

Comment 13

[Stefan [:stefanh]]

Attachment: 191 version that combines attachment #357370 and #357715 (pushed to 1.9.1)

Apart from the reported issue (that SeaMonkey suffers from), this also fixes the problem with Firefox opening 2 instances of Help if you hit cmd+? in the prefwindow.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 357841 [details] [diff] [review]
191 version that combines attachment #357370 [details] [diff] [review] and #357715 (pushed to 1.9.1)

a191=beltzner

Comment 15

[Stefan [:stefanh]]

Pushed attachment #357841 [details] [diff] [review] to releases/1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/301c0bb5208a