The default bug view has changed. See this FAQ.

Unconditional removal of 'disabled' state in function changeContextMenuForTask

RESOLVED FIXED in 1.0b1

Status

Calendar
Tasks
--
minor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mschroeder, Assigned: Decathlon)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.49 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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");
(Assignee)

Comment 1

8 years ago
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

8 years ago
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.
(Reporter)

Comment 3

8 years ago
Confirmed as bug per comment#2.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Investigate unconditional removal of 'disabled' state in function changeContextMenuForTask → Unconditional removal of 'disabled' state in function changeContextMenuForTask
(Assignee)

Comment 4

8 years ago
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'.
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Attachment #401818 - Flags: review?(philipp)
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.
Attachment #401818 - Flags: review?(philipp) → review+
(Assignee)

Comment 6

8 years ago
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.
Attachment #401818 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
Since "patch-v2" has changes request in comment #5, I don't think it needs another review.
Keywords: checkin-needed
Sorry for the late checkin, forgot about this one!

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

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.