357027 - The 'New event' and 'New Task' windows have no accesskey

Status: RESOLVED WONTFIX

(Calendar :: General, defect)

Description

[Cédric Corazza]

These windows have no accesskey. I expect to provide a patch before the end of this week.

Comment 1

[Cédric Corazza]

Attachment: Patch adding accesskeys for tne New Event and New Task Windows

As most labels are common in both the windows and the numbers of letters are limited, so I had to use some accesskeys that are not recommended ('i' for Title, 't' for Due date --New Event Windows-- and 'y' for Priority).
At least, this is the best combination I found ans there is no accesskey collision.
I also removed from the dtd file 3 entities that were not used.

Comment 2

[cmtalbert]

Comment on attachment 242666 [details] [diff] [review]
Patch adding accesskeys for tne New Event and New Task Windows

R + with my comment about wrapping addressed. Here was our discussion about the patch:
Hi Cedric, I have some comments
	<Cedric>	yes ?
	<ctalbert>	First is a general one, are you familiar with Mozilla's 80 character line rule?
	<ctalbert>	(I ask because I wasn't)
	<Cedric>	nope :")
	<ctalbert>	It's ok.
	<Cedric>	That, I can fix easily
	<ctalbert>	The idea is that they want to limit lines to no more than 80 characters in order to help the readability of things, particularly in bugzilla diffs.
	<Cedric>	understood
	<ctalbert>	So, there's several lines that need to be adjusted there.
	<Cedric>	ok. Something else to fix ?
	<ctalbert>	yes, had to go look it up.
	<ctalbert>	Down in calendar.dtd
	<Cedric>	ok. I'm online for at least 3 hours
	<ctalbert>	You removed ENTITY newevent.category.label
	<Cedric>	yes
	<ctalbert>	Why?
	<Cedric>	it doesn't appear anywhere in the xul files
	<ctalbert>	That would be a good reason.
	<ctalbert>	Hmm...you're right. I didn't realize that.
	<ctalbert>	Does the same hold true for the other items you removed?
	<Cedric>	I guess that the task.category entity is used for that
	<ctalbert>	I guess so.
	<ctalbert>	Ok. I'm lxr searching the other entity names, and finding that they aren't used anymore either.
	<Cedric>	yes : I made a grep -R to find these 3 entities all around the /calendar directory
	<ctalbert>	Okay that's cool then.
	<ctalbert>	Oh, a question I wanted to ask you so I can learn from you.
	<Cedric>	calendar.dtd is quite messy : the entities are spread averywhere in this file and not gathered by dialog, as it used to be for other files
	<ctalbert>	I can see that. It is a mess.
	<ctalbert>	What purpose does the "control="item-title"> tag serve?
	<ctalbert>	I'm not familiar with the "control" tag
	<Cedric>	to put the focus on the right place
	<ctalbert>	oh. I see.
	<ctalbert>	Okay, so my only nit is that you just need to wrap those long lines.
	<Cedric>	but this is not necessary for checkbox, etc...
	<Cedric>	ok. I'll do that this evening
	<ctalbert>	I'll post this log into a comment on the bug,and you'll have my r+ with those lines wrapped. Nice work.
	<Cedric>	ok. thx

Comment 3

[Cédric Corazza]

Attachment: Fixing the 80 char. per line rule

This one fixes the 80 characters-per-line rule for patch. I didn't see this before. Maybe this rule worth to be documented, or, if already, highlighted.

Comment 4

[Joey Minta]

Comment on attachment 242690 [details] [diff] [review]
Fixing the 80 char. per line rule

Yay accessibility!

+            <label value="&newtodo.duedate.label;" class="todo-only" accesskey="&newtodo.duedate.accesskey;" control="todo-duedate"/>

This guy didn't get wrapped. r2=jminta with that.  Thanks for the patch!

Comment 5

[Cédric Corazza]

Attachment: [checked in] Fixing the wrapping issue

This patch fixes the wrapping issue raised by jminta. It should be good now

Comment 6

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 7

[Cédric Corazza]

There are wrong control attributes for the 'New task' dialog.
Patch follows

Comment 8

[Cédric Corazza]

Attachment: [checked in] Fixing the wrong control attributes

Comment 9

[Matthew (lilmatt) Willis]

Comment on attachment 243012 [details] [diff] [review]
[checked in] Fixing the wrong control attributes

This moves control from the datepickers (which will be disabled until the checkbox is checked) to the checkbox.

r=lilmatt

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 243012 [details] [diff] [review]
[checked in] Fixing the wrong control attributes

r2=mvl

Comment 11

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 12

[Koen Hendrickx]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061031 Calendar/0.4a1

I tried the accesskeys. They are there, but the some of them aren't working for me:
Event: - To (T) -> nothing happens.
Task: - date (a) -> nothing happens.
      - Status has no accesskey.

Comment 13

[cmtalbert]

(In reply to comment #12)

I have spent quite a bit debugging this because the patch looked very good to me when I reviewed it, and the checked in code looked good too.

However, koen has a point, the following access keys do not work:
--Tasks--
the "a" for the Task Entry Date
The status field has no access key
--Events--
The "F" for From Date does not work
The "T" for To Date does not work.

I looked into the Task issue specifically, since the "t" for Due Date _does_ work.  I managed to verify that it is the actual choice of "a" that seems to be messing things up. If you change the xul at 
http://lxr.mozilla.org/seamonkey/source/calendar/base/content/calendar-event-dialog.xul#117

to be:

<label value="&newevent.date.label;z" class="todo-only"
       accesskey="z"
       control="todo-has-entrydate"/>

You will find that the "z" access key does work.

I can't explain what is wrong with the "a" selection, but at least this proves that it is not a weird datepicker issue (which was my first thought), and that it is definitely related to the access keys. 

Therefore, sadly, I will reopen this bug.

Comment 14

[cmtalbert]

The problem here is that the same dialog is used for both tasks and events. Therefore, when creating accesskeys for this dialog, you have to take into account which access keys are used on BOTH the "New Event" and "New Task" page. And you can only use the accesskeys that are NOT in use on either page. 

For example, the trick in comment 13 works because "a" is defined as an Access key for "All Day" on the "New Event" dialog (which is hidden on the "New Task" dialog). Similiarly, if you were to leave the "Date" accesskey as "a" on the Tasks page, and you changed the accesskey for "All Day" to something other than "A", then you would find that the "Date" item on the "New Task" dialog would begin working.

The problem that you will quickly run into is that you may run out of available letters for your accesskeys since each accesskey can only be defined once on both the "New Event" and "New Task" dialog. Perhaps that can be addressed by providing accesskeys for more commonly used dialog features, and leaving them off of the more uncommon dialog features?

Comment 15

[Stefan Sitter [:ssitter]]

Is this still an issue with the new event/task dialog (AKA prototype dialog) in current 0.8pre nightly builds? If yes I would prefer to close this bug that belongs to the old dialog and file a new bug for the new dialog.

Comment 16

[Cédric Corazza]

Though bug 143065 has been fixed, the behavior stated in comments 12 to 14 still occurs : ALT+s and ALT+n don't focus on Start and End.
I guess this could be part of a follow-up bug. Whatever, I'm not skilled to fix this.
Verified with :
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13pre) Gecko/20080218 Calendar/0.8pre

Comment 17

[Martin Schröder [:mschroeder]]

(In reply to comment #16)
> Whatever, I'm not skilled to fix this.

-> Reassigning to nobody.

Comment 18

[Stefan Sitter [:ssitter]]

The dialog in question is no longer available. I see no reason to keep this bug opened.

Comment 19

[Simon Paquet [:sipaq]]

Marking WONTFIX, as this bug is targeted at the old event dialog. 

Please file outstanding issues in the new dialog in followup bugs!