-
-
Notifications
You must be signed in to change notification settings - Fork 749
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 GeckoView tile loading android assets folder #4478
Fix GeckoView tile loading android assets folder #4478
Conversation
…oid" (i.e., the assets folder) in GeckoView on Android
What about ajax.ts usage of file? |
No other changes are required. "file://" isn't referenced anywhere else in the code base, and, it's only this check that is causing an issue. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4478 +/- ##
==========================================
- Coverage 87.84% 87.65% -0.20%
==========================================
Files 246 246
Lines 33421 33421
Branches 2219 2205 -14
==========================================
- Hits 29359 29295 -64
- Misses 3043 3124 +81
+ Partials 1019 1002 -17 ☔ View full report in Codecov by Sentry. |
See here: maplibre-gl-js/src/util/ajax.ts Line 140 in 04e39c5
I'll be surprised if this wouldn't need a patch as well... |
You are right, I was just looking at that line. However, if I just patch the one line in the commit, it all springs into life. I'll do a bit more digging. Thanks. |
It looks like the code is trying to avoid using the Fetch API when using "file://" and instead drop back to the legacy XMLHttpRequest. It's not clear to me why, as I can't find anything in Mozilla Fetch API documentation (https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) or Chrome documentation (https://web.dev/articles/introduction-to-fetch) that states that the Fetch API can't work with a "file://" request (or "resource://android" in my case). I've done some tests an using GeckoView on Android and both the Fetch API and XMLHttpRequest successfully load the fonts and glyphs. I've had a quick look at Chromium on Windows too using "file://" and this is the same, both Fetch API and XMLHttpRequest seem to work fine. So, I think this check is redundant, though, I'm not keen to back it out, even if not needed, as I assume it was added for an older browser than may not support "file://" access via the Fetch API? Removing it may cause a breakage for someone else, but, with a modern browser, it doesn't appear to be needed anymore. |
I can find old articles about fetch not supporting file protocol: It might be that this is no longer relevant, IDK... |
If you agree, maybe the best option for now is to apply this fix for GeckoView, and the use of the Fetch API vs XMLHttpRequest can be looked at at a later point? A change to ajax.ts isn't needed to resolve the specific issue for GeckoView :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not.
Fixes #4451