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

Default marker icon doesn't load (because of race condition?) #7202

Open
IvanSanchez opened this issue Jul 7, 2020 · 9 comments · May be fixed by #7203 or #7204
Open

Default marker icon doesn't load (because of race condition?) #7202

IvanSanchez opened this issue Jul 7, 2020 · 9 comments · May be fixed by #7203 or #7204
Labels

Comments

@IvanSanchez
Copy link
Member

Lately I've been hitting a heisenbug regarding the dreaded autodetection of the path for L.Icon.Default (see #7092, #6496, and others).

In order to reproduce:

  • Clear your browser cache (or go into private mode)
  • Create a map with (at least) one default L.Marker
  • Load the map and wish that your browser evaluates the Leaflet JS code before it evaluates the CSS code

One out of every 20 times, I get something like

imagen

Refreshing the page (with the Leaflet JS/CSS code cached) will make the default marker appear (with the right L.Icon.Default URL):

imagen

I'm able to reproduce this in a fairly consistent manner using Firefox 77 (on Debian), private mode, network console open, cache disabled.

@IvanSanchez IvanSanchez added the bug label Jul 7, 2020
@IvanSanchez
Copy link
Member Author

I was hitting a similar issue when I was doing a bit of svelte-leaflet last week, so one way of working around this would be polling for the Leaflet CSS to be loaded, and delay the map's whenReady() until the code is fairly sure that the Leaflet CSS is there. I'm not aware of any browser API that can tell the JS code when some (new) CSS is being applied to an element.

Another approach would be to (finally) succumb to embedding the default icons as data: or blob: URIs in the JS code and remove all the path autodetection stuff.

I don't like either way. 😕

@johnd0e
Copy link
Collaborator

johnd0e commented Jul 8, 2020

one way of working around this would be polling for the Leaflet CSS to be loaded

What if instead of loading css you add style with JS?

@IvanSanchez
Copy link
Member Author

What if instead of loading css you add style with JS?

Based on my experience with svelte-leaflet, importing the Leaflet CSS devolve into the usual webpack/parcel/rollup bundling (there are like 20 ways of importing the Leaflet CSS, and you don't know which one the dev will use).

Say I wanted to have my build system grab the Leaflet CSS as a JS string, so my code could create a <style>, fill it, append it to the DOM, and do something on the next animationFrame. Then I would depend on rollup-plugin-string for import leafletcss from 'leaflet/dist/leaflet.css' to work, and everything would break if a dev builds the code using e.g. rollup-plugin-css-only (ditto for webpack config).

An approach like #4586, and having each Leaflet module/class add its own CSS could work, but that would involve hard-coding the CSS inside each Leaflet JS modules (or making a mess of the rollup-based build system so a bunch of ESM modules is created as output).

@johnd0e
Copy link
Collaborator

johnd0e commented Jul 8, 2020

there are like 20 ways of importing the Leaflet CSS, [...]

But that's slightly another problem. You've open the issue about vanilla Leaflet, haven't you?

With all that bundlers things get too unpredictable, so may be we should use some different approach, not css.
For example:

  1. As you described in 1st post, sometimes path detection just fails. In that case Leaflet tries to load icons without path.
    I suppose we could fix most cases if we would use standard path (images/) as default instead.

  2. And when we actually need to override the path - we already have another way for that (L.Icon.Default.imagePath).

@IvanSanchez
Copy link
Member Author

You've open the issue about vanilla Leaflet, haven't you?

Yes, the problem is reproducible with vanilla leaflet under the right browser/network conditions, as long as the browser decides to finish loading leaflet.css and leaflet-src.js at roughly the same time:

imagen

So far I've had luck reproducing it using any plunkr demo (e.g. https://plnkr.co/edit/PkoIbkLQfcONE5VI ), with Firefox 77 and Firefox 78 in two different Linux boxes. Using a non-plunkr simpler page (e.g. loading a local checkout of https://github.com/Leaflet/Leaflet/blob/master/debug/map/markers.html over file:///) makes my browser do less queued network requests, so it finishes loading the CSS 100msec earlier and the bug doesn't trigger. I guess it would be possible to tweak a webserver to add a synthetic delay to CSS files to trigger this easily.

As you described in 1st post, sometimes path detection just fails. In that case Leaflet tries to load icons without path.
I suppose we could fix most cases if we would use standard path (images/) as default instead.

So the plunkr use case would try to use https://plnkr.co/images as the path, and fail. 😞

@IvanSanchez
Copy link
Member Author

IvanSanchez commented Jul 8, 2020

Oh my oh my.

https://developer.mozilla.org/es/docs/Web/API/Document/currentScript

We can pull the <script> DOM element being processed, get its src, detect the path from there, and fall back to the CSS hack for IE10/IE11. Even throw a warning if the script name doesn't contain leaflet for the webpack gang.

Why didn't we discover this earlier?!


Oh wait, that's the <script> that adds a marker to the map, not the Leaflet <script> implementing the functionality being called 😖

@johnd0e
Copy link
Collaborator

johnd0e commented Jul 8, 2020

Well, Leaflet could find its own location in this way:

<link id="leaflet-css" type="text/css" rel="stylesheet" href="https://unpkg.com/leaflet/dist/leaflet.css" />
document.querySelector('link#leaflet-css').href

This will work as long as linking convention is obeyed.
And will break if id not specified, or style inlined, or [whatever else bundlers could do].

But combing this with current method (incl. possibility to override L.Icon.Default.imagePath) we would cover most cases.

@IvanSanchez
Copy link
Member Author

IvanSanchez commented Jul 8, 2020

document.querySelector('link#leaflet-css').href

And will break if id not specified, or style inlined, or [whatever else bundlers could do].

I... like this. And after reading https://www.w3.org/TR/2011/REC-css3-selectors-20110929/#attribute-substrings , I think it could be done by checking if the last characters of the href attribute match a string, like...

document.querySelector('link[href$="leaflet.css"]').href

...since we only have dist/leaflet.css. I'd be OK with throwing a console.warn if no <link> element is found, prompting the dev to specify the path for the default image.

@johnd0e
Copy link
Collaborator

johnd0e commented Jul 8, 2020

I... like this

Something like this 411e6ea

(Haven't seen two recent PRs yet)

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