256915 - help doesn't appear (seamonkey-based dialogs)

Status: VERIFIED FIXED

(Thunderbird :: Help Documentation, defect)

Description

[sairuh (rarely reading bugmail)]

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)]

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)]

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]

*** Bug 257981 has been marked as a duplicate of this bug. ***

Comment 4

[Yuliy Pisetsky]

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]

Attachment: 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]

fixed branch and trunk

Comment 7

[sairuh (rarely reading bugmail)]

everything looks fine, with the exception of the Change Master Password dialog,
as noted in comment 5.

Comment 8

[Mike Cowperthwaite]

(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]

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]

Attachment: 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]

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]

(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]

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]

Attachment: add missing @id to pref-editdirectories.xul

Add the missing id attribute to the one dialog missing it

Comment 15

[Paul Tomlin]

Attachment: 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]

Attachment: 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

[[e-mail]]

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

[[e-mail]]

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]

Attachment: remove typo, add jar.mn, add contextHelp.js

Comment 20

[Paul Tomlin]

Attachment: Remove help, ondialoghelp and script=contextHelp from affected dialogs

Touched up after Neils' comments

Comment 21

[[e-mail]]

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]

Attachment: overlay help button and ondialoghelp to mailnews dialogs (v3)

Comment 23

[[e-mail]]

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]

Attachment: 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]

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]

Attachment: 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]

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]]

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]

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

[[e-mail]]

OK, I think it's all in, let's hope it all works :-)