[Bug] Fix custom map style input

[akre54]

Fix for #2560 and #2458: custom map style urls aren't being loaded.

Quoting from #2560:

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. Setting any id from the onChange handler seems to fix the issue.

The icon is also broken. 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". I'm not sure what the best fix for that would be.

[netlify]

❌ Deploy Preview for keplergl2 failed.

Name	Link
🔨 Latest commit	ff892b3
🔍 Latest deploy log	https://app.netlify.com/sites/keplergl2/deploys/663303ff5a5e8a00087f1ce5

[ibgreen]

Is it possible to separate this PR from the dependency fix so we can discuss them each on their merits?

[akre54]

Done. I had to branch off of the other one to get the fix

[ibgreen]

The icon is also broken. 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". I'm not sure what the best fix for that would be.

Looking at your URL it seems the style is expected to be a name not a URL (see the double https in https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl).

So while this PR may unbreak some thing, it is probably not a complete solution.

@ilyabo @macrigiuseppe Any thoughts?

[akre54]

I can confirm that the main issue is still broken with just a mapbox url scheme:

Screen.Recording.2024-04-19.at.2.17.18.PM.mov

[akre54]

With this fix, the mapbox:// url scheme still doesn't work. Looks like the only two places that check for a mapbox url scheme are isStyleUsingMapboxTiles (called to determine attribution in map-container.tsx) and getStyleDownloadUrl (called from getLoadMapStyleTasks)

Mapbox.url.scheme.bug.mov

[ilyabo]

Maplibre dropped the support for mapbox:// style URLs. To work around this we would probably have to load the basemap library dynamically. Maybe we can add a switch in this dialog to choose from Maplibre and Mapbox. Or revert to Mapbox. @ibgreen what do you think?

[ibgreen]

I don't think that reverting to mapbox is what we want to do. Supporting both would be an option, though that is a feature that someone needs to implement.

One option is to remove the map style support and the dialog.

How important is this feature? Are we breaking a important workflows here?

[akre54]

Would it be possible to simply rewrite the urls? Either in Kepler or in Maplibre? This feels like a pretty useful feature

Or at the very least include a warning message if you try to use a mapbox:// url scheme? This feels unexpected

[ilyabo]

I suspect that newer mapbox styles would not work correctly with maplibre. I have experienced issues when trying to use newer styles with mapbox@1.

[akre54]

I can look into the icon import. It feels odd that the importer would assume that all urls come from mapbox, but it's not clear to me that Carto, for instance, returns a unique icon for all of its styles, or that every other style provider is required to return one as well. It seems to me like this would be better if it just used a generic icon, or black square.

I do think it's fine for mapbox:// urls to be broken so long as basic style.jsons still work. What do you think?

[chrisgervang]

It doesn't really make sense to continue supporting Mapbox styles (or their URLs) unless the actual Mapbox library is added as an alternative option, right?

[akre54]

Maybe not the url protocol but it's the most commonly used map style format. What's the alternative?

And how have they diverged? 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?

[ibgreen]

Apologies that this is taking time, the basemap support in kepler clearly lacks a decisive owner right now. Together we all have parts of the answer, but the risk is that this thread could go on for a long time unless we decide on something. Following up in the issue.

[akre54]

Yeah totally agree. I don't want to bike shed this any more than necessary. I agree there needs to be a better default for the icon. If I come up a patch to make this black or a basic color or something would that be acceptable?

[ibgreen]

come up a patch to make this black or a basic color or something would that be acceptable?

Yes let's land something reasonable.

[birkskyum]

It doesn't really make sense to continue supporting Mapbox styles (or their URLs) unless the actual Mapbox library is added as an alternative option, right?

I think this is right. The reason mapbox:// was removed from maplibre was not only a matter of compatibility, but also a license compliance concern. This change in the mapbox style spec license saying:

The software and files in this repository (collectively, "Software") are
licensed under the Mapbox TOS for use only with the relevant Mapbox product(s)
listed at www.mapbox.com/pricing.

Looking at the www.mapbox.com/pricing etc. it seemed risky to interpret that in any other way than "mapbox styles coming from the mapbox api will be under latest version of this license, and are thus only allowed to be rendered with a mapbox renderer".

[akre54]

I added a temporary fix using the NO_BASEMAP_ICON for custom map styles and removing the logic for isValidStyleUrl which checked for mapbox styles. I'm sure this can be changed in the future but for now this fixes the major issues.

[akre54]

The ternary wasn't necessary because getStyleDownloadUrl already guards for urls.

[birkskyum]

seems like ci is failing on the "should set the inputStyle" test