512436 - Unconditional removal of 'disabled' state in function changeContextMenuForTask

Status: RESOLVED FIXED

(Calendar :: Tasks, defect)

Description

[Martin Schröder [:mschroeder]]

Follow-up from bug 411849:
Investigate unconditional removal of 'disabled' state in function changeContextMenuForTask(aEvent) in calendar/base/content/calendar-task-tree.js:
111     document.getElementById("calendar_new_todo_command").removeAttribute("disabled");
112     document.getElementById("calendar_new_todo_todaypane_command").removeAttribute("disabled");

Comment 1

[Decathlon]

Opening context menu on todo panel of todaypane with no tasks selected (e.g. just after Thunderbird has started), the function 'applyAttributeToMenuChildren' [1] called inside changeContextMenuForTask [2],
sets all popup menu items with a "command" attribute to 'disabled' state, but 'New Task ..." item, i.e. "calendar_new_todo_command", must be enabled to allow to create a new task, so the code lines  111 and 112 in calendar-task-tree.js file remove the disabled state for new_todo_command that must be the only enabled command when no task is selected.
Might it be the right answer?


[1] http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-task-tree.js#98 
[2] http://mxr.mozilla.org/comm-central/source/calendar/base/src/calUtils.js#1477)

Comment 2

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.4pre) Gecko/20090826 Calendar/1.0pre

Doesn't seem correct, at least in the following case: All calendars are read-only or switched off. Opening the context menu enables the New Task menu entry and New Task toolbar button although it is not possible to create a new task.

Comment 3

[Martin Schröder [:mschroeder]]

Confirmed as bug per comment#2.

Comment 4

[Decathlon]

Attachment: patch

(In reply to comment #2)
> Doesn't seem correct, at least in the following case: All calendars are
> read-only or switched off. Opening the context menu enables the New Task menu
> entry and New Task toolbar button although it is not possible to create a new
> task.

Therefore, basically, it should be checked only the enabled state of the commands "calendar_new_todo_command" and "calendar_new_todo_todaypane_command".

I tried this patch with enable/disable/read-only/writable calendars and with a calendar that doesn't support tasks (Google Calendar + Google Provider), and it seems working in every case.

I'm not completely sure whether 'removeAttribute(disabled)' is the same of 'setAttribute("disabled", false)'. If not, can be used a 'if-else' statement with both 'removalAttribute' and 'setAttribute'.

Comment 5

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

Comment on attachment 401818 [details] [diff] [review]
patch

>+    let newTodoEnabled = calendarController.isCommandEnabled("calendar_new_todo_command") &&
>+                         calendarController.isCommandEnabled("calendar_new_todo_todaypane_command");
>+    document.getElementById("calendar_new_todo_command").setAttribute("disabled", !newTodoEnabled);
>+    document.getElementById("calendar_new_todo_todaypane_command").setAttribute("disabled", !newTodoEnabled);


setAttribute doesn't take a boolean as a second argument, but a string, so you need to setAttribute/removeAttribute there. You could do setAttribute("x", "false"), but its more common to remove the attribute.

Please see our helper in calendar-ui-utils.js. We have setBooleanAttribute, but I just say its using "true"/"false" too. The other possibility is

setElementValue("calendar_new_todo_todaypane_command", !newTodoEnabled && "true", "disabled");

(or similar, please doublecheck). Setting the second parameter to boolean false (=== comparison) causes the attribute to be removed.

codewise r+ with that changed, please upload a new patch.

Comment 6

[Decathlon]

Attachment: patch-v2

(In reply to comment #5)

> setAttribute doesn't take a boolean as a second argument, but a string, so you
> need to setAttribute/removeAttribute there. You could do setAttribute("x",
> "false"), but its more common to remove the attribute.
> 

Here the difference. Sorry for that and thanks for your explanation.
In my defence (;-)) I have to say that before doing the patch I checked the "setAttribute" function on calendar code (by mxr) and aside from "false" and "true" arguments, I found a lot of setAttribute("disabled", true/false) or even cases with variables as second argument.

I've done in the more common way by removing the attribute when it should be disabled.

Comment 7

[Decathlon]

Since "patch-v2" has changes request in comment #5, I don't think it needs another review.

Comment 8

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

Sorry for the late checkin, forgot about this one!

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/5b06b0538127>

-> FIXED