1683460 - dragging ics file to today pane does not populate "New Event" window

Status: RESOLVED FIXED

(Calendar :: Import and Export, defect)

Description

[rodrigo]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

I took a standard ics calendar event filed and dragged it to the "Today Pane".

Actual results:

A "New Event" window was shown (expected) but the window was not populated with the details of the ics file. Instead, the ics file was added as an attachment.

Expected results:

As with pre-78 Thunderbird versions, the "New Event" window should have been populated with the details of the ics file and made available for edit by the user.

Comment 1

[Alice0775 White]

Could you please attach an ics calendar event file for test if possible.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1683460.ics

Sample event attached.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=506be393d5ef8d60b287903bb8e0cb1fca37460e&tochange=9aa142882f1850579cd4262bdbb7263306b2f00a
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c982daa151954c59f20a9b9ac805c1768a350c2&tochange=4fd5c458be4c3bc2d1f22bd575667104a5d173fe

Comment 4

[Lasana Murray]

Bug 437711 seems to have made changes that will treat any file drag and dropped to the today pane as an attachment. Not sure this was intentional or not, bug 437711 comment 36 suggests this was working at the time of the patch, strange.

Comment 5

[Lasana Murray]

I'm making some changes to calendar-dnd-listener for this. The fix is cleaner if I use the non-vendor specific data transfer apis. I noticed however that ThreadOnPaneDragStart adds 3 additional items to the data transfer:

https://searchfox.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#1793

This gives the DataTransfer.items property a length of 4 items instead of one and I would have to add code for this case if I keep using the standard apis. Additionally the deprecated, vendor specific mozItemCount property remains at 1 so I can't help but feel like this is either a bug or wrong use of the data transfer.

Do we need to have 4 items sent for a message data transfer? I prefer to use the standard apis because this route detects the ics file quite nicely.

Comment 6

[Lasana Murray]

Ok, I think I see what's going on here. The items property isn't a list of items included in the drag, it's a list of alternate formats for the data. Guess I'll have to use the moz extensions.

Comment 7

[Lasana Murray]

On second thought, the spec says items.length "Returns the number of items in the drag data store.":
https://html.spec.whatwg.org/multipage/dnd.html#the-datatransferitemlist-interface

The MDN page for items says:

"The read-only DataTransfer property items property is a list of the data transfer items in a drag operation. The list includes one item for each item in the operation and if the operation had no items, the list is empty."

https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/items

This api seems rather ambiguous, the naming does not help either.

Comment 8

[Lasana Murray]

Attachment: bug1683460.patch

I'm uploading this for feedback now as I still need to do some more work on this before I try getting it landed.

calendar-dnd-listener file seemed pretty broken, not sure if it was always the case or it got left behind as time rolled on.

I ended up redoing most of the file albeit, keeping the same logic for now.

This bug happens because the old calendar-dnd-listener ends up treating everything dragged from the OS as a url, so it gets added to the event as an attachment. This is because mozTypesAt gives the following mime types: application/x-moz-file, text/x-moz-url for more or less everything.

If we use the standardized apis however, we get more accurate information like application/x-extension-ics which we can use to figure out what to do. Note: Items dragged from the message pane also get treated like urls including attached ics files.

I mentioned in previous comments that it seems like m-c treats multiple items in a datatransfer as alternatives to the same item. Thus requiring use of the moz* apis to figure out what was actually sent. This is different to drops coming from the OS or external applications where, at least for the applications I tried with, each item dragged and drop has its own DataTransfer.items entry.

This patch updates the code to better detect that and use the appropriate apis where possible. (We want to use the standardized apis if possible because they better identify the items being dropped)

I refactored the code to have each mime type have its own class to handle preprocessing of the drop data before calling the relevant callback. This makes the main listener less complex, adding new mime types is easy as creating a new class and including it in the mimeHandlers property.

I also did some clean up and renaming for consistency sake.

I sill have more work to do on this mainly:

1. Manually test dropping different file types internally and externally to make sure the result makes sense.
2. For eml files, probably parse the message contents into an event.
3. Add some mochitests, hopefully.
4. Investigate the listener instances created here that appear not to be used anywhere.

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9203303 [details] [diff] [review]
bug1683460.patch

Review of attachment 9203303 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I like this in theory. I've only given the details a quick look-over but if you're just using the existing code that's probably fine.

I'd like to see most of this file inside a set of curly braces to avoid adding yet more things to the global scope (like we do for custom element definitions). That'll involve a ridiculous amount of indentation changes, so do it when everything's ready, in a separate revision that we can tell tools to ignore. No need to do anything now, I'm just telling you so you can bear it in mind.

::: calendar/base/content/calendar-dnd-listener.js
@@ +230,5 @@
> +   * destination.
> +   *
> +   * @param {string} data
> +   */
> +  doTransfer() {}

Maybe these empty overridden functions should throw NS_ERROR_NOT_IMPLEMENTED. Since the only subclasses are nearby it's not really necessary but it might help some future developer comprehend what's going on.

(Memories of Java coding in the distant past returned and wanted mark this class as abstract. Wrong language!)

@@ +462,5 @@
> +   * @returns {CalDNDHandler}
> +   */
> +  getPreferredHandlerForList(types) {
> +    return this.mimeHandlers.find(handler => handler.willTransfer(types));
> +  }

This is the same as the previous function, essentially.

Comment 10

[Lasana Murray]

Attachment: bug1683460v2.patch

I changed the approach a bit based on manually testing of various scenarios. I moved the handler classes into a jsm module so they don't pollute the global namespace. As you noticed, I did not touch much of the existing logic (yet!). Some of it is broken but I don't want to make this patch too long, I'll follow up with other bugs. I'm also attaching a spreadsheet I used to keep track of what works and what does not, might be useful when reviewing this patch.

I want to add some tests for this but I'll have to come back for that in the interest of time.

Comment 11

[Lasana Murray]

Attachment: test-plan.ods

Test plan for this bug.

Comment 12

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #10)

I moved the handler classes into a jsm module so they don't pollute the global namespace.

You haven't achieved that at all. Now you've got a bunch of getters in the global namespace which is effectively the same thing. Plus the observers are still in the namespace. This code is only used in one window so I think a JSM isn't a good approach.

In case I was unclear before, here's what I'm asking for:

{
  class CalDNDTransferHandler { ... }
  class CalDNDMozMessageTransferHandler extends CalDNDTransferHandler { ... }

  ... etc. ...

  class CalViewDNDObserver extends CalDNDListener { ... }
  window.calendarViewDNDObserver = new CalViewDNDObserver();
  ... etc.
}

Or you could go for window.calendarViewDNDObserver = new class extends CalDNDListener { ... } to avoid defining a class you're going to use only one of.

Comment 13

[Lasana Murray]

(In reply to Geoff Lankow (:darktrojan) from comment #12)

(In reply to Lasana Murray from comment #10)

I moved the handler classes into a jsm module so they don't pollute the global namespace.

You haven't achieved that at all. Now you've got a bunch of getters in the global namespace which is effectively the same thing. Plus the observers are still in the namespace. This code is only used in one window so I think a JSM isn't a good approach.

Ah I understand now. The defineLazyModuleGetter adds the imports to window? I'm guessing ChromeUtils.import would not, but lazy loading is preferred for performance reasons?

For future reference, what if I add the getters to a local variable instead of window?
Like:

const ns = {}; 
XPCOMUtils.defineLazyModuleGetter(ns, {/*...*/});

let instance = new ns.SomeClass();

Comment 14

[Lasana Murray]

Attachment: bug1683460v3.patch

Removed the jsm. Placed most of the file in a block.

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 9205190 [details] [diff] [review]
bug1683460v3.patch

Review of attachment 9205190 [details] [diff] [review]:
-----------------------------------------------------------------

This is good.

I seem to be picking on only your comments for some reason, but I did look at the code too! I also noticed a few comments in particular are wrapped quite narrowly and they look squished.

::: calendar/base/content/calendar-dnd-listener.js
@@ +23,4 @@
>  
> +  /* exported invokeEventDragSession, calendarViewDNDObserver,
> +   *          calendarMailButtonDNDObserver, calendarCalendarButtonDNDObserver,
> +   *          calendarTaskButtonDNDObserver

This goes outside the block, since it's about what exists out there.

@@ +430,5 @@
>  
> +    /**
> +     * Treats the data provided as an uri to an .ics file and attempts to parse
> +     * its contents. If we detect calendar data in however, we delegate to the
> +     * "text/calendar" handler.

Stray "in"?

@@ +549,5 @@
> +     * those are preferable however not always possible.
> +     *
> +     * This method tries to determine which of the apis is more appropriate for
> +     * processing the drop. It does that by checking for a source node or a
> +     * difference between length of DataTransfer.items and DataTransfer.mozItemCount.

APIs

@@ +563,5 @@
> +
> +      // No mozSourceNode means it's an external drop, however if the drop is
> +      // coming from firefox then we can expect the same behaviour as done
> +      // internally. Generally there may be more DataTransferItems than
> +      // mozItemCount indicates.

Firefox

@@ +834,3 @@
>  
> +  /* exported calendarViewDNDObserver, calendarMailButtonDNDObserver,
> +   calendarCalendarButtonDNDObserver, calendarTaskButtonDNDObserver */

I'm not sure why this comment even exists, since all of these things are declared as exports at the top. Odd.

Comment 16

[Lasana Murray]

I also noticed a few comments in particular are wrapped quite narrowly and they look squished.

I try to keep lines <= 80 characters or less, maybe that's what your are referring to? Can you provide and example and an alternative?

Comment 17

[Lasana Murray]

Attachment: bug1683460v4.patch

Requested changes made.

Comment 18

[Geoff Lankow (:darktrojan)]

Comment on attachment 9205518 [details] [diff] [review]
bug1683460v4.patch

Review of attachment 9205518 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/calendar-dnd-listener.js
@@ +628,5 @@
> +
> +    /**
> +     * Gets called in case we're dropping an array of items
> +     * on one of the calendar views. In this case we just
> +     * try to add these items to the currently selected calendar.

This comment tops out at 65 characters for no apparent reason.

Comment 19

[Lasana Murray]

(In reply to Geoff Lankow (:darktrojan) from comment #18)

Comment on attachment 9205518 [details] [diff] [review]
bug1683460v4.patch

Review of attachment 9205518 [details] [diff] [review]:

::: calendar/base/content/calendar-dnd-listener.js
@@ +628,5 @@

• /**

• * Gets called in case we're dropping an array of items
  

• * on one of the calendar views. In this case we just
  

• * try to add these items to the currently selected calendar.
  

This comment tops out at 65 characters for no apparent reason.

I see. Those are actually old comments that were there before. Since this patch already touches the whole file I'll reformat them.

Comment 20

[Lasana Murray]

Attachment: [checked in] bug1683460v5.patch

Comment 21

[Lasana Murray]

I have some other drag and drop related tests to get around to, I should be able to do some for this too.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9aa47a90e16
Overhaul calendar-dnd-listener to handle internal and external drops. r=darktrojan

Comment 23

[Magnus Melin [:mkmelin]]

There's a linting error I will land a fix for. Exposing globals on the window but never declaring them this way seems odd though.
ALso, the calendarViewDNDObserver is never used anywhere. Is that intentional?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f028af9fa479
followup to fix linting. rs=eslint DONTBUILD

Comment 25

[Lasana Murray]

(In reply to Magnus Melin [:mkmelin] from comment #23)

ALso, the calendarViewDNDObserver is never used anywhere. Is that intentional?

I need to do some further digging to determine if usage was removed accidentally or intentionally.

Comment 26

[Lasana Murray]

Attachment: bug1683460-tests.patch

Adding some tests. Using the drag service was difficult with these so I opted for simulating dom events instead.

Comment 27

[Lasana Murray]

Try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2eaacee98c5de3a5d5bb818bc736037df80daa0c

Comment 28

[Geoff Lankow (:darktrojan)]

Comment on attachment 9210146 [details] [diff] [review]
bug1683460-tests.patch

Review of attachment 9210146 [details] [diff] [review]:
-----------------------------------------------------------------

I like where this is going but it seems too closely tied to the implementation. I'd really like to see the events fired from the DOM, ie. by getting some element from the today pane and calling dispatchEvent on it.

::: calendar/test/browser/browser.ini
@@ +22,5 @@
>  [browser_taskDelete.js]
>  [browser_tabs.js]
>  [browser_taskDisplay.js]
>  [browser_todayPane.js]
> +[browser_todayPane_dragAndDrop.js]

You've put this one line too high. The skip-if belongs to browser_todayPane.js.

::: calendar/test/browser/browser_todayPane_dragAndDrop.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Test dragging an event item in month view works.

I don't think so.

@@ +12,5 @@
> +  inboxFolder,
> +  be_in_folder,
> +  create_folder,
> +  create_thread,
> +  select_click_row,

Alphabetise this list.

@@ +35,5 @@
> +}
> +
> +/**
> + * Tests droping a message from the message pane on to the today pane brings
> + * up the new event dialog.

"droping"? :-D

@@ +42,5 @@
> +  let folder = create_folder("Mochitest");
> +  let thread = create_thread(1);
> +  let [msg] = thread.synMessages;
> +  let subject = "The Grand Event";
> +  let body = "Parking is available.";

There's a tidier way to do all of this. Here's the first example I found: https://searchfox.org/comm-central/rev/f4335f3645ea0d016fcd4936f02e511bc86c7842/mail/test/browser/composition/browser_forwardedContent.js#29-36

@@ +53,5 @@
> +
> +  let [msgStr] = window.gFolderDisplay.selectedMessageUris;
> +  let msgUrl = window.messenger.messageServiceFromURI(msgStr).getUrlForUri(msgStr);
> +
> +  // Setup a DataTransfer to mimic what the message pane sends.

You should mention that you got this from ThreadPaneOnDragStart so it can be found again easily later.

@@ +190,5 @@
> + * Tests dropping a file with an ics extension on the today pane is parsed as an
> + * ics file.
> + */
> +add_task(async function testICSFileDrop() {
> +  let file = await File.createFromNsIFile(new FileUtils.File(getTestFilePath("data/event.ics")));

File.createFromFileName should do it.

@@ +198,5 @@
> +  let promise = CalendarTestUtils.waitForEventDialog("edit");
> +  await ensureTodayPane();
> +
> +  // For some reason, dataTrasnfer.items.add() results in a mozItemCount of 2
> +  // instead of one. Call onExternalDrop directly to get around that.

Typo.

Comment 29

[Lasana Murray]

Attachment: bug1683460-testsv2.patch

Requested changes made.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/00ca542b2738
Add tests for various supported drop types. r=darktrojan

Comment 31

[Lasana Murray]

Attachment: Bug 1683460 - Add tests for various supported drop types. r=darktrojan