399048 - Add accesskeys to all buttons, make Get Certificate the default button (Add Exception dialog)

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert [:KaiE:]]

We must add accesskey to all buttons in the new dialog that we added with bug 387480.

Bob proposed the default button should be the "get cert" button.

Requesting blocking1.9 as this is a usability issue with the new UI.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

This implements the access keys.

I've tried to change the default button to be "get certs", but that doesn't work (enter does not trigger it, it's not highlighted).

I suppose it's not possible to have the default button to be one of those we're adding in the middle of the content.

I suppose the default can only be one of these buttons:
- cancel
- confirm
(or none)

If it's not possible to make "get cert" the default, I propose we stay with "no default".

Comment 2

[Johnathan Nightingale [:johnath]]

Comment on attachment 284022 [details] [diff] [review]
Patch v1

>Index: mozilla/security/manager/pki/resources/content/exceptionDialog.xul
>         buttonlabelextra1="&exceptionMgr.exceptionButton;"
>+        buttonaccesskeyextra1="&exceptionMgr.exceptionButton;"
>         style="width: 500px; height: 480px;"

I suspect you mean "&exceptionMgr.exceptionButtonAccess;" here...

>-        defaultButton="null">
>+        defaultButton="checkCertButton">

I agree that if this doesn't do the trick, we should leave it as-is.  Though it would be nice to add a keyboard listener to the textfield to at least submit-on-enter-keypress.  Needn't be handled in this bug, of course.

>Index: mozilla/security/manager/locales/en-US/chrome/pippki/certManager.dtd
>===================================================================
>+<!ENTITY exceptionMgr.exceptionButtonAccess   "C">
>+<!ENTITY exceptionMgr.certlocation.accesskey  "G">
>+<!ENTITY exceptionMgr.certstatus.accesskey    "V">

Is it deliberate that you're choosing the uppercase letters here?  My understanding is that the access key semantics try matching case first, then opposite case. That would seem to argue that these should be "c", "g" and "v" unless those are already in use here, which I don't think is the case (perhaps I'm mistaken?)

Looks good to me with those changes, so I'm +'ng the review, though I'm not sure I can officially give r+ in this component, if it's needed.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> I suppose it's not possible to have the default button to be one of those
> we're adding in the middle of the content.

That's right. The defaultButton must be one of the dialog widget's button, which can be either one of the standard dialog buttons (OK, Cancel) or those created based on the "button*extra" attributes.

defaultButton="null" is unneeded, "null" has no special meaning here. It's identical to an omitted attribute.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Patch v2

(In reply to comment #2)
> >+        buttonaccesskeyextra1="&exceptionMgr.exceptionButton;"
> 
> I suspect you mean "&exceptionMgr.exceptionButtonAccess;" here...

Oops, right!
Changed


> >-        defaultButton="null">
> >+        defaultButton="checkCertButton">
> 
> I agree that if this doesn't do the trick, we should leave it as-is.

It doesn't do the trick.
Based on Gavin's comment, I'm removing that statement altogether.


> Though it
> would be nice to add a keyboard listener to the textfield to at least
> submit-on-enter-keypress.  Needn't be handled in this bug, of course.

Ok, I won't do it now.
I guess this would require a tricky interaction with current-focus, because when the focus is on a different button, it seems, people expect that enter triggers the focused button.

I think "doing nothing on enter"
is better than "doing something unexpected on enter".


> >Index: mozilla/security/manager/locales/en-US/chrome/pippki/certManager.dtd
> >===================================================================
> >+<!ENTITY exceptionMgr.exceptionButtonAccess   "C">
> >+<!ENTITY exceptionMgr.certlocation.accesskey  "G">
> >+<!ENTITY exceptionMgr.certstatus.accesskey    "V">
> 
> Is it deliberate that you're choosing the uppercase letters here?  My
> understanding is that the access key semantics try matching case first, then
> opposite case. That

My intention was to assign the accesskeys to the uppercase letters that are used in the first word. I thought that's a good idea.

Would you prefer to pick a lowercase letter from the middle of the word?

Comment 5

[Johnathan Nightingale [:johnath]]

(In reply to comment #4)
> > Is it deliberate that you're choosing the uppercase letters here?  My
> > understanding is that the access key semantics try matching case first, then
> > opposite case. That
> 
> My intention was to assign the accesskeys to the uppercase letters that are
> used in the first word. I thought that's a good idea.
> 
> Would you prefer to pick a lowercase letter from the middle of the word?

Whoops - actually no, I just wasn't looking at the dialog when I made that comment, and got confused - I think what you have is fine.

Comment 6

[Kai Engert [:KaiE:]]

see also bug 399234, where Steffen works on an overlapping patch

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v3

I merged Steffen's code from bug 399234 into this patch.

Comment 9

[Steffen Wilberg]

From reading _hitEnter, _doButtonCommand and _fireButtonEvent in dialog.xml, instead of the onkeypress handler on the textbox and the oncommand attribute at the checkCertButton button, adding |ondialogextra2="checkCert();"| to the dialog element should work as well but cleaner.
But I can't test this right now.

Comment 10

[Johnathan Nightingale [:johnath]]

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

>+function handleKeyPress(aEvent) {
>+  var checkCertButton = document.getElementById('checkCertButton');
>+  if (!checkCertButton.disabled && aEvent.keyCode == KeyEvent.DOM_VK_RETURN)
>+    checkCertButton.click();
>+}

This all looks great to me, thanks for doing the work!  mxr is not unanimous on whether to check KeyEvent.DOM_VK_RETURN or KeyEvent.DOM_VK_ENTER or both, but the majority side with your current approach.  Once again, I dunno if my review is binding, but it seems like a pretty straightforward (and useful!) change.

Comment 11

[Kai Engert [:KaiE:]]

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

Should someone with more experience in the JS key handling domain do a superreview of this patch? Maybe gavin?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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

I've tested Steffen's approach in comment 9, and it seems to work fine. It has the advantage of not having to do your own event handling, instead using the code that already exists (and has been tested across platforms, etc) in dialog.xml.

Comment 13

[Steffen Wilberg]

Attachment: patch v4

Implements the idea from comment 9.

Comment 14

[Kai Engert [:KaiE:]]

checked in, marking fixed.