547593 - Delete Key does not work on selected event in views

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Markus Adrario [:Taraman]]

With the latest Trunk Version (Built yesterday, Feb 20th 2010)
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100218 Lightning/1.1a1pre Shredder/3.2a1pre
the Del-Key is not working.

When selecting an event and pressing Del, I get the following error Message:

Error: An error occurred executing the cmd_delete command: [Exception... "'[JavaScript Error: "focusedElement is null" {file: "chrome://calendar/content/calendar-common-sets.js" line: 321}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 100

I get this on two different Systems with two seperate builds.

Comment 1

[Markus Adrario [:Taraman]]

Attachment: Patch V1

It seems, that when executing the delete command the case that there is no focusedItem and the calendar is in foreground is not handled.
In that case the fact that focusedItem is Null causes the error.

This patch should fix it.

The same may apply to calendar_modify_focused_item_command [1] but this is only used in Sunbird as far as I could see.
Sunbird does not show this bug.


[1]:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-common-sets.js#292

Comment 2

[Decathlon]

Testing the patch I've found this issue:

-create a new event;
-select a calendar in the calendars list;
-select the event in the view;
-press the "del" key;
--> you can't delete the event or, if you have more than one calendar, Lightning
    asks for deleting the calendar;
you can  delete again the event moving the focus on the event (another click to modify the summary or by selecting the event in the unifinder).

Comment 3

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

Comment on attachment 429361 [details] [diff] [review]
Patch V1

r- based on Decathlon's testing. 

Decathlon, if you want, go ahead and review the next iteration of this patch :-)

Comment 4

[Decathlon]

(In reply to comment #3)
> Decathlon, if you want, go ahead and review the next iteration of this patch
> :-)

NO, no, please :-)
I apologize, but this issue bugs me a lot, so when I see a patch, I try it immediately. Since it works fine for the normal use, I've left it on my Lightning even if it has the problem in comment #2, which is an issue by far minor than the original bug.

Comment 5

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

No need to apologize, I appreciate your testing very much! Just trying to get you interested in doing reviews since your coding is already very good :-)

Comment 6

[Markus Adrario [:Taraman]]

Attachment: Patch V2

Since the problem seemed to be that there was no focusedItem when the bug appeared, I had a closer look into the focus-functions.
Normally, boxes should be focussable, but for some reason, the boxes in the view aren't.
So the apprpoach in Patch V2 is totally diferent from V1, but it works for me.
Until now I'm not sure why, but it solves the problem, which should not be existant anyhow.

Another example that with a computer you solve problems you wouldn't have without one... ;-)

Since decathlon is not available for rviews, I thought I try the feedback field, because you seem to do some good testing.

Comment 7

[Decathlon]

Comment on attachment 430769 [details] [diff] [review]
Patch V2

The patch seems working fine with the events on the views, but there is still a problem on today pane. I'm not sure whether it's related to this issue or depends on some problems about focus on the today pane:

1. create more events for e.g. today (either tomorrow or soon);
2. select the first one (on today-pane) and press "del" key
--> the event is deleted and the following one is highlighted;
3. press again "del" key
--> the error mentioned in commen #0 appears in the console and the event is not deleted;
now every time you press the "del" key you get the error.

If you repeat the same str while in mail mode, the behavior is the same but the error message doesn't appear.

Seems like the event on today-pane doesn't get the focus when the previous has been deleted (though, it looks like a focused element i.e. it's highlighted). 
I know there are problems with focus on today-pane but at the moment I can't find the related bugs.

r- but it's a r+ if the problem mentioned here is related to today-pane focus.

Comment 8

[Markus Adrario [:Taraman]]

When looking at the agenda code, which is responsible for the events on the today-pane, I saw that it has own functions to handle various key-presses.

Since at least the delete and modify commands are defined in calendar-common-sets.js, the agenda code should point there.

So I would keep this bug to the view-pane only and care of the today-pane keys in a seperate one.

p.s.: The Bugs decatholon was looking for could be Bug 417241 & Bug 437943

Comment 9

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

Comment on attachment 430769 [details] [diff] [review]
Patch V2

Unfortunately this patch causes all events in the view to show up in the tab order, i.e you can tab between each event. We don't have any :focus rules on our event boxes, so the tabbing situation gets even worse.

I don't think that adding styles would be the right thing to do here, the amount of tabable elements would get quite large and we rather want to reduce this and use other keys.

Comment 10

[Markus Adrario [:Taraman]]

In this case, the solution will require big changes in the "case "button_delete":" section as fas as I can see.

This function relies on the focus, but if the event is not focussable, the focus can be on any other element in the ui which had the focus before clicking the event.
Using focusedElement as selector here is likely to produce erratic behaviour then (as seen in comment #2)

I will have another look at it very soon.

Comment 11

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

Maybe there is a way to make it focusable, but not add it to the tab order? maybe tabIndex="0" works.

Comment 12

[Markus Adrario [:Taraman]]

Good hint. it is tabindex="-1".
Now I'm looking for a way to get the -moz-user-focus out of the css, to prevent it being changed by accident in a theme.

unfortunately style="-moz-user-focus: normal;" on calendar-editable-item (where i put the tabindex property) does not work.

Will have to try something different.

Comment 13

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

I think its fine to leave it in the css files. If a themes breaks this, its rather the theme author's fault not ours :-) You can add a comment in the css file though.

Comment 14

[Markus Adrario [:Taraman]]

Attachment: Patch V3

This patch fixes the tabable events, while still allowing the boxes to be focused.

Comment 15

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

Comment on attachment 435154 [details] [diff] [review]
Patch V3


>-  <binding id="calendar-editable-item">
>+  <binding id="calendar-editable-item"
>+           tabindex="-1">
>     <content mousethrough="never" tooltip="itemTooltip">
You need to set the tabindex on the <content> node.

Otherwise r=philipp

Comment 16

[Markus Adrario [:Taraman]]

tabindex moved to <content>

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/498643c181a9>

-> FIXED