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

fix(ios): Entry.toURL() to produce DOM-usable uri when using scheme-hosted webview #642

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

breautek
Copy link
Contributor

@breautek breautek commented Oct 30, 2024

Platforms affected

iOS

Motivation and Context

Entry's toURL() method is intended to produce a DOM-usable URIs.
This works as intended on Android.

On iOS however, it only worked if the app was configured to be in a file:// hosted webview. If the webview was configured to use schemes, it would still produce a file:// uri which is not usable in the DOM.

I believe this can be considered a patch, for file:// hosted users, this should behave the same as before. For scheme-hosted users, the .toURL() will return a different URI, but the original format didn't work as intended. If the user were using .toURL() for another purpose, then this could be considered a breaking change?

Description

Mimick how Android solves this issue, by creating an ios specific Entry.js file which contains a toURL method override with iOS specific logic.

Testing

Manually tested with and without the scheme preference to observe toURL() producing file:// urls while in file:// hosted mode, or app:// uris if the app is using scheme preference.

Also tested completely custom schemes using:

<preference name="scheme" value="myapp" />
<preference name="hostname" value="appname" />

and observing URIs such as myapp://appname/_app_file_/var/mobile/Containers/Data/Application/84D10AA8-302C-4295-8690-54F4C4EA9BB2/tmp/cdv_photo_1730252303.jpg.

Confirmed that in both cases, the produced URIs are usable in the DOM to properly load image assets, for example.

Unit Test Refactoring

Now that .toURL() returns a app:// uri instead of a file:// uri, it broke several tests that used .toURL() to resolve a file entry. app:// uris return an encoding error, presumably because there is no filesystem implementation to handle the scheme.

Most cases I've replaced the usage of .toURL() with .nativeURL to make the test pass. However there is a handful of tests that was specifically calling .toNativeURL() to test the resolve method and .toNativeURL() simply calls .toURL() with a deprecation notice. I think these tests can be complete as I believe they are an out-dated practice. .toNativeURL() is also deprecated, but I've kept them for now, just disabled them until we decide what to do.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@breautek breautek marked this pull request as draft October 30, 2024 03:53
@breautek
Copy link
Contributor Author

I'm putting this in draft.

Good news is, this PR actually addresses that failing test that we've had for some time:

resolveLocalFileSystemURL for cdvfile
✓ file.spec.147 should be able to resolve cdvfile applicationDirectory fs root

Bad news is, it introduces a lot more failing tests but I think most of them is because the tests are expecting file:// uris where it will now receive app:// uri on scheme-hosted apps. So I think it just a matter of making the tests be aware of the iOS changes. I'll try to go through them tomorrow night.

@breautek breautek requested a review from dpogue October 31, 2024 22:40
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me.

Taking a step back (and keeping in mind that it's been several years since I used the File plugin): it's not clear to me what toURL() and nativeURL are meant to do exactly, and there doesn't seem to be any documentation beyond the deprecated W3C spec that says "toURL() returns a URL" and has no mention of nativeURL.

My initial assumption is that toURL() is intending to return a DOM-usable URL, and nativeURL is the raw filesystem path, but that sounds like it's not necessarily the case?

@breautek breautek marked this pull request as ready for review November 1, 2024 03:19
@breautek
Copy link
Contributor Author

breautek commented Nov 1, 2024

My initial assumption is that toURL() is intending to return a DOM-usable URL, and nativeURL is the raw filesystem path, but that sounds like it's not necessarily the case?

I believe once upon a time, toURL() returned the cdvfile:// urls. But with scheme-based webviews, they are now considered cross-origin so they get blocked. In that sense, I do believe so, cdvfile protocol was the original "DOM-usable" urls.

.nativeURL I believe is the underlying url, so usually file:// but I think it could also be content:// on android. I'm not sure why .toNativeURL() calls and simply returns .toURL() though.

www/ios/Entry.js Outdated Show resolved Hide resolved
@breautek breautek added this to the 8.1.3 milestone Nov 2, 2024
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise looks good to me.

@breautek breautek merged commit 953e653 into apache:master Nov 3, 2024
15 of 16 checks passed
@breautek breautek deleted the fix/ios/entry.tourl branch November 3, 2024 13:31
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.

4 participants