fix(Android): Apply WebView settings before loading content #2766
Conversation
TheAlmightyBob
requested review from
Titozzz and
jamonholmgren
as code owners
|
@Titozzz @jamonholmgren Could I get a quick approval on this small bug fix? Can't approve my own PRs. (as noted in the description, the actual fix is just moving one line in one file.. the rest is about adding an example... though if you have concerns about that I welcome you to share them) |
|
Do you have discord ? I'd like to chat about the changes 🙂 |
|
@Titozzz I don't. I'm in the Infinite Red React Native Slack though (and, more specifically, in the React Native Webview channel)? And I'm not opposed to Discord, I just don't currently use it. |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Hello bot yes that's a mistake please keep this open |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
…guish the "require(...)" case from the "uri: 'file://...'" case (saying "import" instead of "require" just because "require" is too vague a word...)
…WebView on Android, because otherwise we start loading the content before relevant WebView settings have been applied (for example, allowFileAccess, meaning that initial loading of local files would fail) (this is not an Android-specific requirement, but "source" was already at the end for other platforms)
…de the local file test on Windows/Mac
(was broken by changes to react-native-test-app)
TheAlmightyBob
force-pushed
the
FixAndroidLocalFiles
branch
from
15e664c
to
db2458f
Compare
|
Welp, after merging the new architecture changes, I'm no longer seeing this change fix the issue (that is, even with (I'm not 100% certain if the new arch changes are related or a coincidence, but if I revert to before those changes it works as expected... it's possible that this was a change in React Native itself and just upgrading that as part of the new arch work made the difference...) At this point, my best guess for a "proper" fix would be to override |
|
Hm. The Windows CI failure is:
It worked before, so I'm guessing this is because the Windows CI was updated to target newer versions as part of the new architecture changes, and |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Hi bot please keep open... for now... |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Can you please reopen this one? We're still facing this bug with Fabric enabled in the current latest version of |
|
@ArGeoph I'll reopen, but your comment that you're seeing a bug "with Fabric enabled" tells me that this PR won't help, at least in its current state. This bug predates the new architecture work (so is not specific to Fabric being enabled), and my last update was that the changes here no longer seem to help after rebasing on the new architecture changes. Unfortunately I think there's still a lot of work to do here... |
Thanks for the reply! I'll see if I can do anything to fix this issue and open a PR if I find a solution. |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
Fixes #2723. As mentioned in that issue, the problem was that the
11.22.0refactor moved thesourceproperty before others when renderingNativeWebViewon Android, which meant thatallowFileAccesswas not set until after the WebView was already trying (and failing) to load a local file. Other platforms were unaffected because thesourceproperty was not moved in those files.Added example from @jacobpenny. Modified to use react-native-blob-util instead of react-native-file-access to populate the local filesystem because the latter broke the macOS build. The former does not support macOS or Windows, so the example is disabled on those platforms, but at least it doesn't break the build (it did require a tweak to the
build.gradlefile for Android, related to this issue again... I'm guessingreact-native-blob-utilneeds to update its React Native dependency, but I'm not 100% certain...)(The
podfile.lockchange is just because that's been getting updated any time I runpod installon the example project for a long time now so I finally decided maybe I should actually commit the updated version)To be extra-clear: The actual fix here is just a tiny change in
WebView.android.tsx. Everything else is just about adding the example.