DnD month view: Drag and Drop stops working if event is dropped out of calendar view

RESOLVED FIXED

Status

Calendar
Internal Components
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: ssitter, Assigned: Joey Minta)

Tracking

Trunk
x86
Windows 2000

Details

Attachments

(1 attachment)

2.37 KB, patch
cmtalbert
: first-review+
Michiel van Leeuwen (email: mvl+moz@)
: second-review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
Fall out from Bug 312736:
DnD month view: Drag and Drop stops working if event is dropped out of calendar view 

Steps to Reproduce:
1. Switch to month view
2. Create normal event
3. Drag and Drop that event out of calendar view, e.g. to task list
   --> Drop shadow is still displayed at the last displayed position.
4. Refresh view (e.g. select 'Go to Today').
   --> Event is shown at original position.
5. Drag and Drop that event to another day.

Actual Results:
Drag and Drop is not possible at all. JavaScript console shows:
    Error: shadow.parentNode has no properties
    Source File: chrome://calendar/content/calendar-month-view.xml 
    Line: 471

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060629 Sunbird/0.3a2+ (my own trunk build)
(Assignee)

Comment 1

12 years ago
So the obvious solution here is for the dropping code to also add a listener to its containing window for an ondrop event.  This solves the bug as described.  The tougher case is where someone drops our data outside the app (either successfully or unsuccessfully).  As far as I know, we have no way of getting this information at all.  I'm looking for opinions on a.) Is this simply an edge case we don't care about?  b.) Is it sufficient to simply update ourselves the next time we're moused over?  In this case, the same fix for dropping outside the app would work for dropping inside the app. or c.) Is this something we need to lobby heavily for gecko to care about?
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
(Assignee)

Comment 3

12 years ago
Created attachment 231046 [details] [diff] [review]
add mouseover listener

Gosh, it's painful to have to write code this ugly.  Such is the price of playing with mozilla dnd code...

Patch adds a mouseover listener, to check whether we're really still dragging.  If we dropped somewhere else, we'll revert the drag gracefully.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #231046 - Flags: first-review?(dmose)
(Assignee)

Updated

12 years ago
Blocks: 343268
(Assignee)

Comment 4

12 years ago
We need to add a listener for when we leave the view, and then register this kind of listener.  We should also put the event back and kill the shadow at this point.  If that's a lot of work, we'll land this.
Comment on attachment 231046 [details] [diff] [review]
add mouseover listener

Moving reviews to ctalbert and lilmatt per dmose
Attachment #231046 - Flags: second-review?(lilmatt)
Attachment #231046 - Flags: first-review?(dmose)
Attachment #231046 - Flags: first-review?(cmtalbert)

Comment 6

12 years ago
Comment on attachment 231046 [details] [diff] [review]
add mouseover listener

This looks great. I tested this on WinXP. The only nit I have is that the window.addEventListener and the two window.removeEventListener lines could be wrapped and they would be under 80 chars. r+
Attachment #231046 - Flags: first-review?(cmtalbert) → first-review+
Comment on attachment 231046 [details] [diff] [review]
add mouseover listener

>Index: calendar/base/content/calendar-month-view.xml
>+        monthbox.monthView.dropListener = function checkStillDragging() {
>+            var dragService = Components.classes["@mozilla.org/widget/dragservice;1"]
>+                                    .getService(Components.interfaces.nsIDragService);
Align this line ^^^

>+             var session;
>+             try {
>+                 session = dragService.getCurrentSession();
>+             } catch(ex) {}
>+             if (!session) {
>+                 monthbox.removeDropShadows();
>+                 monthbox.addItem(item);
>+                 window.removeEventListener("mouseover", monthbox.monthView.dropListener, true);
>+             }
Somehow all of this got indented one space too much.
Also, please add a space between "catch" and the (, and wrap the window.removeEventListener line so it's under 80 chars.

>+        };
>+        window.addEventListener("mouseover", monthbox.monthView.dropListener, true);
Wrap this line also.

>+         window.removeEventListener("mouseover", this.monthView.dropListener, true);
Wrap this line also, and what's up with the wacky indenting in this area?

r+ with those fixed.
Attachment #231046 - Flags: second-review?(lilmatt) → second-review+
Whiteboard: [needs checkin]
(Assignee)

Comment 8

12 years ago
Comment on attachment 231046 [details] [diff] [review]
add mouseover listener

need a peer here.
Attachment #231046 - Flags: second-review+ → second-review?(mvl)
Comment on attachment 231046 [details] [diff] [review]
add mouseover listener

r2=mvl
Attachment #231046 - Flags: second-review?(mvl) → second-review+
(Assignee)

Comment 10

12 years ago
Patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs checkin]
(Reporter)

Updated

12 years ago
Duplicate of this bug: 369059
You need to log in before you can comment on or make changes to this bug.