Last Comment Bug 512436 - Unconditional removal of 'disabled' state in function changeContextMenuForTask
: Unconditional removal of 'disabled' state in function changeContextMenuForTask
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Tasks (show other bugs)
: Trunk
: All All
: -- minor (vote)
: 1.0b1
Assigned To: Decathlon
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-25 04:30 PDT by Martin Schröder [:mschroeder]
Modified: 2010-02-04 11:04 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.33 KB, patch)
2009-09-21 04:32 PDT, Decathlon
philipp: review+
Details | Diff | Review
patch-v2 (1.49 KB, patch)
2009-09-21 07:54 PDT, Decathlon
no flags Details | Diff | Review

Description Martin Schröder [:mschroeder] 2009-08-25 04:30:05 PDT
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 2009-08-26 02:31:47 PDT
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 2009-08-26 13:01:51 PDT
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] 2009-09-19 04:51:05 PDT
Confirmed as bug per comment#2.
Comment 4 Decathlon 2009-09-21 04:32:42 PDT
Created attachment 401818 [details] [diff] [review]
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] 2009-09-21 06:07:08 PDT
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 2009-09-21 07:54:06 PDT
Created attachment 401853 [details] [diff] [review]
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 2009-10-01 14:22:12 PDT
Since "patch-v2" has changes request in comment #5, I don't think it needs another review.
Comment 8 Philipp Kewisch [:Fallen] 2009-10-11 03:44:27 PDT
Sorry for the late checkin, forgot about this one!

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

-> FIXED

Note You need to log in before you can comment on or make changes to this bug.