Last Comment Bug 256915 - help doesn't appear (seamonkey-based dialogs)
: help doesn't appear (seamonkey-based dialogs)
Status: VERIFIED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Help Documentation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird1.0
Assigned To: Scott MacGregor
:
:
Mentors:
: 257981 (view as bug list)
Depends on:
Blocks: 361683
  Show dependency treegraph
 
Reported: 2004-08-25 15:25 PDT by sairuh (rarely reading bugmail)
Modified: 2012-06-20 03:06 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
simple fix to hide help button in all dialog (698 bytes, patch)
2004-11-11 14:42 PST, Scott MacGregor
no flags Details | Diff | Splinter Review
Show the help button is ondialoghelp is defined (749 bytes, patch)
2006-10-20 09:28 PDT, Paul Tomlin
mscott: review-
Details | Diff | Splinter Review
add missing @id to pref-editdirectories.xul (710 bytes, patch)
2006-10-20 14:15 PDT, Paul Tomlin
neil: review+
Details | Diff | Splinter Review
overlay help button and ondialoghelp to mailnews dialogs (6.97 KB, patch)
2006-10-20 14:17 PDT, Paul Tomlin
neil: review+
Details | Diff | Splinter Review
remove help and ondialoghelp attributes from affected files (10.42 KB, patch)
2006-10-20 14:31 PDT, Paul Tomlin
no flags Details | Diff | Splinter Review
remove typo, add jar.mn, add contextHelp.js (7.92 KB, patch)
2006-10-21 00:20 PDT, Paul Tomlin
neil: review+
Details | Diff | Splinter Review
Remove help, ondialoghelp and script=contextHelp from affected dialogs (13.67 KB, patch)
2006-10-21 00:24 PDT, Paul Tomlin
mscott: review+
Details | Diff | Splinter Review
overlay help button and ondialoghelp to mailnews dialogs (v3) (8.57 KB, patch)
2006-10-21 05:58 PDT, Paul Tomlin
no flags Details | Diff | Splinter Review
overlay help button and ondialoghelp to mailnews dialogs (v4) (8.57 KB, patch)
2006-10-22 01:35 PDT, Paul Tomlin
neil: review+
mscott: superreview+
Details | Diff | Splinter Review
remove hack from dialog.xml (777 bytes, patch)
2006-10-23 13:49 PDT, Paul Tomlin
neil: review+
mconnor: superreview+
Details | Diff | Splinter Review

Description sairuh (rarely reading bugmail) 2004-08-25 15:25:30 PDT
in some dialogs in Thunderbird --namely the seamonkey-based ones-- clicking the
Help (or ?) buttons doesn't bring up the Help window (currently doesn't do
anything other than throwing errors in the js console).

I'll list the dialogs affected by this.

Account Settings dialog (Help button)
Error: openHelp is not defined
Source File: chrome://messenger/content/am-help.js
Line: 78

LDAP Directory Servers (Help)
Error: openHelp is not defined
Source File: chrome://messenger/content/addressbook/pref-directory.js
Line: 518

Directory Server Properties (Help)
Error: openHelp is not defined
Source File: chrome://messenger/content/addressbook/pref-directory-add.js
Line: 269

Certificate Manager (Help)
Error: openHelp is not defined
Source File: chrome://pippki/content/certManager.js
Line: 112

Device Manager (Help)
Error: openHelp is not defined
Source File: chrome://global/content/bindings/dialog.xml
Line: 291

will add more in a bit...
Comment 1 sairuh (rarely reading bugmail) 2004-08-25 15:37:14 PDT
Message Filters (Help)
Error: openHelp is not defined

Filter Rules (Help)
Error: openHelp is not defined
Source File: chrome://messenger/content/FilterEditor.js
Line: 729

Customize Message Views (Help)
Error: openHelp is not defined
Source File: chrome://global/content/bindings/dialog.xml
Line: 291

Message View Setup
Error: openHelp is not defined
Source File: chrome://messenger/content/mailViewSetup.js
Line: 128
Comment 2 sairuh (rarely reading bugmail) 2004-08-25 15:54:02 PDT
Message Security dialog (from mail compose)
Error: openHelp is not defined
Source File: chrome://messenger-smime/content/msgCompSecurityInfo.js
Line: 282

Password Manager (from Options > Advanced)
Error: openHelp is not defined
Source File: chrome://communicator/content/wallet/SignonViewer.js
Line: 712

Change Master Password (from Options > Advanced)
Error: openHelp is not defined
Comment 3 Scott MacGregor 2004-09-03 16:06:17 PDT
*** Bug 257981 has been marked as a duplicate of this bug. ***
Comment 4 Yuliy Pisetsky 2004-09-03 19:52:22 PDT
Message security dialog (from either menus in main window or in message reading
window)
Error: openHelp is not defined
Source File: chrome://messenger-smime/content/msgReadSecurityInfo.js
Line: 263

Filter rules dialog (NOT the filter editor dialog)
doesn't show a line/file in bugzilla

Sorry for posting the dupe. (I searched on the S/Mime extension, which I
initially thought was to blame). What should be done about these? Does another
file from seamonkey need to be included into TB? Are these help boxes going to
disappear?
Comment 5 Scott MacGregor 2004-11-11 14:42:56 PST
Created attachment 165624 [details] [diff] [review]
simple fix to hide help button in all dialog

this simple fix addresses all of the dialogs Sarah listed above with the
following EXCEPTIONS:

1) Message Security dialog (from mail compose)
2) Master Password --> Change Password dialog

This is because these old security style dialogs are hard coding the
help/ok/cancel buttons instead of using the generic dialog overlay to get them.
I doubt we'll bother to fix those two dialogs. This gets us almost all of the
way there with little cost.
Comment 6 Scott MacGregor 2004-11-11 14:45:32 PST
fixed branch and trunk
Comment 7 sairuh (rarely reading bugmail) 2004-12-01 16:04:16 PST
everything looks fine, with the exception of the Change Master Password dialog,
as noted in comment 5.
Comment 8 Mike Cowperthwaite 2005-01-10 15:25:07 PST
(In reply to comment #5)
> this simple fix addresses all of the dialogs Sarah listed above with the
> following EXCEPTIONS:
> 
> 1) Message Security dialog (from mail compose)

Bug 268540.
Comment 9 Paul Tomlin 2006-10-20 08:27:10 PDT
Can I propose reopening this on the basis that extension developers may want to include a help button on dialogs within TB but this patch has made it necessary to go the old manual route.

I've done a quick search on mailnews on lxr and find 11 instances of dialogs using help in their buttons attribute. 

http://lxr.mozilla.org/mailnews/search?string=%2Chelp

If I was to submit a patch to #ifdef the help button out of those files and reinstate the possibility of having a standard help button on dialogs, would this be considered?

Are there other cases where a dialog will show a help button that's not included in that list?

The help button can be handled in TB via the ondialoghelp attribute, and since there isn't a standard help facility I don't see this adding any further confusion to users, but making inclusion of help links in extensions slightly easier for authors.
Comment 10 Paul Tomlin 2006-10-20 09:28:23 PDT
Created attachment 242889 [details] [diff] [review]
Show the help button is ondialoghelp is defined

Actually, I think this is simpler. If the XUL defines an ondialoghelp attribute, then show the help button. This should work since it's not going to expect the openHelp function to be available as the handler is overridden.
Comment 11 Scott MacGregor 2006-10-20 09:37:25 PDT
Comment on attachment 242889 [details] [diff] [review]
Show the help button is ondialoghelp is defined

You might want to ask a toolkit peer for review, but even then I'm pretty sure they are not going to want a thunderbird ifdef in dialog.xml.

Hmm...
Comment 12 Mike Cowperthwaite 2006-10-20 10:37:49 PDT
(In reply to comment #11)
> (From update of attachment 242889 [details] [diff] [review] [edit])
> You might want to ask a toolkit peer for review, but even then I'm pretty
> sure they are not going to want a thunderbird ifdef in dialog.xml.

The ifdef is already there...
Comment 13 Paul Tomlin 2006-10-20 13:12:10 PDT
Actually, the ifdef was mscotts' creation to start with, but nevermind, we have another plan.

Neil described to me an overlay, like the security one used elsewhere on SM, to add the help buttons manually. I've worked up an overlay to suit the remaining incidents of ",help" in mailnews, I just need to put a patch together to apply the overlay and remove the ",help" from the dialog/@buttons

The overlay, I believe, will need applying to http://lxr.mozilla.org/mailnews/search?string=%2Chelp and that's what it provides for. 

http://lxr.mozilla.org/mailnews/source/mailnews/addrbook/prefs/resources/content/pref-editdirectories.xul doesn't have an @id, so there's a patch to add one.

mark@standard8 mentioned that if TB is run on top of XUL runner, since MOZ_THUNDERBIRD is not defined there.
Comment 14 Paul Tomlin 2006-10-20 14:15:42 PDT
Created attachment 242916 [details] [diff] [review]
add missing @id to pref-editdirectories.xul

Add the missing id attribute to the one dialog missing it
Comment 15 Paul Tomlin 2006-10-20 14:17:47 PDT
Created attachment 242917 [details] [diff] [review]
overlay help button and ondialoghelp to mailnews dialogs

Once part-1 is out the way, this should seamlessly slot in. Please note these diffs are blind, I have no SM build setup here.
Comment 16 Paul Tomlin 2006-10-20 14:31:27 PDT
Created attachment 242919 [details] [diff] [review]
remove help and ondialoghelp attributes from affected files

Once part-1 and part-2 are in, this should remove the need for the hack in dialog.xml
Comment 17 neil@parkwaycc.co.uk 2006-10-20 15:55:59 PDT
Comment on attachment 242917 [details] [diff] [review]
overlay help button and ondialoghelp to mailnews dialogs

I forgot to point out that extensions/help/resources/jar.mn needs a change.
r=me given that the new file is added to jar.mn and the typo below is fixed.

>+    <RDF:li resource="chrome://messenger-smime/contentmsgReadSecurityInfo.xul"/>
Typo: missing / after content

>+<overlay id="helpMessengerOverlay"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
You could add the contextMenu.js script here and remove it from the dialogs.
Comment 18 neil@parkwaycc.co.uk 2006-10-20 15:58:44 PDT
Comment on attachment 242919 [details] [diff] [review]
remove help and ondialoghelp attributes from affected files

>   <script type="application/x-javascript" src="chrome://help/content/contextHelp.js"/>
This is one of those lines that could be moved to the overlay.

>+        ondialogcancel="return subscribeCancel();">   
Trailing whitespace is frowned upon.

>-        ondialogaccept="return window.close();"
Actually this is the default action for accept so it could just be removed.
Comment 19 Paul Tomlin 2006-10-21 00:20:45 PDT
Created attachment 242959 [details] [diff] [review]
remove typo, add jar.mn, add contextHelp.js
Comment 20 Paul Tomlin 2006-10-21 00:24:57 PDT
Created attachment 242960 [details] [diff] [review]
Remove help, ondialoghelp and script=contextHelp from affected dialogs

Touched up after Neils' comments
Comment 21 neil@parkwaycc.co.uk 2006-10-21 04:43:57 PDT
Comment on attachment 242959 [details] [diff] [review]
remove typo, add jar.mn, add contextHelp.js

r=me with these nits fixed:

>         content/help/helpContextOverlay.xul             (content/helpContextOverlay.xul)
>         content/help/helpSecurityOverlay.xul            (content/helpSecurityOverlay.xul)
> *       content/help/platformClasses.css                (content/platformClasses.css)
>+        content/help/helpMessengerOverlay.xul           (content/helpMessengerOverlay.xul)
Nit: we normally keep these in alphabetical order

>Index: resources/content/helpMessengerOverlay.xul
>===================================================================
>RCS file: resources/content/helpMessengerOverlay.xul
>diff -N resources/content/helpMessengerOverlay.xul
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ resources/content/helpMessengerOverlay.xul	1 Jan 1970 00:00:00 -0000
Nit: this file has been differenced from mozilla/extensions/help but the other two files were differenced from mozilla/extensions/help/resources

>+         
Nit: (trailing) spaces on line
Comment 22 Paul Tomlin 2006-10-21 05:58:32 PDT
Created attachment 242991 [details] [diff] [review]
overlay help button and ondialoghelp to mailnews dialogs (v3)
Comment 23 neil@parkwaycc.co.uk 2006-10-21 09:11:14 PDT
Comment on attachment 242959 [details] [diff] [review]
remove typo, add jar.mn, add contextHelp.js

>+  <script type="application/x-javascript" 
Not having much luck with those trailing spaces are you ;-)
Comment 24 Paul Tomlin 2006-10-22 01:35:11 PDT
Created attachment 243082 [details] [diff] [review]
overlay help button and ondialoghelp to mailnews dialogs (v4)

I tried all the ways I can think of to check there's no trailing whitespace. You will, of cource, find one :D
Comment 25 Scott MacGregor 2006-10-23 12:21:36 PDT
Comment on attachment 243082 [details] [diff] [review]
overlay help button and ondialoghelp to mailnews dialogs (v4)

FYI, the * before an entry in a jar.mn file causes us to run the XUL pre-processor  on the file before it gets put into the JAR file.

So any XUL or JS file that uses an ifdef like

#ifdef MOZ_THUNDERBIRD

#endif

needs an * entry in the jar.mn file for the xul pre-processor to be invoked.

This patch looks good to me. Thanks for working on it. Please get a module owner review for these help changes before checking them in.
Comment 26 Paul Tomlin 2006-10-23 13:49:31 PDT
Created attachment 243250 [details] [diff] [review]
remove hack from dialog.xml

last piece of the puzzle to remove the earlier patch to hide help buttons in TB
Comment 27 Paul Tomlin 2006-10-23 14:05:23 PDT
Comment on attachment 243250 [details] [diff] [review]
remove hack from dialog.xml

trying to find the correct sr for /toolkit/content/widgets/dialog
Comment 28 Mike Connor [:mconnor] 2006-10-23 14:48:11 PDT
Comment on attachment 243250 [details] [diff] [review]
remove hack from dialog.xml

r=me, assuming thunderbird no longer needs this hackaround (I hope not!)
Comment 29 Paul Tomlin 2006-10-23 16:57:04 PDT
Assuming the @id is added
https://bugzilla.mozilla.org/attachment.cgi?id=242916

the overlay is added
https://bugzilla.mozilla.org/attachment.cgi?id=243082

we remove the help and ondialoghelp stuff
https://bugzilla.mozilla.org/attachment.cgi?id=242960

and we finally remove the hack
https://bugzilla.mozilla.org/attachment.cgi?id=243250

all should be well in fairyland

I think I've got all the r+ and sr+ needed. You guys with commmit privs need to do the rest. 

I don't want to keep pumping r=? and sr=? out so someone needs to pick this up from here, I can't commit.
Comment 30 neil@parkwaycc.co.uk 2006-10-24 08:38:29 PDT
OK, I think it's all in, let's hope it all works :-)

Note You need to log in before you can comment on or make changes to this bug.