Make use of currently unused certManager.dtd access key strings

RESOLVED FIXED in Firefox 38

Status

--
trivial
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

unspecified
mozilla38

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8560844 [details] [diff] [review]
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
Attachment #8560844 - Flags: review?(dkeeler)
(Assignee)

Comment 2

4 years ago
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 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!
Attachment #8560844 - Flags: review?(dkeeler) → review?(jjones)
Sure. I'll get to this one first thing in the morning.
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
Attachment #8560844 - Flags: review?(jjones) → review+
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Thanks for the review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c65c11882c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73ff82f8207e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(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…
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whoops; meaning `certmgr.hierarchy.accesskey` becoming `certmgr.hierarchy.accesskey2` ?

Not sure how I missed that. Cykesiopka - was that intentional?
Flags: needinfo?(cykesiopka.bmo)
(Assignee)

Comment 12

4 years ago
(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
Flags: needinfo?(cykesiopka.bmo)
(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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

4 years ago
(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).
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.