453256 - Reallocate accesskeys in prefpanes with tabs now that they're independent

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect, P4)

Description

[Phil Ringnalda (:philor)]

Prior to the fix for bug 143065, accesskeys in the Display, Composition, Privacy, and Advanced prefpanes were shared across all tabs, so that if Advanced/General used "A" for "Automatically..." then Advanced/Update couldn't use A. That's resulted in accesskey choices that sometimes look like we'd never seen http://developer.mozilla.org/En/XUL_Accesskey_FAQ_and_Policies and other times like we had, and were trying our best to do the opposite. Now that we aren't stuck having to use too few letters for too many controls, we should improve things, ideally after we've gotten rid of prefs we're dropping from the UI so we don't compromise to give a good letter to something that's going away.

Comment 1

[Simon Paquet [:sipaq]]

Attachment: Patch v1

This is simple enough, so that I can take it. Maybe we can even get this in for beta2.

Comment 2

[Phil Ringnalda (:philor)]

Comment on attachment 360467 [details] [diff] [review]
Patch v1

Generally, omg so much nicer.

The accesskey faq probably ought to have an ordered list instead of unordered, since the rules are certain to conflict. And since a sanserif font isn't unusual on Linux (or Mac, but we don't actually show accesskeys there), the "no i or l" rule really needs to be higher on the list.

> <!-- Display and Reading Settings -->

> <!ENTITY markAsReadNoDelay.label       "Immediately on display">
>-<!ENTITY markAsReadNoDelay.accesskey   "d">
>+<!ENTITY markAsReadNoDelay.accesskey   "I">

"o" to avoid the I?

> <!-- Update -->
> <!ENTITY autoCheck.label                "Automatically check for updates to:">
> <!ENTITY enableAppUpdate.label          "&brandShortName;">
> <!ENTITY enableAppUpdate.accesskey      "h">
> <!ENTITY enableAddonsUpdate.label       "Installed Add-ons">
>-<!ENTITY enableAddonsUpdate.accesskey   "t">
>+<!ENTITY enableAddonsUpdate.accesskey   "I">
> <!ENTITY whenUpdatesFound.label         "When updates to &brandShortName; are found,">
> <!ENTITY modeAskMe.label                "Ask me what I want to do">
>-<!ENTITY modeAskMe.accesskey            "k">
>+<!ENTITY modeAskMe.accesskey            "A">

Using "A" for Installed Add-ons and "m" for Ask me not only avoids the I, it'll probably also match what Firefox will probably do when Mano probably gets around to reviewing bug 303634, which is nice when we can.

>+++ b/mail/locales/en-US/chrome/messenger/preferences/compose.dtd
> <!ENTITY inline.label                         "Inline">
> <!ENTITY inline.accesskey                     "I">
> <!ENTITY asAttachment.label                   "As Attachment">
> <!ENTITY asAttachment.accesskey               "A">

Not your fault, but those are probably the two silliest accesskeys I've ever seen. Is your fault, you're (sensibly) using A for something else in this same pane. Please make that work by removing these two accesskeys, like the person who switched from suite's radiobuttons to Tb's menulist should have done when they switched.

Comment 3

[Phil Ringnalda (:philor)]

wladow: we both know you'll spot something that I'll feel foolish for having missed as soon as I say r+ on the next patch - want to spot it before I do, instead? :)

Comment 4

[Simon Paquet [:sipaq]]

Attachment: Patch v2

Thanks for the review, Phil. I missed the paragraph about not using "i" or "l" as an accesskey.

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 361133 [details] [diff] [review]
Patch v2

Looks good, thanks.

Comment 6

[Vlado Valastiak [:wladow]]

Comment on attachment 361133 [details] [diff] [review]
Patch v2

(In reply to comment #3)
> wladow: we both know you'll spot something that I'll feel foolish for having
> missed as soon as I say r+ on the next patch - want to spot it before I do,
> instead? :) 

Heh, I'm back home from FOSDEM now, looking at the r+ patch and guess what... :) But you don't have to feel foolish for it, really ;)  

> <!ENTITY Diskspace "Disk Space">
> <!ENTITY offlineCompact.label "Compact folders when it will save over">
>-<!ENTITY offlineCompact.accesskey "v">
>+<!ENTITY offlineCompact.accesskey "f">
> <!ENTITY kb.label "KB">

The 'f' isn't the best solution here, try to use a wider character, like 'a' or 'd'

Comment 7

[Simon Paquet [:sipaq]]

http://hg.mozilla.org/comm-central/rev/497ca78a30f5 pushed.

Thanks for the reviews, guys!