Closed
Bug 1130402
Opened 10 years ago
Closed 10 years ago
Make use of currently unused certManager.dtd access key strings
Categories
(Core Graveyard :: Security: UI, defect)
Core Graveyard
Security: UI
Tracking
(firefox38 fixed)
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
Details
Attachments
(1 file)
4.33 KB,
patch
|
jcj
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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•10 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 3•10 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]:
-----------------------------------------------------------------
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)
Comment 4•10 years ago
|
||
Sure. I'll get to this one first thing in the morning.
Comment 5•10 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]:
-----------------------------------------------------------------
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•10 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•10 years ago
|
||
Thanks for the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c65c11882c
Keywords: checkin-needed
Comment 8•10 years ago
|
||
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 10•10 years ago
|
||
(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 → ---
Comment 11•10 years ago
|
||
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•10 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)
Comment 13•10 years ago
|
||
(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
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 14•10 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).
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•