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

RESOLVED FIXED

Status

Core Graveyard
Security: UI
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: kaie, Assigned: Steffen Wilberg)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
Flags: blocking1.9?
(Reporter)

Comment 1

11 years ago
Created attachment 284022 [details] [diff] [review]
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".
Attachment #284022 - Flags: review?(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.
Attachment #284022 - Flags: review?(johnath) → review+
(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.
(Reporter)

Comment 4

11 years ago
Created attachment 284171 [details] [diff] [review]
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?
Attachment #284022 - Attachment is obsolete: true
(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.

(Assignee)

Updated

11 years ago
Attachment #284171 - Attachment is patch: true
Attachment #284171 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 6

11 years ago
see also bug 399234, where Steffen works on an overlapping patch
(Reporter)

Updated

11 years ago
Duplicate of this bug: 399234
(Reporter)

Updated

11 years ago
Summary: Add accesskeys to all buttons in Add-Exception-Dialog → Add accesskeys to all buttons, make Get Certificate the default button (Add Exception dialog)
(Reporter)

Comment 8

11 years ago
Created attachment 284226 [details] [diff] [review]
Patch v3

I merged Steffen's code from bug 399234 into this patch.
Attachment #284171 - Attachment is obsolete: true
Attachment #284226 - Flags: review?(johnath)
(Assignee)

Comment 9

11 years ago
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 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.
Attachment #284226 - Flags: review?(johnath) → review+
(Reporter)

Comment 11

11 years ago
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?
Attachment #284226 - Flags: superreview?(gavin.sharp)
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.
Attachment #284226 - Flags: superreview?(gavin.sharp) → superreview-
(Assignee)

Comment 13

11 years ago
Created attachment 284367 [details] [diff] [review]
patch v4

Implements the idea from comment 9.
Assignee: kengert → steffen.wilberg
Attachment #284226 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #284367 - Flags: review?(gavin.sharp)
Attachment #284367 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

11 years ago
Attachment #284367 - Flags: approval1.9?

Updated

11 years ago
Attachment #284367 - Flags: approval1.9? → approval1.9+
(Reporter)

Comment 14

11 years ago
checked in, marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Keywords: access
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.