657763 - [PATCH] There is an uneeded (F) next to Event in the Event window

Status: RESOLVED FIXED

Description

[Alexandre Demers]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/534.35 (KHTML, like Gecko) Chrome/13.0.762.0 Safari/534.35
Build Identifier: Lightning 1.0b2

When adding, editing or viewing an event, there is a (F) next to the Event menu. It is used for keyboard shortcuts. It is useless and should be replaced by one of the letters in the word "Event". Of course, Alt+E is already assigned to "Edit", but using Alt+F (and adding the text next to the word Event) doesn't make sense. It is both inaesthetic and unnatural since the letter is not in the word.

Reproducible: Always

Steps to Reproduce:
Open an Event window dialog. Look at the menus.

Actual Results:  
There is a (F) next to Event.

Expected Results:  
There shouldn't be any and the keyboard shortcut should use one of the letter in the word Event.

Comment 1

[Alexandre Demers]

I think it uses the "F" from "File" menu, while renaming the menu (label) to "Event". May I suggest:
- adding the menu label in the locales files so it can be translated
- adding the accesskey in the locales files so it can be changed according to the languages

I think two files must be changed:
- locales for label + accesskey for menu name and shortcut
- calendar-event-dialog.xul to remove the fixed label and accesskey so it will use the ones described in the local file.

Am I right to think so?

Comment 2

[Stefan Sitter [:ssitter]]

Yes, this is a leftover from the renaming changes. Previously the menu was labeled "File" using the "F" access key. Now the menu is either label "Event" or "Task" depending on the dialog operation mode. The access key was kept the same because a) all other useful keys are already taken and b) because Lightning users are used to the keyboard shortcuts since years and there is no reason to break it for them.

Both label and access key are already in language dependent files and can be changed by localization teams. http://mxr.mozilla.org/comm-central/source/calendar/locales/en-US/chrome/calendar/calendar-event-dialog.properties#330

Comment 3

[Alexandre Demers]

Thanks for the info, I was looking under ./calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd and was wondering why it was not there. But now I understand why.

I've been using Lightning for many years myself and I've always wondered why the accesskey was never replaced by "T" (This would also be a good candidate since it is in the word "Task"). I've almost never used the accesskey, but when I did, it didn't feel right to use "F".

As I said before, "It is both inaesthetic and unnatural since the letter is not in the word." I think Lightning is the only application I know that propose an accesskey that is not in the menu label (except maybe poorly translate applications that I've came across). I can propose a patch, but does it have any chance to be applied?

Comment 4

[Alexandre Demers]

Created attachment 535941 [details] [diff] [review]
Change accesskey from F to T

Comment 5

[Stefan Sitter [:ssitter]]

Comment on attachment 535941 [details] [diff] [review]
Change accesskey from F to T

T is already used as the access key for the Title input field.

Comment 6

[Alexandre Demers]

(In reply to comment #5)
> Comment on attachment 535941 [details] [diff] [review] [review]
> Change accesskey from F to T
> 
> T is already used as the access key for the Title input field.

Yeah, I missed that one... Could we change also the accesskey for the Title input field? Since this is the first field and it's selected, I think the accesskey must be used less than others, IMHO. It could be mapped to "i", freeing the "t" accesskey.

By the way, talking about other accesskeys, is it normal that "s" and "n" don't do anything even if they are mapped to "Start" and "End"? Is it because of the input type?

Comment 7

[Alexandre Demers]

Created attachment 546793 [details] [diff] [review]
Change title accesskey from T to I

Changing accesskey so T can be mapped to Event and Task correctly (in the first patch attached). As explained previously, since title is the default text input when creating a new Event or Task, changing its accesskey shouldn't be a big deal.

Comment 8

[Martin Schröder [:martinschroeder]]

Comment on attachment 546793 [details] [diff] [review]
Change title accesskey from T to I

Pushed to queue for further consideration and review.

Comment 9

[Alexandre Demers]

The first patch shouldn't have been put as obsolete since both are needed. However, for convenience, I'll make a single patch with both of them.

Comment 10

[Alexandre Demers]

Created attachment 546864 [details] [diff] [review]
[test on linux] Combined patches

Combined patches in a single one for convenience and using mercurial to create it.

Comment 11

[Martin Schröder [:martinschroeder]]

(In reply to comment #9)
> The first patch shouldn't have been put as obsolete since both are needed.
> However, for convenience, I'll make a single patch with both of them.

Sorry for marking the patch obsolete. I thought it has been replaced by the second one. Please, ask for review by setting the appropriate flag on the attachment. How the create a patch in the preferred format is documented at https://developer.mozilla.org/en/Creating_a_patch.

Comment 12

[Alexandre Demers]

It's been almost 5 months since I submitted the patch, just pinging to be sure it has not been lost. Any comment on the patch would be appreciated. Thank you.

Comment 13

[Philipp Kewisch [:Fallen] [:📆]]

Comment on attachment 546864 [details] [diff] [review]
[test on linux] Combined patches

Hey, sorry for the large delay. I'm on my mac very often now where no access keys are shown. I need to look at this from a different machine. I've marked it so that I know what to do next time I'm on my linux box.

Stefan, if you have a moment, could you doublecheck if this variant has no conflicts?

Comment 14

[Philipp Kewisch [:Fallen] [:📆]]

Comment on attachment 546864 [details] [diff] [review]
[test on linux] Combined patches

[Triage Comment]

Tested, looks fine to me. r=philipp

Given we are not actually changing any string keys but merely fixing the en-US locale, I'm going to push this down to comm-aurora.

Comment 15

[Philipp Kewisch [:Fallen] [:📆]]

comm-central changeset 66527bcf0a8a
releases/commm-aurora changeset 17f718dfee56