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

[Bug] Custom Map Style loading failure #2560

Open
CalypsoR opened this issue Apr 11, 2024 · 8 comments
Open

[Bug] Custom Map Style loading failure #2560

CalypsoR opened this issue Apr 11, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@CalypsoR
Copy link

Describe the bug
I am not able to load my Mapbox map style in Kepler.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Kepler.gl/demo'
  2. Select 'Base Map' tab
  3. Click on 'Add Map style'
  4. Paste style URL 'mapbox://styles/leon-sildano/clrq8rugo001g01qvbzpudmn4'
  5. Add the name 'Locala map'

Expected behavior
Add style button should be selectable.

Screenshots
Capture d’écran 2024-04-11 à 15 03 54

Desktop (please complete the following information):

  • OS: IOS
  • Browser Chrome
  • Version 123.0.6312.105 (Build officiel) (x86_64)
@CalypsoR CalypsoR added the bug Something isn't working label Apr 11, 2024
@akre54
Copy link
Collaborator

akre54 commented Apr 18, 2024

Confirmed this is reproducible. Here's the stack trace with this url:

       Failed to load resource: net::ERR_FAILED
bundle.js:82 Error: Failed to fetch
    at bundle.js:19:128310
_onEvent @ bundle.js:82
bundle.js:419 Fetch API cannot load mapbox://mapbox.mapbox-streets-v7,examples.0fr72zt8. URL scheme "mapbox" is not supported.
(anonymous) @ bundle.js:419
bundle.js:419 Fetch API cannot load mapbox://sprites/examples/cjikt35x83t1z2rnxpdmjs7y7@2x.json. URL scheme "mapbox" is not supported.
(anonymous) @ bundle.js:419
bundle.js:419 Fetch API cannot load mapbox://sprites/examples/cjikt35x83t1z2rnxpdmjs7y7@2x.png. URL scheme "mapbox" is not supported.
(anonymous) @ bundle.js:419
3bundle.js:82 Error: Failed to fetch
    at bundle.js:19:128310
_onEvent @ bundle.js:82

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 18, 2024

@ilyabo @birkskyum Some leftovers from the maplibre transition?

@birkskyum
Copy link
Collaborator

birkskyum commented Apr 18, 2024

The bug with the "Add style" button not being clickable actually predates the work on maplibre transition.

This bug was i.e. reported here Nov 29, 2023:

The maplibre transition PR merged and published Dec 19-21, 2023

That said, MapLibre iirc also requires the style to be a normal http(s):// url instead of the mapbox:// - I think we can look up somewhere what the actual url to prepend is if the mapbox api license allow for it.

@akre54
Copy link
Collaborator

akre54 commented Apr 19, 2024

Looks like the issue (or one issue) is that the confirmButton in the AddMapStyle modal is always disabled. It's checking for mapStyle.inputStyle.style which is always null since the input onChange listener only sets a url and not a style property. Also addCustomMapStyleUpdater is checking for state.inputStyle.id which is also null.

Making these changes seems to help:

diff --git a/src/components/src/modal-container.tsx b/src/components/src/modal-container.tsx
index a26e7579..77152824 100644
--- a/src/components/src/modal-container.tsx
+++ b/src/components/src/modal-container.tsx
@@ -434,7 +434,7 @@ export default function ModalContainerFactory(
               onConfirm: this._onAddCustomMapStyle,
               confirmButton: {
                 large: true,
-                disabled: !mapStyle.inputStyle.style,
+                disabled: !mapStyle.inputStyle.url,
                 children: 'modal.button.addStyle'
               }
             };
diff --git a/src/components/src/modals/add-map-style-modal.tsx b/src/components/src/modals/add-map-style-modal.tsx
index f5e27343..72703488 100644
--- a/src/components/src/modals/add-map-style-modal.tsx
+++ b/src/components/src/modals/add-map-style-modal.tsx
@@ -194,7 +196,7 @@ function AddMapStyleModalFactory() {
                 <InputLight
                   type="text"
                   value={inputStyle.url || ''}
-                  onChange={({target: {value}}) => this.props.inputMapStyle({url: value})}
+                  onChange={({target: {value}}) => this.props.inputMapStyle({url: value, id: 'Custom Style
' })}
                   placeholder="e.g. https://basemaps.cartocdn.com/gl/positron-gl-style/style.json"
                 />
               </StyledModalSection>

Of course, the icon is broken, but this appears to be the basic fix.

@akre54
Copy link
Collaborator

akre54 commented Apr 19, 2024

The getStyleImageIcon called from inputMapStyleUpdater is returning a Mapbox url even for non-mapbox basemaps. For instance, inputting the example url "https://basemaps.cartocdn.com/gl/positron-gl-style/style.json" gives me "https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl-style/style.json/static/-122.3391,37.7922,9,0,0/400x300?access_token={snip}&logo=false&attribution=false"

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 29, 2024

It doesn't really make sense to continue supporting Mapbox URLs unless the actual Mapbox library is added as an alternative option

Let's decide that we are going to remove the mapbox URL protocol. However, to avoid future confusion, we should do it right.

There are a bunch of references to in docs and code to mapbox:// url protocol. There could also be some screenshots.
If we no longer support it, that should be at least superficially cleaned up, in a quick PR.

The functionality to load mapbox V1 styles works after a bugfix. I don't feel like having a preview icon is enough reason to break this workflow. What do you think?

If we can keep support for V1 styles that would be preferable, but I am still confused as to what is supported in maplibre and what is not.

@birkskyum
Copy link
Collaborator

birkskyum commented Apr 29, 2024

Any mapbox v1 style will still be supported by maplibre, albeit when passed as http://, not mapbox://, because there has yet to be introduced a breaking change in the style spec. The style spec is in general quite stable and changes mainly by addition.

@akre54
Copy link
Collaborator

akre54 commented Apr 29, 2024

So then it seems reasonable to not support mapbox:// for base urls, and instead prefer http resources.

I've seen a few cases where assets within map style jsons are loaded via mapbox://, which might necessitate rewriting the urls or dropping support, but I think fixing the loading is probably enough for now. My preference would be for this to happen in maplibre but I understand if / why that's less than desirable. We can revisit if there's enough interest. Hows that sound?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants