343395 - Help buttons not displayed on SeaMonkey when built as a xul application (MOZ_XUL_APP=1)

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Mark Banner (:standard8)]

On most of the security dialogs (e.g. certificate manager, security devices, master passowrd dialogs) that are contained within /security/manager/pki/resources/content the help buttons aren't displayed when building SeaMonkey with MOZ_XUL_APP=1 set.

This is due to configuration options in the files that prevent the help buttons being included because firefox didn't have the documentation for them (see bug 289555 comment 9). However, SeaMonkey does have the documentation and wants to keep them.

Rather than change the MOZ_XUL_APP ifdefs to MOZ_SUITE, what I propose is to move the help button definitions (and onhelpbutton attributes) to a separate overlay contained within /suite. This will provide future compatibility if and when SeaMonkey is moved onto xulrunner.

cc'ing Camino folks as I don't know if they will be affected by this change as well.

Comment 1

[Mark Banner (:standard8)]

Attachment: Probable Patch

This is the patch that fixes the help buttons for SeaMonkey by moving all the help button functionality out of security/manager/pki and into an overlay in extensions/help. I want to check a couple of things before I request reviews, but this is probably the one to go with.

Comment 2

[[e-mail]]

Comment on attachment 228301 [details] [diff] [review]
Probable Patch

>Index: security/manager/pki/resources/jar.mn
I expected to see a Makefile.in change to avoid defining __HELP_BUTTON__

>+++ security/manager/pki/resources/content/crlImportDialog.js	6 Jul 2006 17:05:34 -0000
>@@ -1,4 +1,4 @@
>-/* ***** BEGIN LICENSE BLOCK *****
>+* ***** BEGIN LICENSE BLOCK *****
Oops ;-)

>-#ifndef MOZ_XUL_APP
>-        buttons="help"
>-#endif
Weird, how does that work in Firefox, seeing as the lack of a buttons attribute displays ok and cancel by default...


>   spacerflex="1"
>   spacerflex="1"
>   spacerflex="1"
Move the spacerflexes into the overlay, they're xpfe specific.


>+      var selTab = document.getElementById('certMgrTabbox').selectedItem;
>+      var selTabID = selTab.getAttribute('id');
>+      if (selTabID == 'mine_tab') {
>+        key = "my_certs";
>+      } else if (selTabID == "others_tab") {
>+        key = "others_certs";
>+      } else if (selTabID == "websites_tab") {
>+        key = "web_certs";
>+      } else if (selTabID == "ca_tab") {
>+        key = "ca_certs";
>+      }
>+      openHelp(key);
Might as well call openHelp in the conditional block directly, like the function below. Would a switch on the id be neater?

>+      if (document.title == bundle.GetStringFromName("deleteUserCertTitle"))
I think it would be slightly better to switch on gParams.GetString(0) (why those strings are in the .properties I have no idea)

Comment 3

[Mark Banner (:standard8)]

Attachment: Revised patch

This patch incorporates Neil's comments (thanks Neil), and fixes a few other problems.

The dialogs are all the same for SeaMonkey. Firefox and Thunderbird will no longer have ok and cancel buttons on the CRL Manager dialog, just a Close button which I think was the intended functionality as ok & cancel don't actually do anything.

I've a few final checks to make but I think this should be the one...

Comment 4

[[e-mail]]

Comment on attachment 228301 [details] [diff] [review]
Probable Patch

>+
>+  <RDF:Seq about="chrome://pippki/content/escrowWarn.xul">
>+    <RDF:li>chrome://help/content/helpSecurityOverlay.xul</RDF:li>
>+  </RDF:Seq>
>+  <RDF:Seq about="chrome://pippki/content/getp12password.xul">
>+    <RDF:li>chrome://help/content/helpSecurityOverlay.xul</RDF:li>
>+  </RDF:Seq>
>+
Oops, I overlooked the missing blank line here :-[

Comment 5

[Mark Banner (:standard8)]

Attachment: Patch v3

Revised patch fixes missing blank line and also gets the bundle in the js function.

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 228615 [details] [diff] [review]
Patch v3

Thanks for this patch.

r=kengert assuming you have tested it

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> (From update of attachment 228615 [details] [diff] [review] [edit])
> Thanks for this patch.
> 
> r=kengert assuming you have tested it
> 
I've tested it as much as possible and not found any problems (yet ;-) )

Comment 8

[Mark Banner (:standard8)]

Patch checked in.

Comment 9

[[e-mail]]

If stephend's going to be picky about the summary then so will I :-P

Comment 11

[Mahesh Kumar]

(In reply to comment #2)
> (From update of attachment 228301 [details] [diff] [review])
> >Index: security/manager/pki/resources/jar.mn
> I expected to see a Makefile.in change to avoid defining __HELP_BUTTON__
> >+++ security/manager/pki/resources/content/crlImportDialog.js	6 Jul 2006 17:05:34 -0000
> >@@ -1,4 +1,4 @@
> >-/* ***** BEGIN LICENSE BLOCK *****
> >+* ***** BEGIN LICENSE BLOCK *****
> Oops ;-)
> >-#ifndef MOZ_XUL_APP
> >-        buttons="help"
> >-#endif
> Weird, how does that work in Firefox, seeing as the lack of a buttons attribute
> displays ok and cancel by default...
> >   spacerflex="1"
> >   spacerflex="1"
> >   spacerflex="1"
> Move the spacerflexes into the overlay, they're xpfe specific.
> >+      var selTab = document.getElementById('certMgrTabbox').selectedItem;
> >+      var selTabID = selTab.getAttribute('id');
> >+      if (selTabID == 'mine_tab') {
> >+        key = "my_certs";
> >+      } else if (selTabID == "others_tab") {
> >+        key = "others_certs";
> >+      } else if (selTabID == "websites_tab") {
> >+        key = "web_certs";
> >+      } else if (selTabID == "ca_tab") {
> >+        key = "ca_certs";
> >+      }
> >+      openHelp(key);
> Might as well call openHelp in the conditional block directly, like the
> function below. Would a switch on the id be neater?
> >+      if (document.title == bundle.GetStringFromName("deleteUserCertTitle"))
> I think it would be slightly better to switch on gParams.GetString(0) (why
> those strings are in the .properties I have no idea)