Closed Bug 938673 Opened 11 years ago Closed 10 years ago

Add context menu to status bar padlock icon

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.28

People

(Reporter: InvisibleSmiley, Assigned: ewong)

References

Details

Attachments

(1 file, 7 obsolete files)

The connectivity icon on the status bar has a context menu (opens upon right mouse click) that includes (among others) a shortcut to the Proxies Preferences pane. Similarly, the padlock icon on the status bar could feature a context menu that would allow to directly open the Certificate Manager.
Attached patch Proposed patch. (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8337476 - Flags: review?(neil)
Comment on attachment 8337476 [details] [diff] [review]
Proposed patch. (v1)

Review of attachment 8337476 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/common/utilityOverlay.xul
@@ +85,5 @@
> +  <menupopup id="padlock-context-menu">
> +    <menuitem  id="padlockCertManager"
> +               label="&padlockCertManager.label;"
> +               accesskey="&padlockCertManager.accesskey;"
> +               oncommand="goPreferences('certs_pane');"/>

That's not the Certificate Manager. ;-) I'm not saying you're doing the wrong thing (it's perfectly debatable whether we should open the pane or the CM directly), but currently what you name and what you do doesn't match.
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Open the *real* Certificate Manager.
Attachment #8337476 - Attachment is obsolete: true
Attachment #8337476 - Flags: review?(neil)
Attachment #8337565 - Flags: review?(neil)
Sorry, I don't understand the use case here. I could possibly understand a comprehensive context menu (e.g. View Security Info, View Certificate, Manage Certificates), or an extra button in Page Info's Security tab to manage certficates, but this just looks like some weird inconsistent UI to me.
(In reply to neil@parkwaycc.co.uk from comment #4)
> Sorry, I don't understand the use case here.

Easy access to directly related functionality that is otherwise hidden behind several steps.

> I could possibly understand a
> comprehensive context menu (e.g. View Security Info, View Certificate,
> Manage Certificates)

I'm not at all opposed to that. It's only that the CM came to my mind first. So, let's see...

* Left mouse button (already) opens Page Info and selects the Security tab.
* Right mouse button should open a context menu, featuring:
  1. "View Security Info" (same as LMB action I take it?)
  2. "View Certificate" (same as Page Info / Security / View Certificate)
  3. Separator (like with the connectivity icon's context menu)
  4. "Manage Certificates" (opens Certificate Manager).

Agreed? Or direct access to the pref pane, too?
> <menupopup id="padlock-context-menu"/>
I think the "padlock" in your patch might be confusing. What if we change the padlock icon for something else in the future? What is the naming convention for other statusbar popup menus?
(In reply to Jens Hatlak from comment #5)
> * Right mouse button should open a context menu, featuring:
>   1. "View Security Info" (same as LMB action I take it?)
Including use of the default attribute, of course.

> Or direct access to the pref pane, too?
Could do, if you want. (That would go last, I take it?)
Comment on attachment 8337565 [details] [diff] [review]
proposed patch (v2)

Cancelling review as per comment #5.
Attachment #8337565 - Attachment is obsolete: true
Attachment #8337565 - Flags: review?(neil)
Attached patch proposed patch - wip. (obsolete) — Splinter Review
Attachment #8340890 - Flags: feedback?(philip.chee)
Comment on attachment 8340890 [details] [diff] [review]
proposed patch - wip.

> --- a/suite/common/utilityOverlay.xul
> +++ b/suite/common/utilityOverlay.xul 
> +  <script type="application/javascript"
> +          src="chrome://navigator/content/pageinfo/security.js"/>

This is wrong. utilityOverlay.xul is used almost everywhere so you're unnecessarily pulling in security.js in all those contexts. Put this somewhere in /browser/ instead.

> +    <menuitem id="padlockSecurityInfo"
> +               label="&padlockSecurityInfo.label;"
> +               accesskey="&padlockSecurityInfo.accesskey;"

"padlock" is wrong. You're describing how the UI looks. Use something that reflects what those items *do*

> +               oncommand="openViewSecurityInfo();"/>
....
> +function openViewSecurityInfo()
> +{
> +  BrowserPageInfo(null, 'securityTab');
> +}

Just use:
oncommand="BrowserPageInfo(null, 'securityTab');"/>

> +               oncommand="security.viewCert();"/>

I don't think this will work.

> +++ b/suite/common/utilityOverlay.js 
> +var gWindow = null;
Oh dear!

> +function onViewSecurityContextMenu()
> +{
> +  const CERTIFICATEDIALOGS_CONTRACTID = "@mozilla.org/nsCertificateDialogs;1"
> +  var cd = Components.classes[CERTIFICATEDIALOGS_CONTRACTID]
> +                     .getService(Components.interfaces.nsICertificateDialogs);
> +  cd.viewCert(window, cert);

I think the above code belongs in function openViewCertificate() ?

> +function openCertManager()
> +{
> +    openDialog("chrome://pippki/content/certManager.xul",
> +               "mozilla:certmanager", "non-private,chrome,titlebar,dialog=no,resizable",

I think "centerscreen,resizable=yes,dialog=no" is adequate. chrome and titlebar default to on.

> +  var cert = null;
> +  var viewCertificateItem = document.getElementById("padlockCertificate");
> +  gWindow = gBrowser.contentWindow;

Don't create new globals if unnecessary. See if you could stick gBrowser.contentWindow on the security object.

> +  var info = security._getSecurityInfo();
> +  if (!info)
> +    alert("bamboozle");
> +  if (info)
> +    alert("booo");
> +  if (!cert)
> +    viewCertificateItem.disabled = true;
> +
> +  // cd.viewCert(parent, cert);
> + 
> +}

If you have info you have info.cert:

  var info = security._getSecurityInfo();
  var viewCertificateItem = document.getElementById("padlockCertificate");
  var isDisabled = !info || !info.cert;
  viewCertificateItem.disabled = isDisabled;

This *almost* works. A slight tweak to security.js is needed.

calling _getSecurityInfo() here generates the following error:

Thu Dec 12 2013 23:48:49
Error: TypeError: invalid 'in' operand window.opener
Source file: chrome://navigator/content/pageinfo/security.js
Line: 82
Attachment #8340890 - Flags: feedback?(philip.chee) → feedback-
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8340890 - Attachment is obsolete: true
Attachment #8349136 - Flags: feedback?(philip.chee)
Comment on attachment 8349136 [details] [diff] [review]
proposed patch (v2)

Review of attachment 8349136 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by: (also, you could align some dots as is common practice throughout our code, and delete the empty line before the onViewSecurityContextMenu block end.

[No need to publish a new patch until Philip came back with his feedback]

::: suite/common/utilityOverlay.js
@@ +1835,5 @@
> +  if (cert)
> +  {
> +    var cd = Components.classes["@mozilla.org/nsCertificateDialogs;1"]
> +                         .getService(Components.interfaces.nsICertificateDialogs);
> +    cd.viewCert(parent, cert);

Uh-oh, we're missing a parent! The child will be very unhappy!

More seriously, I think you can directly put "window" here instead.
Comment on attachment 8349136 [details] [diff] [review]
proposed patch (v2)

This context menu only makes sense in a browser environment so please move all the stuff in utilityOverlay.xul to navigator.xul and utilityOverlay.js to somewhere suitable in /suite/browser/

+++ b/suite/browser/navigator.xul
+    <menupopup id="security-context-menu"/>

> +++ b/suite/common/utilityOverlay.xul
> +  <menupopup id="security-context-menu"
> +             onpopupshowing="onViewSecurityContextMenu();">
> +    <menuitem id="viewSecurityInfo"
> +               label="&viewSecurityInfo.label;"
> +               accesskey="&viewSecurityInfo.accesskey;"
> +               oncommand="BrowserPageInfo(null, 'securityTab');"/>
> +    <menuitem id="viewCertificate"
> +               label="&viewCertificate.label;"
> +               accesskey="&viewCertificate.accesskey;"
> +               oncommand="openViewCertificate();"/>
I think "viewCertificate();" is sufficient.

> +    <menuseparator/>
> +    <menuitem  id="viewCertManager"
> +               label="&viewCertManager.label;"
> +               accesskey="&viewCertManager.accesskey;"
> +               oncommand="openCertManager(;)"/>
Oops, syntax error: "openCertManager();"

> +  </menupopup>
merge into navigator.xul

> +function onViewSecurityContextMenu()
> +{
> +  document.getElementById("viewCertificate").disabled = !getCert();
> +
> +}

We can reduce code duplication here by noticing that the security-button has an attribute of level="high" only if the current page has a cert. Thus:

function onViewSecurityContextMenu()
{
  var level = document.getElementById("security-button").getAttribute("level");
  document.getElementById("viewCertificate").disabled = level != "high";
}

See: http://hg.mozilla.org/comm-central/annotate/b55d96af36a4/suite/browser/nsBrowserStatusHandler.js#l380

> +function getCert()
Merge this into openViewCertificate()

> +function openViewCertificate()
...
> +  if (cert)
openViewCertificate() is callable only if we have a certificate.

> +  {
> +    var cd = Components.classes["@mozilla.org/nsCertificateDialogs;1"]
> +                         .getService(Components.interfaces.nsICertificateDialogs);
> +    cd.viewCert(parent, cert);
As Jens says  parent shuold be window.

> +    openDialog("chrome://pippki/content/certManager.xul",
> +               "mozilla:certmanager", "non-private,chrome,titlebar,dialog=no,resizable",
This should be enough: "non-private,chrome,all,dialog=no"

> +               null);
Attachment #8349136 - Flags: feedback?(philip.chee)
Attached patch proposed patch(v3) (obsolete) — Splinter Review
Attachment #8349136 - Attachment is obsolete: true
Attachment #8358769 - Flags: review?(neil)
Comment on attachment 8358769 [details] [diff] [review]
proposed patch(v3)

>+    var cd = Components.classes["@mozilla.org/nsCertificateDialogs;1"]
>+                       .getService(Components.interfaces.nsICertificateDialogs);
>+    cd.viewCert(window, sslStats.serverCert);
Could write this as
Components.classes["@mozilla.org/nsCertificateDialogs;1"]
          .getService(Components.interfaces.nsICertificateDialogs)
          .viewCert(window, sslStats.serverCert);

>+function openCertManager()
>+{
Cert manager might already be open, so use toOpenWindowByType (the mozilla:certmanager doesn't belong in the openDialog name parameter anyway)

>+    openDialog("chrome://pippki/content/certManager.xul",
>+               "mozilla:certmanager", "non-private,chrome,all,dialog=no",
Wrong features, everyone else uses resizable,dialog=no,centerscreen

>+function onViewSecurityContextMenu()
>+{
>+  var level = document.getElementById("security-button")
>+                      .getAttribute("level");
>+  document.getElementById("viewCertificate").disabled = level != "high";
Not guaranteed to work, certificate could be invalid, or page could have mixed content (previous version was better)

>+<!ENTITY viewCertManager.label       "Open Certificate Manager">
>+<!ENTITY viewCertManager.accesskey   "O">
I think "M" might be better.
Comment on attachment 8349136 [details] [diff] [review]
proposed patch (v2)

>+  var sslStats = getBrowser().securityUI
>+                         .QueryInterface(Components.interfaces.nsISSLStatusProvider)
>+                         .SSLStatus;
What happened to the u?

>+  var cert = null;
>+
>+  if (sslStats)
>+    cert = sslStats.serverCert;
>+
>+  return cert;
Just return sslStatus && sslStatus.serverCert;
(In reply to neil@parkwaycc.co.uk from comment #16)
> Comment on attachment 8349136 [details] [diff] [review]
> proposed patch (v2)
> 
> >+  var sslStats = getBrowser().securityUI
> >+                         .QueryInterface(Components.interfaces.nsISSLStatusProvider)
> >+                         .SSLStatus;
> What happened to the u?

It was on purpose that I used sslStats.  I'll change it to sslStatus.
Attached patch proposed patch(v4) (obsolete) — Splinter Review
Attachment #8358769 - Attachment is obsolete: true
Attachment #8358769 - Flags: review?(neil)
Attachment #8359009 - Flags: review?(neil)
Attachment #8359009 - Flags: review?(neil)
Attached patch proposed patch(v4) (obsolete) — Splinter Review
Attachment #8359009 - Attachment is obsolete: true
Attachment #8359011 - Flags: review?(neil)
Comment on attachment 8359011 [details] [diff] [review]
proposed patch(v4)

>+  toOpenWindowByType("mozilla:certmanager", "chrome://pippki/content/certManager.xul");
Missing features string. r=me with that fixed.

>+      <menuitem id="viewSecurityInfo"
default="true" perhaps?
Attachment #8359011 - Flags: review?(neil) → review+
ewong: Ping (I think we have everything we need here?)
Flags: needinfo?(ewong)
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #21)
> ewong: Ping (I think we have everything we need here?)

will need to ask for Callek's push-to-trunk approval.
Flags: needinfo?(ewong)
Attachment #8359011 - Attachment is obsolete: true
Attachment #8401074 - Flags: review+
Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/9d7718c69e6a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 992561
Target Milestone: --- → seamonkey2.28
You need to log in before you can comment on or make changes to this bug.