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

0.45.0 broke setRTLTextPlugin on local source #6719

Closed
hyperknot opened this issue May 23, 2018 · 9 comments
Closed

0.45.0 broke setRTLTextPlugin on local source #6719

hyperknot opened this issue May 23, 2018 · 9 comments
Assignees

Comments

@hyperknot
Copy link

hyperknot commented May 23, 2018

mapbox-gl-js version: 0.45.0
browser: various

Steps to Trigger Behavior

  1. Save the RTL example into a static html: https://www.mapbox.com/mapbox-gl-js/example/mapbox-gl-rtl-text/
  2. Save mapbox-gl-rtl-text.js next to it
  3. Change the code to load locally:
mapboxgl.setRTLTextPlugin('/mapbox-gl-rtl-text.js')
  1. Start node serve or python3 -m http.server

Expected Behavior

Should work.

Actual Behavior

  • Doesn't load RTL plugin in Chrome.
  • Breaks all vector layers in IE 11.

OK, so this wasn't an easy one to diagnose. In IE 11 all vector layer renderings were broken. Not just text, every layer, as in nothing was displayed at all, only raster layers. Reverting to 0.44.2 makes everything work.

It turns out, the culprit is 0.45.0's setRTLTextPlugin(), but only when used with local sources. With https CDN it works well.

@jfirebaugh
Copy link
Contributor

Likely due to #6170.

If you pass an absolute local URL (e.g. http://0.0.0.0:8000/mapbox-gl-rtl-text.js) to setRTLTextPlugin, does it work? If so, we could have setRTLTextPlugin() resolve any relative URLs it receives before sending the URL on to the web worker.

@jfirebaugh
Copy link
Contributor

cc @ChrisLoer

@ChrisLoer ChrisLoer self-assigned this May 23, 2018
@hyperknot
Copy link
Author

Yes, changing it to

mapboxgl.setRTLTextPlugin(window.location.origin + '/mapbox-gl-rtl-text.js');

makes it work, including IE11.

Also, it'd be quite nice to have some kind of error thrown, so at least we receive reports that IE11 users are not seeing anything on the maps.

@ChrisLoer
Copy link
Contributor

ChrisLoer commented May 24, 2018

Thanks for the report @hyperknot, and sorry for the difficulty tracing down the source of the problem. setRTLTextPlugin actually takes an error callback that would have helped in this case, but of course you didn't know ahead of time you were looking for the problem. I think I'll try updating the example to something like this:

mapboxgl.setRTLTextPlugin(
    'https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.1.2/mapbox-gl-rtl-text.js',
    function(err) { console.log(err); }
);

I'll try to get a fix in soon for handling relative URLs. I still have to investigate what was happening on IE 11, I would have expected that the plugin would just fail to load the same way as in Chrome.

@hyperknot
Copy link
Author

hyperknot commented May 24, 2018

@ChrisLoer, thanks for the callback option!

@ChrisLoer
Copy link
Contributor

I would use the URL API to convert relative to absolute URLs, but apparently that's not supported by IE 11. @ryanhamley, do you know a canonical way to absolutify URLs?

@hyperknot
Copy link
Author

I think window.location.origin is supported in IE 11+ and probably every other browser GL JS supports.

@jfirebaugh
Copy link
Contributor

const a = document.createElement('a');
a.href = href;
return a.href;

window.location.origin + href won't work on non-root pages.

@ryanhamley
Copy link
Contributor

All the approaches I know of are some variation on what John posted or using window.location. I think those have some drawbacks. The relative path would have to be down the hierarchy (e.g. you couldn't pass in ../../some/path.js), they'd have to provide the path from the root as John said (we could do window.location.href instead of .origin to get a full path but I'm not sure what the use case is for non-root paths in this case?) and the url wouldn't be normalized so you could end up with ourapp.comsome/rel/path.js or ourapp.com//some/rel/path.js if we didn't filter for those things. I think it'd be hard to know what the user is expecting when passing in a relative path unless we assumed it was from the root, in which case window.location.origin should work I'd think. This seems like it's treating a URL as an import statement in some ways which makes it tricky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants