1669803 - FileLink regressions in calendar event/task dialog

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Josef Kufner]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.116 Safari/537.36

Steps to reproduce:

Lightning's events and tasks allow only links to be attached. It is a CalDAV limitation. It would be nice if we could attach a local file and it would be uploaded to a cloud, adding the link to the event.

"FileLink for Nextcloud and ownCloud" (https://gitlab.com/joendres/filelink-nextcloud/) extension can handle the uploading part. It works just fine for e-mail attachments, but there is no Filelink API support for such a thing in the calendar.

See https://gitlab.com/joendres/filelink-nextcloud/-/issues/266#note_425612547 for the original issue.

Comment 1

[Josef Kufner]

It seems that the feature is already there, but user interface does not expect it nor handle it well.

See https://thunderbird.topicbox.com/groups/addons/T6413bcfe1aceb6b8-Mf5e0651f11a7bdaacc31e62b/possible-as-an-addon-add-filelink-to-event-editor:

this is already possible, but it looks like the option to do so got lost from the toolbar button, which is worth filing as a bug (interested in patching? :).
You can still do so via the menu Options > Attach > File using (ProviderName)

Moreover, the file upload is not handled well by the user interface. Completed uploads appear in-progress forever and failures are not handled.

So, it is likely only matter of user interface rather than some internals.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

This is a regression on previously existing functionality, the attach button used to have a dropdown, and uploads would attach normally. Would be great to get this fixed along with a bc test to keep it working.

Comment 3

[Magnus Melin [:mkmelin]]

I still see the code in 60 - https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/calendar/lightning/content/lightning-item-toolbar.xul#77-80 which would now be at https://searchfox.org/comm-central/rev/2d7ba685276279da8ba4d01b5081e4420c897398/calendar/lightning/content/lightning-item-toolbar.inc.xhtml#53-59

Have no idea where it went and hg doesn't seem to want to tell me. But I'd imagine it's trivial to add back.

Comment 4

[Lasana Murray]

How do I enable the WeTransfer plugin in a development version? I see it in daily but it's not there in my development build.

Comment 5

[Magnus Melin [:mkmelin]]

Preferences | Attachments | Add WeTransfer.
Showing for me in local builds.

Comment 6

[Lasana Murray]

Attachment: wetransfer.png

This is what I see when I go there.

Comment 7

[Magnus Melin [:mkmelin]]

Strange. For the purpose of this bug you can of course install some other provider as well.

Comment 8

[Lasana Murray]

How do I add an extension in a test?

Comment 9

[Lasana Murray]

Oh, I should add that it appears the WeTransfer plugin is not there in artifact builds.

Comment 10

[Lasana Murray]

Never mind I found this test:
https://searchfox.org/comm-central/source/mail/components/preferences/test/browser/browser_cloudfile.js#20

Comment 11

[Lasana Murray]

Attachment: bug1669803.patch

I restored the attach menu and added a test for web link and a custom provider.

Comment 12

[Lasana Murray]

Try run in progress:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=93f5d33beecd3280a4c8b0967c4d14949f62e9df

Comment 13

[Lasana Murray]

I plan to add the helper functions to CalendarTestUtils in bug 1683444.

Comment 14

[Lasana Murray]

Test is failing on try server. Working on it...

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 9194046 [details] [diff] [review]
bug1669803.patch

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

::: calendar/lightning/content/lightning-item-toolbar.inc.xhtml
@@ +61,5 @@
> +    <menupopup id="button-attach-menupopup">
> +      <menuitem id="button-attach-url"
> +                label="&event.attachments.url.label;"
> +                command="cmd_attach_url"/>
> +    </menupopup>

Please add a comment here about menu items being added dynamically, so the next time somebody comes along and sees a menu button with one option, they don't remove it like the last fool (me) did.

Also in options-attachments-menupopup.

::: calendar/test/browser/eventDialog/browser_attachMenu.js
@@ +71,5 @@
> +
> +/**
> + * Tests using the "Website" menu item attaches a link to the event.
> + */
> +add_task(async function testAttachWebPageWorks() {

I think better names would be testURLAttachment and testCloudFileAttachment. The "works" is unnecessary, at least.

@@ +166,5 @@
> +              resolve({
> +                id: 1,
> +                url: "file:///path/to/mock/file",
> +                path: "/path/to/mock/file",
> +                leafName: "mockFile",

Only really need the URL here, and it should be a http: URL.

@@ +192,5 @@
> +  await CalendarTestUtils.setCalendarView(window, "month");
> +  await calendar.addItem(event);
> +  window.goToDate(event.startDate);
> +
> +  let summaryWin = await openEventFromBox(await getEventBox("calendar-month-day-box-item"));

I think your Windows failures might be here, since after the previous test you're already at this date. Not sure why that would cause a failure though.

@@ +203,5 @@
> +  let menuShowing = BrowserTestUtils.waitForEvent(menu, "popupshowing");
> +
> +  Assert.ok(attachButton, "attach menu button found");
> +  EventUtils.synthesizeMouseAtCenter(attachButton, {}, eventWin);
> +  await menuShowing;

You should wait for the popupshown event here instead of popupshowing.

@@ +206,5 @@
> +  EventUtils.synthesizeMouseAtCenter(attachButton, {}, eventWin);
> +  await menuShowing;
> +
> +  let menuItem = menu.querySelector("menuitem[label='File using Mochitest Account']");
> +  EventUtils.synthesizeMouseAtCenter(menuItem, {}, eventWin);

You should assert that the menuItem exists before clicking on it. After clicking on it you should wait for the popuphidden event and verify that the file picker "opened".

@@ +226,5 @@
> +  Assert.equal(
> +    listItem.attachCloudFileUpload.url,
> +    "file:///path/to/mock/file",
> +    "upload attached to event"
> +  );

Give the provider an icon URL and check that it's set here. Test it before and after the Promise resolves.

When I tested, I noticed that list items had the spinning "connecting" circle, and it never went away. Also, after saving and reopening the event, since my extension had the default icon (chrome://messenger/content/extension.svg), which is larger than the usual size, it wasn't constrained to be the usual size. That should be fixed.

Comment 16

[Lasana Murray]

verify that the file picker "opened"

Is that possible with MockFilePicker? Seems like it does not actually open the file dialog.

Also, after saving and reopening the event, since my extension had the default icon (chrome://messenger/content/extension.svg), which is larger than the usual size, it wasn't constrained to be the usual size. That should be fixed.

Can we tackle this in another bug?

Comment 17

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #16)

Is that possible with MockFilePicker? Seems like it does not actually open the file dialog.

No, it doesn't open the dialog, that's the point of it. (Actually opening the dialog would cause the test to time out because we can't control it.) But you can check that it was asked to open the dialog by looking at the value of MockFilePicker.shown.

Can we tackle this in another bug?

If you must, so long as it happens.

Comment 18

[Lasana Murray]

Attachment: bug1669803v2.patch

Ok, so I made some changes to the test here after some deep, painful debugging and learned a few things:

1. The calendar-event-dialog uses postMessage() and asynchronously updates various parts of the menu UI and other things. This causes the test to timeout because it seems like the DOM is modified when I click on the attach menu. This shows up intermittently and I was unable to figure out a way to wait until all the changes are done so I use setTimeout().

2. Don't use SimpleTest.waitForFocus(). It looked attractive but looks like it causes serious crashes on OSX and Windows when I used it.

Included the icon url in the test, also updated the code that displays the icon in the dropdown and attachments tab.

Comment 19

[Lasana Murray]

Try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=7546db080cdfb048234a12ad694c791cc564aee6&selectedTaskRun=a2ixsn45TcK9J0wUcxkUFw.0

Comment 20

[Lasana Murray]

This function does async work but is not async, neither is the functions that call it. This caused the test to fail. Can I treat this as a bug Geoff or was it intentional?

https://searchfox.org/comm-central/source/calendar/lightning/content/lightning-item-iframe.js#2348

Interestingly, I see code MockFilePicker.open does the same thing.

Comment 21

[Geoff Lankow (:darktrojan)]

Yes it's intentional. The uploading happens asynchronously but the code calling it doesn't care about the result, and in fact needs to carry on immediately without waiting for the result.

The file picker does the same thing because it's from the mythical time before Promise.

Comment 22

[Magnus Melin [:mkmelin]]

Interestingly the mail/ corresponding uploadCloudAttachment is marked as async.
Just because we mark things as async doesn't need they need to be awaited, so IMO we could mark uploadCloudAttachment etc. async for clarity.

Comment 23

[Geoff Lankow (:darktrojan)]

I've been trying to figure out if that time out can be replaced by something a bit more specific. Can't remember exactly where I was three days ago, but I'll have another look tomorrow.

On a related note, I noticed that right-clicking on the attachments list only brings up the FileLink menu items if an attachment is selected. This doesn't account for those menu items, I guess it was written before they existed.

Comment 24

[Lasana Murray]

Yes it's intentional. The uploading happens asynchronously but the code calling it doesn't care about the result, and in fact needs to carry on immediately without waiting for the result.

I can understand the reasons for this, especially in a single threaded, event loop oriented language environment like JS, but in practice intermingling sync and async operations like this can be a source of very hard to reason about bugs. I've already been stuck a few times elsewhere and realized that async operations were going on and not being awaited (thus my test failing only occasionally). We should at least document stuff like this in function comments so anyone looking instantly knows it's doing something the callers don't have control of.

I'm also wondering what happens if for any reason those promises end in a rejection? Will the error propagate? logged to console? silently ignored?

Just because we mark things as async doesn't need they need to be awaited, so IMO we could mark uploadCloudAttachment etc. async for clarity.

We could, if the body of the function actually awaits or returns its promises. Still, creating promises and not including them in control flow is likely to create problems down the road IMO.

Comment 25

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #24)

I'm also wondering what happens if for any reason those promises end in a rejection? Will the error propagate? logged to console? silently ignored?

This happens. It's better than nothing but really should alert the user on failure like the compose code does.

Comment 26

[Geoff Lankow (:darktrojan)]

Comment on attachment 9194948 [details] [diff] [review]
bug1669803v2.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog.xhtml
@@ +435,5 @@
>            <menu id="options-attachments-menu"
>                  label="&event.attachments.menubutton.label;"
>                  accesskey="&event.attachments.menubutton.accesskey;">
>              <menupopup id="options-attachments-menupopup">
> +              <!-- Additional items are added here in loadCloudProviders(). -->

The items are added *after* the existing ones aren't they? The comment should go there too.

::: calendar/test/browser/eventDialog/browser_attachMenu.js
@@ +47,5 @@
> +
> +  let promise = BrowserTestUtils.domWindowOpened(null, async win => {
> +    await BrowserTestUtils.waitForEvent(win, "load");
> +    return win.document.documentURI == "chrome://calendar/content/calendar-summary-dialog.xhtml";
> +  });

This promise is the same as CalendarTestUtils.waitForEventDialog.

@@ +60,5 @@
> +    if (doc.documentURI == "chrome://calendar/content/calendar-event-dialog.xhtml") {
> +      let iframe = doc.getElementById("lightning-item-panel-iframe");
> +      await BrowserTestUtils.waitForEvent(iframe.contentWindow, "load");
> +      return true;
> +    }

I think you should change waitForEventDialog to do this. There's no situation where we don't want to wait for the iframe to be loaded.

Afterthought: wait, why are we editing an existing event instead of just creating a new event? This is more complex than it needs to be.

@@ +89,5 @@
> +  // At load time the event dialog makes various asynchrounous UI changes.
> +  // These may be the reason this test occasionally timeouts when clicking
> +  // the web link entry.
> +  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
> +  await new Promise(resolve => setTimeout(resolve, 2000));

Hmmm. I can't see anything in this function that should have this problem. Using waitForCondition would be a better option, if you can figure out what's not ready in time.

Also "times out", not "timeouts". :-)

@@ +208,5 @@
> +  // asynchrounously via a postMessage() in lightining-item-(iframe|panel).js
> +  // when the dialog opens. Waiting until the menuitem appears does not appear
> +  // to prevent occassional failure when trying to click the target menu item.
> +  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
> +  await new Promise(resolve => setTimeout(resolve, 2000));

Instead of calling setTimeout, you could wait here for the menuitem to exist.

asynchronously
lightning
occasional

Comment 27

[Josef Kufner]

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

(In reply to Lasana Murray from comment #24)

I'm also wondering what happens if for any reason those promises end in a rejection? Will the error propagate? logged to console? silently ignored?

This happens. It's better than nothing but really should alert the user on failure like the compose code does.

Error handling is indeed really poor (from user perspective), but that is another issue.

Comment 28

[Lasana Murray]

Hmmm. I can't see anything in this function that should have this problem. Using waitForCondition would be a better option, if you can figure out what's not ready in time.

Instead of calling setTimeout, you could wait here for the menuitem to exist.

I tried various iterations of this but the test would still occasionally timeout. Reason seems to be the dropdown menu closes as soon as it is opened and the following click targets the calendar field instead of the menuitem.

For some reason this no longer happens and it does not fail on the try server, I'm removing the timeouts.

Comment 29

[Lasana Murray]

Attachment: bug16698003v4.patch

Made requested changes, removed timeouts, also made the test check the summary dialog for the attachment.

Comment 30

[Geoff Lankow (:darktrojan)]

Comment on attachment 9195512 [details] [diff] [review]
bug16698003v4.patch

This piece is all fine, but I had a look at your Try run and realised this patch has broken this test code, so you'll need to get that fixed before landing. It should just be a case of waiting for the menu to open then clicking on the menuitem, like you're already doing here.

Comment 31

[Lasana Murray]

Attachment: bug1669803v5.patch

Included the changes recommended. eventDialog test no longer fails.

Comment 32

[Lasana Murray]

browser_timezones.js fails on OSX but I don't think it's related to this patch.

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f7b1a46b521c4e894fc450b9d32cf2e14b554520

Comment 33

[Geoff Lankow (:darktrojan)]

Comment on attachment 9195870 [details] [diff] [review]
bug1669803v5.patch

Nice one. Got there in the end.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e159a02960ab
Restore the attachment dropdown in the event dialog. r=darktrojan

Comment 35

[Geoff Lankow (:darktrojan)]

I removed the lines setting/resetting the timezone since I forgot to ask you to do it after bug 1684485.

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 9195870 [details] [diff] [review]
bug1669803v5.patch

[Approval Request Comment]
Regression, baked on beta.

Comment 37

[Wayne Mery (:wsmwk)]

Comment on attachment 9195870 [details] [diff] [review]
bug1669803v5.patch

[Triage Comment]
Approved for esr78

Comment 38

[Geoff Lankow (:darktrojan)]

Attachment: 1669803-calendartestutils-esr78.diff

Adds CalendarTestUtils.jsm to 78.

Comment 39

[Geoff Lankow (:darktrojan)]

Attachment: 1669803-esr78.diff

Port to 78.

Comment 40

[Rob Lemley [:rjl]]

Thunderbird 78.8.0:
https://hg.mozilla.org/releases/comm-esr78/rev/0eb856fb0a4d
https://hg.mozilla.org/releases/comm-esr78/rev/acc2bc0f9d17

Comment 41

[Geoff Lankow (:darktrojan)]

I made a silly mistake in the backporting and somehow failed to notice:
https://hg.mozilla.org/releases/comm-esr78/rev/8de9dbf0b785