Open Bug 1711446 Opened 4 years ago Updated 9 months ago

cloudFile API: Add onBeforeFileUpload event to open composeAction popup

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
enhancement

Tracking

(Not tracked)

Status:
REOPENED

People

(Reporter: tdulcet, Assigned: TbSync)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

_onBeforeSend.zip
1.72 KB, application/x-zip-compressed
Details

Add a onBeforeFileUpload event to the cloudFile API, similar to the existing onBeforeSend event in the compose API (Bug 1532528 and Bug 1617742). I would like to open a composeAction popup before file uploads to allow the user to set options for each file, but I cannot do that with the regular onFileUpload event since it is not set as a user action. As an alternative, I can open separate windows, but that would be very annoying for users who are uploading multiple files. A composeAction popup would also be much more intuitive, as it is of course in the same window as the email.

For example, for my FileLink provider for Send add-on (implements Bug 1516252), I would like to allow users to set a download limit, time limit and optionally a password for each file.

Bug 1674374 would also be useful for this event.

Would you be able to build Thunderbird on your own and give this patch a try, to see if it fulfils your requirements?

As you can see, not all upload requests are actually user initiated. The onBeforeFileUpload event is not triggered by uploads initiated by the compose API in case the compose API is used to replace the content of an attachment (compose.updateAttachment [1]) and that attachment happens to be a cloudFile attachment. This does trigger a new file upload (since TB97 or so), but I cannot trigger the onBeforeFileUpload event, as it is not directly user initiated (for example in the onBeforeSend event).

It is also not triggered, if the compose API is used to clone an attachment from one composer to another (compose.addAttachment [2]). If that attachment happens to be a cloudFile attachment and the developer is also renaming the attachment while cloning, it has to be considered as a new upload, to not invalidate the existing links.

[1] : https://webextension-api.thunderbird.net/en/latest/compose.html#updateattachment-tabid-attachmentid-attachment
[2] : https://webextension-api.thunderbird.net/en/latest/compose.html#addattachment-tabid-attachment

The idea of the onBeforeFileUpload event is to attach individual information to a single upload. For API triggered uploads which replace the content or the name, I think the information should carry over to the new file, but that is something the provider must do. I therefore added a referenceFileId to the onFileUpload and onBeforeFileUpload event, so the provider learns what file this new upload is based on (if applicable). In that case you could decide to not open the composeActionPopup and simply take the values specified for the other file. And for the API triggered uploads, you get the referenceFileId in onFileUpload as well.

(So in the end you probably use the referenceFileId in onBeforeFileUpload to decide to open the popup or not, and in onFileUpload to actually carry over the information, to handle user initiated and API initiated uploads at the same location.)

Good?

Flags: needinfo?(tdulcet)
Assignee: nobody → john
Status: UNCONFIRMED → NEW
Ever confirmed: true

Wow, thanks for implementing this! As you probably know from our recent conversations on other FileLink bugs, after I filed this enhancement request, I did publish my FileLink provider for Send add-on on ATN. I had to use the popup window approach, but I think this will enable a big improvement for the user experience, at least for the compose window, by allowing FileLink add-ons to open a composeAction popup.

I have been meaning to thank you for all your recent FileLink work, including the many long-standing bugs you have fixed. Just a few days ago I updated my FileLink add-on to support the new template features you added in bug 1627497 and bug 1643729. I believe it is the first add-on to support that.

(In reply to John Bieling (:TbSync) from comment #2)

Would you be able to build Thunderbird on your own and give this patch a try, to see if it fulfils your requirements?

Unfortunately, I do not currently have a TB build environment setup, so I think it would be easiest if you could do a try run and then I could just download the installer/binary artifact.

Good?

OK, thanks for all that information. So what is the difference between this new referenceFileId and the existing id already in the CloudFile object provided with the onFileUpload event? Is the referenceFileId just to signal it was an API initiated upload? If so, I think it would be confusing to have two separate IDs for every file.

There is a lot of complexity here, so I want to make sure I understand everything, so my implementation will correctly handle all these edge cases. There are now three ways that a CloudFile upload can be initiated:

  1. The compose window: both the onBeforeFileUpload and onFileUpload events will fire and the tab object will be set.
  2. The calendar: both the onBeforeFileUpload and onFileUpload events will fire, but the tab object will NOT be set.
  3. The compose API: the onBeforeFileUpload event may fire, but the onFileUpload event will fire and the tab object will be set.

Add-ons should then preferably handle those cases like this:

  1. Open a composeAction popup and then store the options for the referenceFileId.
  2. Open a regular popup window (as it does now) and then store the options for the referenceFileId.
  3. Attempt to retrieve the options for the provided referenceFileId, otherwise open a composeAction popup as above.

Is that all correct? (I also think that there should be something similar to the composeAction popup for the calendar and users should be able to initiate FileLink/CloudFile uploads from a chat, but I guess that would be two additional enhancements, which would add even more complexity to this...)

Flags: needinfo?(tdulcet)

(In reply to tdulcet from comment #3)

so I think it would be easiest if you could do a try run and then I could just download the installer/binary artifact.

I will add a few more tests on Monday to see if everything is as it should be and then fire up a try run.

So what is the difference between this new referenceFileId and the existing id already in the CloudFile object provided with the onFileUpload event? Is the referenceFileId just to signal it was an API initiated upload? If so, I think it would be confusing to have two separate IDs for every file.

The referenceFieldId is not limited to API initiated uploads, user initiated uploads can have it as well. This id specifies a previously uploaded file. Consider this:

  • User opens a new composer and attaches a new cloudFile, which gets ID:1
  • User opens another composer and selects that file from the cloud menu (a so called repeated use) - there is no upload happening but the url of ID:1 is just re-used
  • Now the user decides to rename the file in one of the two composers.

Usually you would expect the onFileRename listener of your cloudFile add-on to be called for ID:1, but in this case - because the file is re-used - the onFileUpload listener is called, because the rename could have a negative impact for the other linked instance, so it is issued as a new upload with ID:2, but gets ID:1 as the referenceFileId. And since it is a user initiated event, the onBeforeFileUpload is called as well with the same parameters.

This allows you to do fancy things like cloning the file on the server (if it supports that) without actually sending it (again) and you can reuse the additional information (password, lifetime, ...) of ID:1 or ask the user again for those if you think that is needed.

There are now three ways that a CloudFile upload can be initiated:

  1. The compose window: both the onBeforeFileUpload and onFileUpload events will fire and the tab object will be set.
  2. The calendar: both the onBeforeFileUpload and onFileUpload events will fire, but the tab object will NOT be set.

Correct.

  1. The compose API: the onBeforeFileUpload event may fire, but the onFileUpload event will fire and the tab object will be set.

The compose API (addAttachment and updateAttachment) will not fire the onBeforeFileUpload.

Add-ons should then preferably handle those cases like this:

  1. Open a composeAction popup and then store the options for the referenceFileId.
  2. Open a regular popup window (as it does now) and then store the options for the referenceFileId.
  3. Attempt to retrieve the options for the provided referenceFileId, otherwise open a composeAction popup as above.

I hope my above example clarified this, the referenceFileId is just an additional info which allows you to re-use earlier data. The data the user enters in the comosePopup or in the custom popup window belongs to the new file (fileInfo.id). But you could shortcut and re-use the data belonging to the provided referenceFileId and not ask the user again.

I also think that there should be something similar to the composeAction popup for the calendar and users should be able to initiate FileLink/CloudFile uploads from a chat, but I guess that would be two additional enhancements, which would add even more complexity to this...)

I hope it does not add more complexity :-) However, I have no idea when I will be able to work on those. But I agree, calendar chart also need an action button.

The compose API (addAttachment and updateAttachment) will not fire the onBeforeFileUpload.

This is of course something which can be changed. This is just a draft. If you compare the onBeforeSend event: It can also be triggered by using compose.sendMessage(), so strictly speaking the onBeforeSend is no longer a true user input event in that case. If we want to be consistent, we then should always fire onBeforeFileUpload as well, regardless if it was user initiated or API initiated.

Something which could be discussed as well: Fire onBeforeFileUpload only if no referenceFileId is given. I do not know if this makes it easier for the developer. He would then get the event only for real new uploads and otherwise gets the referenceFileId in onFileUpload to reuse the additional file info of that reference upload.

Edit: If we always fire onBeforeFileUpload, we actually do not need it and could make onFileUpload a user input event handler instead.

Attachment #9260281 - Attachment description: WIP: Bug 1711446 - Add cloudFile.onBeforeFileUpload as a user input event handler. → Bug 1711446 - Change cloudFile.onFileUpload to be a user input event handler. r=mkmelin

(In reply to John Bieling (:TbSync) from comment #4)

This allows you to do fancy things like cloning the file on the server (if it supports that) without actually sending it (again) and you can reuse the additional information (password, lifetime, ...) of ID:1 or ask the user again for those if you think that is needed.

OK, I see how this would be useful for some services, but for my add-on specifically it would be problematic, as one of the options the user sets is the download limit (which defaults to 1). If the same URL were ever reused, it then likely would not work for those additional recipients.

In your scenario, if the user selects the same file in a different compose window, they would not expect the original download limit to still apply. Therefore, my add-on would need to ask the user again for the options in all cases. However, I suppose it could still use the referenceFileId to set the defaults in the popup to those values the user had previously used for that file, as there may be a high likelihood they would want to reuse those values.

The compose API (addAttachment and updateAttachment) will not fire the onBeforeFileUpload.

The compose API will never fire the onBeforeFileUpload? Does this mean that the referenceFileId parameter would always be set in this case?

I hope it does not add more complexity :-) However, I have no idea when I will be able to work on those. But I agree, calendar chart also need an action button.

I am currently planning to check if the tab object is set to determine whether to open a composeAction popup or a regular popup window. If additional action buttons/popups were added for the calendar and/or chat tabs for example, add-ons would then need some way to determine which popup to open. That is what I meant by even more complexity. It actually may be better if CloudFile had its own popup (like a modal), which would reduce this potential future complexity and also eliminate the need for CloudFile add-ons to request the otherwise unneeded compose permission (which is of course required to have a composeAction popup).

(In reply to John Bieling (:TbSync) from comment #5)

This is of course something which can be changed. This is just a draft. If you compare the onBeforeSend event: It can also be triggered by using compose.sendMessage(), so strictly speaking the onBeforeSend is no longer a true user input event in that case. If we want to be consistent, we then should always fire onBeforeFileUpload as well, regardless if it was user initiated or API initiated.

I would prefer to put all my options getting code in onBeforeFileUpload to reduce code duplication, so my vote is to be consistent with onBeforeSend and always fire onBeforeFileUpload. However, I am not sure I understand all the use cases for the compose API addAttachment and updateAttachment functions, so in some cases such as if an extension just renames a file, it may make sense not to fire onBeforeFileUpload when the referenceFileId parameter is set.

Edit: If we always fire onBeforeFileUpload, we actually do not need it and could make onFileUpload a user input event handler instead.

Hmm, I am not sure about that, as onBeforeSend blocks the main TB UI when it is fired, so one would obviously not want onFileUpload to block the TB UI during every file upload. 😲 Another thing to consider is bug 736169, as you may want to show the user a different indicator when in onBeforeFileUpload vs onFileUpload.

(In reply to tdulcet from comment #6)

(In reply to John Bieling (:TbSync) from comment #4)

This allows you to do fancy things like cloning the file on the server (if it supports that) without actually sending it (again) and you can reuse the additional information (password, lifetime, ...) of ID:1 or ask the user again for those if you think that is needed.

OK, I see how this would be useful for some services, but for my add-on specifically it would be problematic, as one of the options the user sets is the download limit (which defaults to 1). If the same URL were ever reused, it then likely would not work for those additional recipients.

Good point. This will already fail in TB91. The repeated use via the cloud menu is part of TB since a couple of years. I have an idea how to fix this. Will explain that later on.

The compose API will never fire the onBeforeFileUpload? Does this mean that the referenceFileId parameter would always be set in this case?

I changed that specific behaviour since the last draft. I removed the onBeforeFileUpload event and made the onFileUpload event a user input event handler. The referenceFileId is replace by the relatedFileInfo object and it is currently included in two cases:

  • a repeatedly used cloud file attachment is being renamed
  • the content of a cloud file is being updated

The object looks like so:

      {
        "id": "RelatedCloudFile",
        "type": "object",
        "description": "Information about an already uploaded cloud file, which is related to a new upload. For example if the content of a cloud attachment is updated or if a repeatedly used cloud attachment is renamed (and therefore should be re-uploaded to not invalidate existing links).",
        "properties": {
          "id": {
            "type": "integer",
            "minimum": 1,
            "optional": true,
            "description": "An identifier for this file. In some circumstances, the id is unavailable."
          },
          "url": {
            "type": "string",
            "description": "The URL where the uploaded file can be accessed.",
            "optional": true
          },
          "templateInfo": {
            "$ref": "CloudFileTemplateInfo",
            "description": "Additional file information used in the cloud file entry added to the message.",
            "optional": true
          },
          "name": {
            "type": "string",
            "description": "Filename of the file."
          },
          "dataChanged": {
            "type": "boolean",
            "description": "The content of the new upload differs from the related file."
          }
        }
      }

I am currently planning to check if the tab object is set to determine whether to open a composeAction popup or a regular popup window. If additional action buttons/popups were added for the calendar and/or chat tabs for example, add-ons would then need some way to determine which popup to open.

The tab property holds the tap type.
https://webextension-api.thunderbird.net/en/latest/tabs.html#tab

That is what I meant by even more complexity. It actually may be better if CloudFile had its own popup (like a modal), which would reduce this potential future complexity and also eliminate the need for CloudFile add-ons to request the otherwise unneeded compose permission (which is of course required to have a composeAction popup).

Hm, currently I do not see a need to add an extra cloud popup. And you do not need to request the compose permission just for the compose action button.

Edit: If we always fire onBeforeFileUpload, we actually do not need it and could make onFileUpload a user input event handler instead.

Hmm, I am not sure about that, as onBeforeSend blocks the main TB UI when it is fired, so one would obviously not want onFileUpload to block the TB UI during every file upload. 😲

There really is no difference. onBeforeFileUpload did not block the main UI and the updated onFileUpload also does not. It just blocks the upload from starting. The only thing that is blocked is the send button.

Another thing to consider is bug 736169, as you may want to show the user a different indicator when in onBeforeFileUpload vs onFileUpload.

That we can do. I will remember that when I start to work on it.

Now back to your download limit issue. Currently, the provider is not notified, if a link is re-used. For your case, that is bad. So what I propose is to always trigger a onFileUpload event for the repeated use and make this the 3rd scenario, where the relatedFileInfo is send:

  • the provider can check, if name or content have changed to identify repeated use (or I can add an extra isReused property to the obj just for that)
  • the provider can decide whether to simply return the url and the templateInfo of the relatedFileInfo and mimic the current repeated use, or to issue a new file upload, so the link gets its own download count.

I would combine that change with a new manifest key "handleRepeatedUse" (or something like that), to enable the new mode, so other add-ons automatically keep re-using links if they do not opt into the new behaviour.

Would that solve the issue for you?

(In reply to John Bieling (:TbSync) from comment #7)

The tab property holds the tap type.
https://webextension-api.thunderbird.net/en/latest/tabs.html#tab

Yes, I was planning to check if this parameter is set (i.e. it is not null/undefined). This seemed to be the best way to differentiate uploads initiated from the compose window vs the calendar. The tab ID (tab.id) is of course also needed to enable/disable a composeAction button.

Hm, currently I do not see a need to add an extra cloud popup. And you do not need to request the compose permission just for the compose action button.

This probably should be a separate followup enhancement request, but I was thinking it would be better to have a single CloudFile popup that could work in all places that CloudFile uploads can be initiated, including the compose window and calendar, as well potential future places like the chat tab. This would eliminate the need for users to have a greyed out and otherwise unusable composeAction button in their compose window for each FileLink add-on (see top right of this screenshot). It would also eliminate the complexity for developers of FileLink add-ons to potentially have to handle multiple different action popups and windows.

There really is no difference. onBeforeFileUpload did not block the main UI and the updated onFileUpload also does not. It just blocks the upload from starting. The only thing that is blocked is the send button.

I actually like that onBeforeSend blocks/locks the compose window UI, as it prevents the composeAction popup from accidentally closing before the user clicks one of the buttons, which would otherwise hang the sending process indefinitely. In the case of onFileUpload, this would hang the file upload indefinitely.

Now back to your download limit issue. Currently, the provider is not notified, if a link is re-used. For your case, that is bad. So what I propose is to always trigger a onFileUpload event for the repeated use and make this the 3rd scenario, where the relatedFileInfo is send:

  • the provider can check, if name or content have changed to identify repeated use (or I can add an extra isReused property to the obj just for that)
  • the provider can decide whether to simply return the url and the templateInfo of the relatedFileInfo and mimic the current repeated use, or to issue a new file upload, so the link gets its own download count.

I would combine that change with a new manifest key "handleRepeatedUse" (or something like that), to enable the new mode, so other add-ons automatically keep re-using links if they do not opt into the new behaviour.

Would that solve the issue for you?

Yes, that would solve the issue for my add-on. Thanks for taking the time to find a solution!

If the name has been changed, onFileRename event should already be called. If the content has been changed, I suspect all FileLink add-ons would want to reupload the file, so onFileUpload event should be called. That just leaves this reused case. Adding a manifest key for it would break backwards comparability with previous versions of TB, so maybe you could add something like an onFileRepeat event to enable the new mode, similar to the onFileRename event. My add-on could then just reuse the same function it currently uses for onFileUpload:

if (browser.cloudFile.onFileRepeat) {
    browser.cloudFile.onFileRepeat.addListener(upload);
}

Another idea would be adding a new property to the CloudFileAccount object, which could be set by cloudFile.updateAccount, but I am not sure if that would cause an error in previous versions of TB.

Another idea would be adding a new property to the CloudFileAccount object, which could be set by cloudFile.updateAccount, but I am not sure if that would cause an error in previous versions of TB.

Even better: The event listener registration for onFileUpload can take additional parameters (see https://webextension-api.thunderbird.net/en/latest/addressBooks.provider.html#onsearchrequest as an example) so you can enable that behaviour will registering the listener. That looks promising.

This probably should be a separate followup enhancement request, but I was thinking it would be better to have a single CloudFile popup that could work in all places that CloudFile uploads can be initiated, including the compose window and calendar, as well potential future places like the chat tab. This would eliminate the need for users to have a greyed out and otherwise unusable composeAction button in their compose window for each FileLink add-on (see top right of this screenshot). It would also eliminate the complexity for developers of FileLink add-ons to potentially have to handle multiple different action popups and windows.

Yes, please go ahead and file a bug for that. It can still be wontfixed, but it is good to have the idea in a place where it is not forgotten.

The issue with the closing popup is indeed mind-boggling. Also in the onBeforeSend listener this can happen and the user is stuck, if there is some action required in that popup. Do standard Javascript close listeners work for the popups?

(In reply to John Bieling (:TbSync) from comment #9)

Even better: The event listener registration for onFileUpload can take additional parameters (see https://webextension-api.thunderbird.net/en/latest/addressBooks.provider.html#onsearchrequest as an example) so you can enable that behaviour will registering the listener. That looks promising.

Great idea!

Yes, please go ahead and file a bug for that. It can still be wontfixed, but it is good to have the idea in a place where it is not forgotten.

OK, will do...

The issue with the closing popup is indeed mind-boggling. Also in the onBeforeSend listener this can happen and the user is stuck, if there is some action required in that popup. Do standard Javascript close listeners work for the popups?

With onBeforeSend I think it is OK, as the compose window UI is locked, which prevents the composeAction popup from closing. However, if onFileUpload will not similarly block the UI, it could be an issue. By standard JS close listeners, I assume you mean something like onbeforeunload. I am not sure if that would work with a composeAction popup, but from a quick search, it does not seem to work with action popups in Firefox/Chrome. It is also not used by either the composeAction popup or regular popup window sample TB extensions, which from my understanding follow the best practices.

Attached file _onBeforeSend.zip —

With onBeforeSend I think it is OK, as the compose window UI is locked, which prevents the composeAction popup from closing.

I am playing around with that just now and the popup closes even though the composer is locked. I attached an example XPI.

As you can see in that XPI, I am able to detect the three states of the popup: closed, ok and cancel.

I am now a bit unsure what to do. Using the composeAction popup does feel better then a stand alone popup window, but if it can make the user get "stuck" .... Hm.

Once you received the close event in the background, you cannot re-open the popup, as the user input state has been cleared, due to the await. The only thing that comes to mind is:

  • use onBeforeFileUpload
  • let it return cancel like onBeforeSend but also retry, which will retrigger the event and allows to re-open the popup.

But it feels wrong. Thinking ... What other things could we do if the popup gets closed? Or should we be able to prevent closing? Do not know if that works and it will be a different behaviour from FF.

(In reply to John Bieling (:TbSync) from comment #11)

But it feels wrong. Thinking ... What other things could we do if the popup gets closed? Or should we be able to prevent closing? Do not know if that works and it will be a different behaviour from FF.

I think the closing is fine as long as developers can detect it and then cancel the FileLink upload (by returning { aborted: true } from onFileUpload). The user could then request to upload the file again if the closing was unintentional. Although, preventing closing of action popups when the UI is locked may also be good (developers can currently do this from the developer tools for debugging purposes, so it is possible).

Again, I think a FileLink specific modal that does not have a close button or method to dismiss without the user explicitly clicking on one of the buttons, would be even better UX. However, a composeAction popup is still much better than the stand alone popup window my add-on uses now, so I do not want to let perfect be the enemy of good so to speak.

Depends on: 1751895
Depends on: 1751897
Blocks: 1752114
Attachment #9260281 - Attachment is obsolete: true

I abandoned the patch for now, as most of it was related to the reuse issue. Created a new bug for that (Bug 1752116). Will work on the blocker bugs first, before continuing here.

John - Firefox is dropping the user action requirement for opening Action popups (see bug 1755763 and bug 1799344), so if Thunderbird were to also implement that change, it would also resolve this bug. Should I update the title of this bug to something like "Remove user input requirement for all Actions popups" or create a new bug?

Interesting, that is quite a change. We would need a new bug called "Port bug 1755763 to Thunderbird" and then make this bug depend on the new one and if it does not need further changes to allow you what you need, close it as Works for me (after the ported bug landed successfully).

Does it also change how and when the onClicked event is fired?

Thanks, will do...

Does it also change how and when the onClicked event is fired?

It looks like that event still only fires for add-ons without a popup. However, note bug 1755763 comment 15.

Depends on: 1817803

To recap:

We still need to fix bug 1751897 which is a real blocker for you. But with 1817803 fixed, this bug can be closed, correct?

Flags: needinfo?(tdulcet)

Yes, I will of course still need to test it after bug 1817803 lands on TB Daily, but presumably this bug should be able to be closed at that point. Thanks again for all your work on this!

However, without this new onBeforeFileUpload event, bug 1751897 no longer applies, as the TB UI will never be locked. It would of course still be very helpful for users to have a way to prevent the compose action popups from closing when uploading files and/or find a workaround for bug 1534041 so developers can detect when the popups are closed.

Flags: needinfo?(tdulcet)

find a workaround for bug 1534041 so developers can detect when the popups are closed.

I should clarify, now that you fixed bug 1687182, the beforeunload event does seem work as expected with standalone popup windows, but it still does not work with the compose action popups and the unload event does not work with either type of popup.

I would like to ask you to create dedicated bugs for the things still missing/broken and NI me for all of them (including those which might already exists). It has become so easy to lose track in here.

I guess I can close this one as WONTFIX.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX

OK, there are a couple of approaches TB could take here, whether to prevent the action popups from closing or allow developers to detect when they are closed. If they chose the latter, I am not sure if it is even possible for the beforeunload and/or unload events to be fixed (this might be a FF bug).

There is also the issue that only one compose action popup can be opened at a time per compose window, so if the user tries to upload multiple attachments at once, there would be a race condition for which file opens the popup first and the other uploads will hang indefinitely. Maybe we need this new onBeforeFileUpload event after all, as I believe this would have been solved by locking the TB UI and then fixing bug 1751897.

Anyway, I would be happy to open dedicated bugs, but I am not sure which approaches TB wants to take and what specifically I should be requesting in these new bugs. Implementing bug 1817803 did allow the compose action popups to be opened, but it also unfortunately introduces new issues that would need to be resolved before many extensions will be able to make use of this new capability...

but it also unfortunately introduces new issues that would need to be resolved before many extensions will be able to make use of this new capability...

Please file a bug.

Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

I am not sure if it is even possible for the beforeunload and/or unload events to be fixed (this might be a FF bug).

I just checked and this is also an issue in Firefox, so bug 1534041 still applies. Specifically, the beforeunload event does not fire for action popups (unless one explicitly calls window.close()). The unload event does fire, but asynchronous functions like runtime.sendMessage() do not work. Anyway, it looks like fixing these events is not possible unless TB wants to differ from FF.

(In reply to John Bieling (:TbSync) from comment #22)

Please file a bug.

Maybe we could repurpose this bug to consider adding a onBeforeFileUpload event as originally requested in comment 0, but that would additionally lock the TB UI and only be called sequentially, even when uploading multiple attachments at once. If bug 1751897 was then fixed, this would prevent the compose actions popups from unintentionally closing and it should also prevent the race condition described in comment 21.

I believe I was able to potenchally work around the race condition by using a complex system with both a promise map and promise chain, but ideally this issue would be fixed at the API level, which the proposed onBeforeFileUpload event would do if it were only called once at a time. Asking for options per attachment upload is likely a common paradigm that most FileLink/CloudFile add-ons follow, so it should not be this complex to implement correctly. Also, note that this workaround and the entire cloudFile API is incompatible with the non-persistent background pages in MV3. I strongly hope that Thunderbird will NOT be following browsers like Chrome in removing support for persistent background pages, as otherwise most FileLink providers and many other add-on types would no longer be possible.

Also, note that this workaround and the entire cloudFile API is incompatible with the non-persistent background pages in MV3. I strongly hope that Thunderbird will NOT be following browsers like Chrome in removing support for persistent background pages, as otherwise most FileLink providers and many other add-on types would no longer be possible.

I would like to remind you of the single-issue-per-bug policy. It is really difficult to keep track of things mentioned in comments of unrelated bugs. If you find an issue, please report it as a new bug. Having said that, I will answer here, but if you have further questions regarding this, please use our chat/topicbox or file a bug if something is broken.

We do follow all the other vendors and removed persistent background pages in MV3. Firefox has introduced persistent events, which is a very good alternative (they wake up the background when needed). One has to store state data in local storage and Firefox is working on a dedicated session storage just for state data. Our MV3 test for cloudFile passes, and I am confident it works in MV3 (https://searchfox.org/comm-central/source/mail/components/extensions/test/browser/browser_ext_cloudFile.js#768).

That reminds me: Please also file a bug (with some details) for your report that the new openPopup() method is not usable (I do not remember where you dropped that).

(In reply to John Bieling (:TbSync) from comment #24)

I would like to remind you of the single-issue-per-bug policy. It is really difficult to keep track of things mentioned in comments of unrelated bugs.

Sure, this was just an aside, but still very relevant to the topic of this bug, which is making the compose action popups work with the cloudFile API. Anyway, I do of course agree that the general topic of MV3 and Thunderbird should be discussed elsewhere, which I might do if the time comes...

Note that I do not work for Mozilla or Thunderbird and only am able to do extension development for donations during my very limited free time. I am happy to help create bugs when time permits, but as I am sure you know, they can take a considerable amount of time to fully research and determine a reliable STR for...

We do follow all the other vendors and removed persistent background pages in MV3.

Not all browsers are removing support for persistent background pages. Brave and several other Chromium and Firefox forks have already committed to support persistent background pages indefinitely. Those that are currently still planning to remove them (Chromium, Firefox and Safari) are allegedly doing so support mobile devices, but I have seen no public announcements that the upcoming Thunderbird for Android will support extensions. Even if a future version does (which I do hope it will), there is no need to force Desktop extensions to support the lowest common denominator platform. Thunderbird could require non-persistent background pages on Android, but continue to support persistent background pages on Desktop. This would still of course significantly reduce the number of potentially available add-ons on Android, but I think could be an acceptable compromise. Considering that Thunderbird has continued to support Experiment APIs almost 6 years and counting after Firefox dropped them in 2017, I do not see why they could not do the same with persistent background pages. Thunderbird really has now has the opportunity to take the lead on this issue, as many of those "other vendors" have unfortunately dropped the ball.

Firefox has introduced persistent events, which is a very good alternative (they wake up the background when needed). One has to store state data in local storage and Firefox is working on a dedicated session storage just for state data. Our MV3 test for cloudFile passes, and I am confident it works in MV3

No, Firefox's event pages are only a minor improvement over Chrome's service workers, but they still suffer from the same fundamental problems from being non-persistent unlike the browser itself. While I am sure you are correct that the cloudFile API technically works in MV3, in practice it is not possible to implement a nontrivial FileLink extension.

For example, even your official Thunderbird example for opening a compose action popup, shows that a promise map is required. With the above workaround for the race condition described in comment 23, my FileLink add-on now requires two separate promise maps. However, with MV3, contrary to your example, the global variables needed for these promise maps are of course not allowed. One could use that upcoming storage.session API you alluded to (bug 1687778), but only supports primitive values, which does not include Maps nor Promises. Yes, one could potentially convert the Map to a JS Object (with decreased performance), but that still cannot store Promises, which along with other issues makes it impossible to reliably implement a nontrivial FileLink provider add-on in MV3 without using a disallowed and unreliable antipattern, as some of your other examples suggest (such as pinging the background script to keep it alive).

That reminds me: Please also file a bug (with some details) for your report that the new openPopup() method is not usable (I do not remember where you dropped that).

That was in comment 21 above. I was referring to the other issues mentioned in that same comment, not anything new. For my testing so far, the openPopup() function by itself does indeed work as expected. Sorry for the confusion.

Anyway, I know you single handedly maintain the extensions support in Thunderbird, so thanks again for all that work.

Attachment #9385128 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: