Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce blob handling for Android & iOS 🚀🚀 #5498

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

gwdp
Copy link

@gwdp gwdp commented Mar 12, 2022

Introduce blob handling for Android & iOS webviews 🚀🚀

Whenever is asked for blob:/ or normal downloads to be 'opened', folder selection is prompted and file saved.

This closes #5478

Test cases are addressed on the following repo: https://github.com/ikon-integration/Capacitor-Blob-Download-Issue


To summarize, there are 4 common download cases that could be addressed:

  • Base64 blobs through blob:data... scheme.
    • Expanding this for 2 cases, known mime-types and unknown mime-types.
    • Both Android and iOS were opening png on the internal browser (as an example). This causes users to be stuck on the image and manual closure of the application is needed.
  • Blob links though blob:http... scheme.
  • Web worker initiated downloads.

All test cases are shown on the example repo above. However, I was able to address only the first 3 on this PR.
Web worker initiated downloads are probably handled by the browser core and my bet is that we won't have access to those APIs.


Approaches:

iOS

Pretty simple, it does only work on iOS 14.5 >=, as WebKit has exposed APIs for handling such only since.
When a download request is received, we prompt for folder selection and then save the file with a unique file name.

Android

LIttle more complex, had to implement JS interface that injects JS code to download the received URL through XHR (so it does use the JS accessible context), pipe that in chunks to an Intent that asks the user for a file selection and then pipe the received content to a file with a 'duplex' stream implementation.

Commonly

Both approaches ask users for destination selection and implement download notifications to be later consumed by App plugin (so applications can decide what to do with it (show alert, show it on a list, or whatever)).


Still, not sure if the implementation is 100% correct but it does work with all the 3 test cases specified above and does not break anything AFAIK. So, maybe is just a matter of consolidating it a little more. Review and suggestions and highly appreciated :)

@riderx
Copy link

riderx commented Mar 25, 2022

This is amazing ! thanks for that

@gwdp
Copy link
Author

gwdp commented Apr 5, 2022

I wish I could implement this as a plugin, but mostly sure I won't have access to or overwrite the things I need.
Change is almost a month old and crickets are chirping.. :/ This is just sad.

@giralte-ionic giralte-ionic self-assigned this Apr 7, 2022
@giralte-ionic
Copy link
Contributor

I wish I could implement this as a plugin, but mostly sure I won't have access to or overwrite the things I need.
Change is almost a month old and crickets are chirping.. :/ This is just sad.

Sorry for the delay in any response, I've enabled the CI checks and there appears to be a lint issue. Please correct this and I will continue to review.

In the mean while I'm trying out the tests for myself. Thanks for contributing!

Dan Giralté - Lead Engineer Capacitor & Portals @ Ionic

@gwdp
Copy link
Author

gwdp commented Apr 7, 2022

@giralte-ionic appreciate your reply and time on it.
Lint issues fixed and rebased. Let me know if you need any other changes.

@gwdp
Copy link
Author

gwdp commented Apr 8, 2022

Fixed iOS test

@gwdp
Copy link
Author

gwdp commented Apr 8, 2022

Fixed Xcode 12.4< errors.
Just not 100% sure about:

// TODO: remove once Xcode 12 support is dropped
#if compiler(<5.5)
    protocol WKDownloadDelegate {}
#endif

It does work (not sure if the best approach). :)

@giralte-ionic
Copy link
Contributor

Sorry it's been taking some time but I've moved this up to the rest of the capacitor team for review. Things are moving forward. More soon.

@gwdp
Copy link
Author

gwdp commented Jun 2, 2022

I'm sorry @giralte-ionic , but this is looking ridiculous now.
Almost 3 months old and not even a code review.

@giralte-ionic
Copy link
Contributor

@gwdp sorry you can't see the significant work that's been put into proving this out, nor some of the controversy this PR has roused. I'm also sorry I haven't posted a newer response.

The issue we have with this change set is that it's being done to the core of Capacitor, and is better suited for the Capacitor Filesystem Plugin.

@rowrowrowrow
Copy link

bump, I'm not sure if this PR works entirely but this functionality is necessary and expected out of the box

@rowrowrowrow
Copy link

rowrowrowrow commented Nov 24, 2022

@gwdp Can you update this PR against the latest so I can test?

I did a quick test for iOS. I tried copying the the full file contents for iOS from the PR. I found that it successfully loaded a separate interface upon clicking a blob url link like 'blob:capacitor://localhost/...' but from what I could tell something stopped there. In the logs I saw the "Downloading File" message and that's the last of it. Figure my version of capacitor, mismatched with this PR or something else needs addressing, let me know.

@petrot
Copy link

petrot commented Feb 28, 2023

Any news on this PR?

@gwdp
Copy link
Author

gwdp commented Mar 15, 2023

Hi all! We never got traction on this from the Ionic team. For that reason, unfortunately, I'm not rebasing on making any additional effort towards it until we can have some of the code reviewed by the team.
Really unlucky on this as it would bring a nice set of features to the framework besides not impacting any additional functionality. Not sure what happened here; guessing a conflict of interest. ✌️

@seemsindie
Copy link

@giralte-ionic Having an anchor tag with a blob these days is expected behavior. Please merge this commit, this is functionality that should be available out of the box.

@maximevast
Copy link

Hello,

Can we get an update on this matter ?

Thanks

@marcomaranao
Copy link

@giralte-ionic could we somehow push this forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Blob downloads don't work on iOS & Android
8 participants