1130402 - Make use of currently unused certManager.dtd access key strings

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[:Cykesiopka]

There are some currently unused access key strings sitting in certManager.dtd that can be put to use. No point in not using them if they already exist.

Comment 1

[:Cykesiopka]

Attachment: bug1130402_use-unused-certManager.dtd-accesskey-strings_v1.patch

This patch allows some of the currently unused access key strings to be put to use.

Included in this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c280971b9d7

Comment 2

[:Cykesiopka]

Comment on attachment 8560844 [details] [diff] [review]
bug1130402_use-unused-certManager.dtd-accesskey-strings_v1.patch

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

::: security/manager/locales/en-US/chrome/pippki/certManager.dtd
@@ +90,5 @@
>  <!ENTITY certmgr.details.accesskey            "F">
>  <!ENTITY certmgr.fields.label                 "Field Value">
>  <!ENTITY certmgr.fields.accesskey             "V">
>  <!ENTITY certmgr.hierarchy.label              "Certificate Hierarchy">
> +<!ENTITY certmgr.hierarchy.accesskey2         "H">

This is used for the cert viewer.
However, "C" is already used for the close button, so I changed the access key.

::: security/manager/pki/resources/content/editemailcert.xul
@@ +31,5 @@
>      <radio label="&certmgr.editemailcert.donttrust;" value="false"/>
>    </radiogroup>
>    <hbox>
>      <button id="editca-button" label="&certmgr.editca.label;"
> +            accesskey="&certmgr.editca.accesskey;" oncommand="editCaTrust();"/> 

I'll remove the trailing whitespace in the next patch revision.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8560844 [details] [diff] [review]
bug1130402_use-unused-certManager.dtd-accesskey-strings_v1.patch

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

JC, I'd like if you reviewed this as well. Maybe give it a quick compile/run and see if the shortcut keys work as expected. Thanks!

Comment 4

[J.C. Jones [:jcj] (he/they)]

Sure. I'll get to this one first thing in the morning.

Comment 5

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8560844 [details] [diff] [review]
bug1130402_use-unused-certManager.dtd-accesskey-strings_v1.patch

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

So I believe this all works fine; I tried all the shortcut keys in certManager. I did not, however, try the new shortcut in editemailcert because, TBH, I don't know how to populate the "Other Certificates" tab. It seems like something I should know! Anyway, you're good to go. Thanks!

r=jcj

Comment 6

[:Cykesiopka]

(In reply to J.C. Jones [:jcj] from comment #5)
> So I believe this all works fine; I tried all the shortcut keys in
> certManager. I did not, however, try the new shortcut in editemailcert
> because, TBH, I don't know how to populate the "Other Certificates" tab. It
> seems like something I should know! Anyway, you're good to go. Thanks!

If you like, you can try the script (in patch form) I uploaded here: attachment 8556925 [details] [diff] [review]
It generates and imports example certs that will show up in the usually empty tabs in the cert manager.

> cd security/manager/ssl/tests/unit/exampleCerts/
> ./generateAndImport.py "<path to profile dir>"
should hopefully work, assuming you have openssl, certutil etc installed.

Comment 7

[:Cykesiopka]

Thanks for the review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c65c11882c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ff82f8207e

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73ff82f8207e

Comment 10

[Stefan Plewako [:stef]]

(In reply to Carsten Book [:Tomcat] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/73ff82f8207e

This patch shouldn't change certmgr.hierarchy.accesskey entity id…

Comment 11

[J.C. Jones [:jcj] (he/they)]

Whoops; meaning `certmgr.hierarchy.accesskey` becoming `certmgr.hierarchy.accesskey2` ?

Not sure how I missed that. Cykesiopka - was that intentional?

Comment 12

[:Cykesiopka]

(In reply to J.C. Jones [:jcj] from comment #11)
> Whoops; meaning `certmgr.hierarchy.accesskey` becoming
> `certmgr.hierarchy.accesskey2` ?
> 
> Not sure how I missed that. Cykesiopka - was that intentional?

Yes, this was intentional:
 - It is to my understanding that string changes like these are supposed to bump the key/entity ID so that the change shows up on l10n tools
   - I don't know if access keys are exempt to this or not
 - In any case, it was also my intention to bump the ID in the hopes that translators will double check that their previous access keys didn't overlap, as was the en-US case

Comment 13

[Stefan Plewako [:stef]]

(In reply to Cykesiopka from comment #12)
> Yes, this was intentional:
>  - It is to my understanding that string changes like these are supposed to
> bump the key/entity ID so that the change shows up on l10n tools
>    - I don't know if access keys are exempt to this or not
>  - In any case, it was also my intention to bump the ID in the hopes that
> translators will double check that their previous access keys didn't
> overlap, as was the en-US case

Changing letter assigned to accesskey shouldn't affect other languages and therefore shouldn't end with updated id - whenever or not there is overlap of accesskey in some of ~100 other locales it should have nothing to do with en-US (values should be different/translated).

If it is one intention to tell localizers to double check some part of UI then posting on mozilla.dev.l10n would be the best.

Comment 14

[:Cykesiopka]

(In reply to Stefan Plewako [:stef] from comment #13)
> Changing letter assigned to accesskey shouldn't affect other languages and
> therefore shouldn't end with updated id - whenever or not there is overlap
> of accesskey in some of ~100 other locales it should have nothing to do with
> en-US (values should be different/translated).
> 
> If it is one intention to tell localizers to double check some part of UI
> then posting on mozilla.dev.l10n would be the best.

Noted, thanks. I'll keep that in mind in the future.

(Although I'm also fine with making a follow-up patch to revert the ID change).