Closed Bug 473444 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

(Keywords: fixed1.9.1)

Attachments

(4 files)

Attached patch demo patchSplinter Review
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.
Related to bug 456311 / fall-out from bug 398514? Likely if you get the beep on 10.4 but not on 10.5.
(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.
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.
Btw, jag - would you consider this (the beep) a cocoa bug?
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.
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #357370 - Flags: review?(gavin.sharp)
Version: 1.9.1 Branch → Trunk
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 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.
Attachment #357370 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/0689654a46ec (will ask for a191 after a short bake).
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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.
Sorry, should have thought about it in the first place...
Attachment #357715 - Flags: review?(gavin.sharp)
Attachment #357715 - Flags: review?(gavin.sharp) → review+
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.
Attachment #357370 - Attachment description: Fix beep and make only one instance of Help open with cmd+? in prefs → Fix beep and make only one instance of Help open with cmd+? in prefs (pushed)
Comment on attachment 357715 [details] [diff] [review]
Use 'this' instead per comment #9 (pushed)

http://hg.mozilla.org/mozilla-central/rev/2cda622c9bd9
Attachment #357715 - Attachment description: Use 'this' instead (per comment #9) → Use 'this' instead per comment #9 (pushed)
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.
Attachment #357841 - Flags: approval1.9.1?
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
Attachment #357841 - Flags: approval1.9.1? → approval1.9.1+
Attachment #357841 - Attachment description: 191 version (combines attachment #357370 and #357715) → 191 version that combines attachment #357370 and #357715 (pushed to 1.9.1)
You need to log in before you can comment on or make changes to this bug.