416138 - Moving of all-day-events in weekview not possible, but shows possibility

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Lars Wohlfahrt (thetux)]

Steps to Perform:

   1. Switch to week view
   2. Create new event all-day-event
   3. Mouse over created event and press mouse button
   4. Move mouse over next day
   5. Release mouse button

Results:

    * Moving the event not possible

Expected Results:

    * Calendar is in week view mode
    * Event is visible in main view
    * Event is selected and its background color has changed
    * New From and To date are visible
    * Event has new start date and is visible in new day

See also https://litmus.mozilla.org/show_test.cgi?id=2595

Comment 1

[Lars Wohlfahrt (thetux)]

Additional info:

Error console is empty.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080206 Calendar/0.8pre 2008020619

Comment 2

[Martin Schröder [:mschroeder]]

This is _NO_ duplicate of bug 350603. Some changes (maybe the new dnd functionality in Lightning) have enabled dnd of all day events, but it should not be shown as being possible in the calendar view.

This is misleading UI and could result in many bug reports after the release. QA team nominates this bug for blocking-calendar0.8.

Comment 3

[Sven Giermann]

Works for me in Lightning 2008020719 and TB2.0.0.9 on WinXP.

Maybe the DND possibility for all-day events in day view may be misleading, but it's still possible to do Event to Task/Mail conversion through DND.

Comment 4

[cmtalbert]

True, there are many ways to edit the event.  This is something we should fix, but because there are easy workarounds, we are not going to block on this.

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Work in Progress - v1

First try at fixing this, no idea if this is the right thing to do and I'm sure there is a better way. 

What was never implemented in the dnd listener was the canDrop method, so the generic method from nsDragAndDrop was used. This patch implements canDrop by checking the target element for a canDrop attribute, and evaluating the attribute value as code. 

If the target doesn't have a canDrop attribute, the DOM tree is traversed upwards until an element has the canDrop attribute. This is useful for i.e the today pane, where the canDrop is on the vbox, and it should be possible to drop on anything in the vbox. While this works nicely, I'm a bit sceptical with the traversal, since the whole DOM branch is traversed upwards to the root element, if the element under the cursor isn't something that should be dropped on.

Current users of the canDrop attribute only return true, but theoreticaly we should check if the data from the drag session to see if the data is actually something we can drop on the respective elements.

I'd be glad if someone else could take over on this.

Comment 6

[Berend Cornelius]

Taking over...

Comment 7

[Berend Cornelius]

Attachment: patch v. #2

Comment 8

[Berend Cornelius]

When I started to work on this issue I noticed that the functionality that I need has actually already been implemented within the monthview, where dragging of eventboxes between day-box-containers is already possible.
This patch adds a new widget capable that may serve as a droptarget and is capable of carrying drop drop shadows similar to the current drag'n drop behaviour in the month-view. The introduced widget should be used for all kind of possible droptargets, where the events have to be adapted to a certain period. It still offers lot of room for improvement but  I hope that it's sufficient for a first step.
Some implementation notes:
- The patch has become quite big  because I consolidated a lot of month-view-code into it and did some other refactoring on the way.
- It only supports drag'ndrop for one single event box like the current implementation also does. Maybe we can improve this for the future and that's why I already keep the dropshadows in an array.
I removed the following code snippet  and could not see any bad side effects... .If necessary I have to reintroduce it.
-I was hesitating to add another two functions to bloated calUtils.js, but looking over that file I found that rather other – more ui-centric – functions should be shifted to calendar-ui-utils than my two functions. I think we should do this in another issue.
>        // Mozilla doesn't provide any generic way for us to listen for the
>        // drag-end.  Add this listener for a best approximation of listening
>        // for that case.
>        var monthbox = this.parentBox;
>         monthbox.calendarView.dropListener = function checkStillDragging() {
>            try {
>                var session = getDragService().getCurrentSession();
>            } catch (ex) {}
>            if (!session) {
>               monthbox.removeDropShadows();
>               monthbox.addItem(item);
>            }
>        }; 
>       window.addEventListener("mouseover",
>                                monthbox.calendarView.dropListener, true);

- I currently only took care that all day events may be dragged and drop from all-day-containers in the week view. 
This patch is certainly not relevant for the 0.9 release!

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 338279 [details] [diff] [review]
patch v. #2

While testing this patch I noticed that if you mouse over the shadows, the shadows flicker slowly. I couldn't reproduce bug 343190 which introduced the code snippet you posted, so I think is safe to remove that code for now. What I did notice though, that if you move around the item then the shadows get removed though and don't come back until you go to the next box. May be a side effect of flickering though.



>-        var action = dragService.DRAGDROP_ACTION_MOVE;
>+        var action = getDragService().DRAGDROP_ACTION_MOVE;
You don't need the whole drag service for this, go ahead and use
Components.interfaces.nsIDragService.DRAGDROP_ACTION_MOVE.

>         // Mozilla doesn't provide any generic way for us to listen for the
>         // drag-end.  Add this listener for a best approximation of listening
>         // for that case.
>         var monthbox = this.parentBox;
If you are removing the code with the drag-end, then please also remove the above comment.

>+            dateBoxes[0].showMonthLabel = true;
>+            dateBoxes[dateBoxes.length-1].showMonthLabel = true;
While you are at it, spaces around operator.

>+      <method name="onDragItem">
>+        <parameter name="aItem"/>
>+        <body><![CDATA[
>+         var newItem =moveItem(aItem, this.mDate);
Missing space

>+dragndropContainer {
>+  -moz-binding: url(chrome://calendar/content/widgets/calendar-widgets.xml#dragndropContainer);
>+}
Is this container used on its own? if not, then afaik no css rule is needed, its possible to extend other bindings without it.

>+      <method name="onDragItem">
>+        <parameter name="aItem"/>
>+        <body><![CDATA[
>+            // method that may be overwritten by derived bindings...
>+        ]]></body>
>+      </method>
May it be, or *must* it be overridden? If the latter, then I'd suggest throwing NS_ERROR_NOT_IMPLEMENTED there. Also please correct the typo (overwritten vs overridden). In any case, it would be nice to add some documentation what exactly a derived binding should do in this method, or in which cases it is invoked.

>+#ifdef XP_UNIX
>+                      var self = this;
>+                      setTimeout(function addDropShadows() { self.addDropShadows(); }, 5);
>+#else
>+                      this.addDropShadows();
>+#endif
What was the reason for this workaround? It would be nice to get rid of it if possible ;-)

>+          return XULElement.prototype.setAttribute.call (this, aAttr, aVal);
Remove space after |call|
>   /**
>+   * replaces an occurrence.
>+   * @param aOldOccurrence      The occurrence to be replaced
>+   * @param aNewOccurrence      The occurrence bound to replace 'aOldOccurrence'
>+   */
>+void replaceOccurrence (in calIItemBase aOldOccurrence,
>+                         in calIItemBase aNewOccurrence);
>+
>+  /**
>    * Modify aOccurrence.  If aNewStartTime and aNewEndTime are given,
>    * update the event to those times.  If aNewTitle is given, modify the title
>    * of the item.  If no parameters are given, ask the user to modify.
>    */
>   void modifyOccurrence (in calIItemBase aOccurrence,
It would be nice if you could make more clear what the difference between replaceOccurrence and modifyOccurrence is in this documentation.



r- for now since the flickering needs to go away, but I'll gladly r+ a patch that doesn't flicker ;-)

Comment 10

[Berend Cornelius]

As Martin mentioned in another bug we have a new Drag'n Drop API described in
http://developer.mozilla.org/en/DragDrop/Drag_and_Drop

As I noticed there is now a "dragleave" handler instead of a "dragexit" hanbdler provided. This could be helpful to resolve the remaining problems

Comment 11

[Berend Cornelius]

Attachment: patch v. #3

I worked in the issues from Philipp's comment #9 and also found away to avoid the flickering that was caused by the transition of the mouse pointer over the internal anonymous elements. I added a lot of consolidation and tried to make the code of the calendar-month-view a bit more understandable.

Comment 12

[Berend Cornelius]

Attachment: patch v. #4

I found out that drag'n drop in the multiday view was also related to my change and therefor regressed. Fixed and consolidated that.

Comment 13

[Philipp Kewisch [:Fallen][☁️📆]]

I found a problem:

* Drag event into calendar list, unifinder, or outside of window (i.e gnome-terminal) and drop it.

Actual:
* Dropshadows still exists

Expected:
* Dropshadows should go away.

Comment 14

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 349980 [details] [diff] [review]
patch v. #4

>+    let serializer = Components.classes["@mozilla.org/calendar/ics-serializer;1"].
>+                                createInstance(Components.interfaces.calIIcsSerializer);
Wrap dot to next line here and elsewhere

>+    transfer.setTransferData("text/calendar", supportsString, supportsString.data.length*2);
>+    transfer.setTransferData("text/unicode", supportsString, supportsString.data.length*2);
Spaces around operator


>+          var icon = document.getAnonymousElementByAttribute(this, "anonid", "item-icon");
use let instead of var

>             var monthName = calGetString("dateFormat", "month." + (aDate.month+1) + ".Mmm");
Space around operator and use of let, while you are here.

>-            for each (var itemData in box.box.mItemData) {
>+            for each (var itemData in box.mItemData) {
let

>+// We may run into certain racing conditions under Unix that may lead to the freezing
>+// of a complete session. That's why we have to set a timeout for this platform.
Do we still have this problem? If so, I'd actually go for just doing this on a timeout anyway and get rid of the ifdef's.

>+    moveItem: function cal_moveItem(aOldItem, aNewDate) {
>+         let newItem = aOldItem.clone();
>+         let start = (aOldItem[calGetStartDateProp(aOldItem)] || aOldItem[calGetEndDateProp(aOldItem)]).clone();
>+         let isDate = start.isDate;
>+         start.resetTo(aNewDate.year, aNewDate.month, aNewDate.day,
>+                       start.hour, start.minute, start.second,
>+                       start.timezone);
>+        start.isDate = isDate;
>+        if (newItem[calGetStartDateProp(newItem)]) {
Indentation in this function seems a bit mixed.

>diff --git a/calendar/base/public/calICalendarView.idl b/calendar/base/public/calICalendarView.idl
>--- a/calendar/base/public/calICalendarView.idl
>+++ b/calendar/base/public/calICalendarView.idl
>@@ -157,4 +157,10 @@ interface calICalendarView : nsISupports
>    * Setting this does not refresh the view.
>    */
>   attribute AUTF8String timezone;
>+
>+  /**
>+   * removes the dropshadows that are inserted into childelements during a
>+   * drag and drop session
>+   */
>+  void removeDropShadows();
> };
Do we really need this in the interface? Sounds like an internal detail to me...


>-box[dropbox="true"] {
>+box[dropshadow="true"] {
Hmm, can't we rather use a .dropshadow-box selector along with the corresponding class? This could minimally improve performance.


r=philipp with comments considered.

Comment 15

[Berend Cornelius]

Attachment: [checked in] patch v. #5

I worked in the issues as commented by Philipp into the attached patch.
Some notes:

>Do we still have this problem? If so, I'd actually go for just doing this on a
>timeout anyway and get rid of the ifdef's.

You seem to be damn right with your assumption. I could not reproduce this bug anymore. But as this is not a final proof and as that bug was so serious I want to deal with it in a follow-up issue that should then be tested by several users/testers or developers. I also like your idea to apply the timeout on all platforms if it should still be necessary for Unix.

>Hmm, can't we rather use a .dropshadow-box selector along with the
>corresponding class? This could minimally improve performance.

I changed the implementation and use the class attribute now...

>Do we really need this in the interface? Sounds like an internal detail to
>me...
In this case I think we need this in the interface simply because the method is called from outside the class.

pushed that attached patch to comm-central

http://hg.mozilla.org/comm-central/rev/f282378ae9e9

->fixed.
Thank you for your careful review!

Comment 16

[Andreas Treumann]

I checked thi sfix and found a new minor issue.

STR:
===

- switch to weekview
- drag an all day event to the all day section of the next day
- wait a moment that the drag shadow gets visible
- move and drop the event outside the all day section
- drag shadow is furthermore visible

Comment 17

[Berend Cornelius]

reopening due to Andreas comment #16.
-At first I will have to remove all dropshadows when the mousepointer is dragged out of the calendar views in a drag session
But how do I deal with allday events that are dragged to the "normal day section" of the day or week view. I see three possibilities:
- raise the Event dialog, before the modified event is dropped.
- keep the all day flag of the event and just adjust the startDate and endDate. The modifed event would then again appear in the allday section of the calendar-view
- not allow this at all because that section is not reserved for all day events.

Comment 18

[Decathlon]

It seems there are a couple of issues due to this patch (to verify when the patch is finished).

1) bug 469146 is probably caused by this patch because of onDropItem()
(http://hg.mozilla.org/comm-central/rev/f282378ae9e9#l2.236 for month-view and  http://hg.mozilla.org/comm-central/rev/f282378ae9e9#l3.138 for multiday-view).
In fact newItem.dueDate is always a null value after onDropItem() is called:
  
6.149 +         let newItem = this.onDropItem(item, session).clone();
6.150 +         let newStart = newItem.startDate || newItem.entryDate;
6.151 +         let newEnd = newItem.endDate || newItem.dueDate;

and this seems to cause bug 469246.

2) If you try to drag and drop a spanning days event, when you drop the event it becomes a one day event (his end date/time becomes the same as start date/time). This behavior would seem caused by onDropItem() too.

Moreover, in line 6.135 :
transfer.addDataFlavor("application/x-moz-cal-event"),
is x-moz-cal-event intentional?

Comment 19

[Decathlon]

> and this seems to cause bug 469246.

...bug 469146

Comment 20

[Philipp Kewisch [:Fallen][☁️📆]]

Comment 18/19 will be fixed in bug 469146.

Comment 22

[Berend Cornelius]

Attachment: [checked in] patch v. #6

Inserted a dragend handler to remove all possibly remaining dropshadows. Notice: Dragging all day events into the "main" view is still not possible und ends in an error message. We should deal with this in another bug.

Comment 24

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 358838 [details] [diff] [review]
[checked in] patch v. #6

> Inserted a dragend handler to remove all possibly remaining dropshadows.
> Notice: Dragging all day events into the "main" view is still not possible und
> ends in an error message. We should deal with this in another bug.

Are you going to take care of the other bug too? Please at least make sure there is no error message and the drop feedback is set so that it isn't allowed in case you don't have time to fix the other bug.

r=philipp

Comment 25

[Berend Cornelius]

pushed to comm-central:

http://hg.mozilla.org/comm-central/rev/7ebc039a89cc

Philipp did a final review over the shoulder.

->fixed.

Comment 26

[Stefan Sitter [:ssitter]]

Looks like this checkin regressed Bug 476219.

Comment 27

[Philipp Kewisch [:Fallen][☁️📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".