Closed
Bug 1130402
Opened 9 years ago
Closed 9 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•9 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•9 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)
Comment 4•9 years ago
|
||
Sure. I'll get to this one first thing in the morning.
Comment 5•9 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•9 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•9 years ago
|
||
Thanks for the review. https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c65c11882c
Keywords: checkin-needed
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ff82f8207e
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/73ff82f8207e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 10•9 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•9 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•9 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•9 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: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•9 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•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•