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

L.Icon.Default brings a wrong image url #4968

Closed
ArminMueller opened this issue Sep 28, 2016 · 90 comments
Closed

L.Icon.Default brings a wrong image url #4968

ArminMueller opened this issue Sep 28, 2016 · 90 comments
Labels
Milestone

Comments

@ArminMueller
Copy link

@ArminMueller ArminMueller commented Sep 28, 2016

  • [ x] I'm reporting a bug, not asking for help
  • I've looked at the documentation to make sure the behaviour is documented and expected
  • [x ] I'm sure this is a Leaflet code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
  • I've searched through the issues to make sure it's not yet reported

The image url leaflet prsents me is https://uismedia.geo-info-manager.com/apis/leaflet_1/imagesmarker-icon-2x.png. It seems there is a missing "/"
Additionally i have an error
leaflet.min.js:5 GET https://uismedia.geo-info-manager.com/apis/leaflet_1/images/ 403 (Forbidden)

@IvanSanchez
Copy link
Member

@IvanSanchez IvanSanchez commented Sep 28, 2016

  • Is there any public webpage in your server we can visit, so we can reproduce the problem ourselves?
  • What OS and web browser are you using?
@jasongrout
Copy link

@jasongrout jasongrout commented Sep 28, 2016

leaflet.min.js:5 GET https://uismedia.geo-info-manager.com/apis/leaflet_1/images/ 403 (Forbidden)

This may be from the same issue discussed in #4849

@IvanSanchez
Copy link
Member

@IvanSanchez IvanSanchez commented Sep 28, 2016

Indeed. Which why I'm curious as to know the circumstances this can be reproduced, so we can make unit tests against those.

@Greigrm
Copy link

@Greigrm Greigrm commented Oct 2, 2016

I had the same issue moving from RC3 to 1.0.1 - in my code I had the line L.Icon.Default.imagePath = 'images'; - can't remember quite why this was, but commenting it out solved the issue - might be worth checking you don't have similar

@codeofsumit
Copy link
Contributor

@codeofsumit codeofsumit commented Oct 15, 2016

suddenly got the same issue in two completely different projects, both using webpack and leaflet.
If I add markers to the map, the images aren't found. Browser throws this error:
image

@codeofsumit
Copy link
Contributor

@codeofsumit codeofsumit commented Oct 16, 2016

ok I got something.

So a marker looks like this currently because the images (icon and shadow) aren't found.
image

this function in leaflet:

_getIconUrl: function (name) {
    if (!L.Icon.Default.imagePath) {    // Deprecated, backwards-compatibility only
        L.Icon.Default.imagePath = this._detectIconPath();
    }

    // @option imagePath: String
    // `L.Icon.Default` will try to auto-detect the absolute location of the
    // blue icon images. If you are placing these images in a non-standard
    // way, set this option to point to the right absolute path.
    return (this.options.imagePath || L.Icon.Default.imagePath) + L.Icon.prototype._getIconUrl.call(this, name);
},

causes the data:image urls to have the following string at the end of the url:
")marker-icon-2x.png.

The name of the file can be removed if you delete + L.Icon.prototype._getIconUrl.call(this, name). The ") part is probably from the _detectIconPath regex magic. I can't fix that so I just tried slicing the last two chars off, which results in this function:

_getIconUrl: function (name) {
    if (!L.Icon.Default.imagePath) {    // Deprecated, backwards-compatibility only
        L.Icon.Default.imagePath = this._detectIconPath();
    }

    // @option imagePath: String
    // `L.Icon.Default` will try to auto-detect the absolute location of the
    // blue icon images. If you are placing these images in a non-standard
    // way, set this option to point to the right absolute path.
  var url = (this.options.imagePath || L.Icon.Default.imagePath);

  return url.slice(0, - 2);
},

and here we go, the icon shows up. One last issue is that the shadow image is a marker icon as well - the src path is already wrong, I have no idea why (yet). So a marker now look like this:

image

@IvanSanchez does this help you a bit narrowing down the issue?

@perliedman
Copy link
Member

@perliedman perliedman commented Oct 17, 2016

@codeofsumit would be nice to step through _detectIconPath and see what's going on there, especially what value does the path variable have before it's passed through the regex.

@codeofsumit
Copy link
Contributor

@codeofsumit codeofsumit commented Oct 23, 2016

@IvanSanchez @perliedman I think I found the bug.

This is _detectIconPath

_detectIconPath: function () {
    var el = L.DomUtil.create('div',  'leaflet-default-icon-path', document.body);
    var path = L.DomUtil.getStyle(el, 'background-image') ||
               L.DomUtil.getStyle(el, 'backgroundImage');   // IE8
    document.body.removeChild(el);

    return path.indexOf('url') === 0 ?
        path.replace(/^url\([\"\']?/, '').replace(/marker-icon\.png[\"\']?\)$/, '') : '';
}

the path variable is something like this:
url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAApCAYAAADAk4LOAAAFgUlEQ…n3EUrUZ10EYNw7BWm9x1GiPssi3GgiGRDKWRYZfXlON+dfNbM+GgIwYdwAAAAASUVORK5CYII=").

Which is correct.

Now the regex wants to extract the data.image url from this, and it returns this:

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAApCAYAAADAk4LOAAAFgUlEQ…n3EUrUZ10EYNw7BWm9x1GiPssi3GgiGRDKWRYZfXlON+dfNbM+GgIwYdwAAAAASUVORK5CYII=")

Notice the last ") that isn't removed from the path variable. The regex /^url\([\"\']?/ only targets url(" at the beginning of the path, not the ") at the end.
Using this regex works:

return path.indexOf('url') === 0 ?
    path.replace(/^url\([\"\']?/, '').replace(/\"\)$/, '').replace(/marker-icon\.png[\"\']?\)$/, '') : '';

This fixes the image problem, together with

var url = (this.options.imagePath || L.Icon.Default.imagePath);

inside_getIconUrl. But it doesn't fix the shadow - I still don't know whats going on with the shadow.

@raduflp
Copy link

@raduflp raduflp commented Oct 23, 2016

having the same issue,
the path value in _detectIconPath is something like "url("data:image/png;base64,i...5CYII=")"
and it looks like _getIconUrl and _detectIconPath weren't designed to handle data URLs

@raduflp
Copy link

@raduflp raduflp commented Oct 23, 2016

the shadow issue seems to be cause by the fact that in _getIconUrl the value for L.Icon.Default.imagePath is already set with the marker data URL, so the image for marker is used and stretched
image

could it be related to this commit hardcoding the marker image?
837d190

@codeofsumit
Copy link
Contributor

@codeofsumit codeofsumit commented Oct 23, 2016

@Radu-Filip I was able to fix it with these modifications to your PR - I'm not sure what other effects this might have though:
image

The problem is - as you said - that the default URL is the marker image, for all icons basically.
So first of all I added the default URL for the shadow to the CSS:

/* Default icon URLs */
.leaflet-default-icon-path {
    background-image: url(images/marker-icon.png);
}

.leaflet-default-shadow-path {
    background-image: url(images/marker-shadow.png);
}

Then I passed the name from _getIconUrl to _detectIconPath and used it to add the class to the element from which the path is extracted:

_getIconUrl: function (name) {

  L.Icon.Default.imagePath = this._detectIconPath(name);

    // @option imagePath: String
    // `L.Icon.Default` will try to auto-detect the absolute location of the
    // blue icon images. If you are placing these images in a non-standard
    // way, set this option to point to the right absolute path.
  var path = this.options.imagePath || L.Icon.Default.imagePath;
  return path.indexOf("data:") === 0 ? path : path + L.Icon.prototype._getIconUrl.call(this, name);
},

_detectIconPath: function (name) {
    var el = L.DomUtil.create('div',  'leaflet-default-' + name + '-path', document.body);
    var path = L.DomUtil.getStyle(el, 'background-image') ||
               L.DomUtil.getStyle(el, 'backgroundImage');   // IE8

    document.body.removeChild(el);

    return path.indexOf('url') === 0 ? path.replace(/^url\([\"\']?/, '').replace(/(marker-icon\.png)?[\"\']?\)$/, '') : '';
}

I also had to remove the if/else around detectIconPath, because it wasn't called for the shadow icon. Now everything works for the default marker icon - not sure about custom ones.

@raduflp
Copy link

@raduflp raduflp commented Oct 23, 2016

@codeofsumit yeah, i did something similar here https://github.com/Radu-Filip/Leaflet/tree/temp
and I'll use if for now as a hack. Hopefully there will be a more future proof solution down the line for those using webpack et al

@codeofsumit
Copy link
Contributor

@codeofsumit codeofsumit commented Oct 23, 2016

@Radu-Filip do you mind updating your PR with that solution? It may get merged.

@raduflp
Copy link

@raduflp raduflp commented Oct 25, 2016

@codeofsumit done, let's see if it goes through

@ghybs
Copy link
Collaborator

@ghybs ghybs commented Oct 26, 2016

Hi,

Maybe I miss something, but it seems to me that this issue of Webpack building could be simply addressed with a Leaflet plugin, that would override L.Icon.Default behaviour.

Demo: http://playground-leaflet.rhcloud.com/nexo/1/edit?html,css,output

With this approach, you get rid of any tricky RegExp, and the default marker images are inlined (by hard coding), which is one of Webpack intended result for small images anyway.

A possible downside is that each marker has its own base64 icon, not sure if browsers can cache that… (same downside for PR #5041)
We could imagine a refinement by setting it as a background image instead of placing it in the image src attribute, as done for Layers Control icon for example.
There may be a hidden trap with this idea (edit: sounds like that one), otherwise I am sure it would have been implemented long time ago, as it would have avoided the image path detection in the first place.

Demo: http://playground-leaflet.rhcloud.com/mey/1/edit?html,css,output (not taking care of retina)

The biggest advantage of the plugin approach, is that it keeps this specific behaviour for Webpack projects only.

Hope this helps.

@ghybs
Copy link
Collaborator

@ghybs ghybs commented Oct 27, 2016

BTW, it seems to me that there is something intrinsically wrong here.

Leaflet makes a "complex" path detection to images, which must be in a pre-defined place compared to the CSS file.

But webpack build process bundles that CSS and may (or not) move the images as well (and rename them!), depending on what the developer requests to webpack (like requiring images).
Therefore the Leaflet detection surely fails when webpack is used.

PR #5041 is like a trick to accept the case where webpack inlines images into the CSS, at the cost of duplicating the Base64 image into each marker. Not even talking about the cost to non-webpack users.

PR #4979 was meant just to prevent the webpack build error message (due to missing file), it does not look to handle at all the actual image path resolution.
I guess developers then manually specify the L.Icon.Default.imagePath?
@jasongrout and @Eschon, maybe you could share how you managed it?

@Eschon
Copy link
Contributor

@Eschon Eschon commented Oct 27, 2016

I don't really manage it. I just don't use the default icon so this bug wasn't a problem for me until now.

@anairamzap-mobomo
Copy link

@anairamzap-mobomo anairamzap-mobomo commented Nov 2, 2016

Hi, just a note to say I can reproduce this path error using the 1.0.1 version of this library.
I'm using it along with the leaflet Drupal module (7.x-1.x-dev), and here there is an issue reported to the module: https://www.drupal.org/node/2814039 in case its useful.

As far as I can see the "problem" is on _getIconUrl function? as after L.Icon.Default.imagePath there is a missing slash, so the image path for example in Drupal gets generated like this "/sites/all/libraries/leaflet/imagesmarker-icon.png". Between the images path and the marker image filename (marker-icon.png) should be a slash /.

@ghybs
Copy link
Collaborator

@ghybs ghybs commented Nov 3, 2016

Hi @anairamzap-mobomo,

Sounds like what you report is a different issue.

Unfortunately, as it seems to involve a port to a framework (Drupal), you would have first to make sure that the bug is not related to how that port is working.

Leaflet 1.0.x with vanilla JS correctly includes the trailing slash: http://playground-leaflet.rhcloud.com/fosa/1/edit?html,output

See for instance http://cgit.drupalcode.org/leaflet/tree/leaflet.module#n51, where L.Icon.Default.imagePath is overriden by the Drupal module.

Looks like that module does not handle the change between Leaflet 0.7.x and 1.0.x, where the slash must now be included in L.Icon.Default.imagePath.

As Leaflet 1.0.0 is a major release, I guess there is no commitment for backward compatibility.

@anairamzap-mobomo
Copy link

@anairamzap-mobomo anairamzap-mobomo commented Nov 3, 2016

hey @ghybs I see... I will contact the Drupal module maintainers to let them know this. As you said, it sounds that they have to modify the module to fit well with the 1.0.x version of the library, or at least add some lines to the docs warning about this.

Thanks for your feedback!

@ajoslin103
Copy link

@ajoslin103 ajoslin103 commented Nov 27, 2016

I'm having the exact same issue as originally reported -- in an aurulia skeleton/esnext+webpack project.

Until this is fixed I've copied the images out to my source folder and am using a custom marker -- omitting size/placement info seems to be ok...

		var customDefault = L.icon({
			iconUrl: 'images/marker-icon.png',
			shadowUrl: 'images/marker-shadow.png',
		});
@crob611
Copy link

@crob611 crob611 commented Dec 1, 2016

Wanted to share what I did to workaround the invalid data:url issue. Basically set the default icon to a new one that uses the provided marker icon and shadow, but will let webpack handle the data encoding of those images individually. Just include a file like this somewhere.

import L from 'leaflet';

import icon from 'leaflet/dist/images/marker-icon.png';
import iconShadow from 'leaflet/dist/images/marker-shadow.png';

let DefaultIcon = L.icon({
    iconUrl: icon,
    shadowUrl: iconShadow
});

L.Marker.prototype.options.icon = DefaultIcon;

Could probably be tweaked to include the retina icon as well.

@disarticulate
Copy link

@disarticulate disarticulate commented Dec 15, 2018

@ghybs thanks for the hotfix. I've ran into this bug a few times in different projects. This entire thread seems absurd that its not fixed or isn't seen as a problem.

@alexanderankin
Copy link

@alexanderankin alexanderankin commented Jan 4, 2019

google brought me here because using the library with Webpack was giving me this error.

@javiertury
Copy link

@javiertury javiertury commented Jan 20, 2019

Does anyone know why aren't those images inlined as svg?

I think this could easily solved with postcss and postcss-inline-svg. The icons would become inline svg files instead of external png. The icons would be crispier as this problem goes away.

@IvanSanchez
Copy link
Member

@IvanSanchez IvanSanchez commented Jan 20, 2019

Does anyone know why aren't those images inlined as svg?

Legacy browser support.

@javiertury
Copy link

@javiertury javiertury commented Jan 20, 2019

Thanks @IvanSanchez

Then I see two potential solutions. One is to inline the images as base64 encoded .png. The other alternative is to inline .svg icons and make devs that target legacy platforms to override the default icons.

Is any of these solutions worth a pull request?

Out of all the browsers supported by leaflet, the following don't have support for svg (caniuse).

  • IE 7 and 8
  • Android browser 2.*
@ghybs
Copy link
Collaborator

@ghybs ghybs commented Jan 20, 2019

inline the images as base64

See #4968 (comment)

ewlarson added a commit to geoblacklight/geoblacklight that referenced this issue Jan 25, 2019
Fixes #706 / FeatureLayer icons do not display correctly as a result of multiple issues. Vendorizing an asset_url "friendly" version of Leaflet 1.4.0 fixes our display issue.

Reasons for local vendorized leaflet:
* leaflet-rails icon path issue remains open: axyjo/leaflet-rails#81
* The leaflet-rails gem appears unmaintained
* Leaflet's icon "path" regex bug breaks image url path: Leaflet/Leaflet#4968
* Overriding L.Icon.Default icon options still runs into the "path" bug
* Rails asset pipeline fingerprinting complicates leaflet icon path
ewlarson added a commit to geoblacklight/geoblacklight that referenced this issue Jan 29, 2019
Fixes #706 / FeatureLayer icons do not display correctly as a result of multiple issues. Vendorizing an asset_url "friendly" version of Leaflet 1.4.0 fixes our display issue.

Reasons for local vendorized leaflet:
* leaflet-rails icon path issue remains open: axyjo/leaflet-rails#81
* The leaflet-rails gem appears unmaintained
* Leaflet's icon "path" regex bug breaks image url path: Leaflet/Leaflet#4968
* Overriding L.Icon.Default icon options still runs into the "path" bug
* Rails asset pipeline fingerprinting complicates leaflet icon path

FeatureLayer: use nearby method to fetch feature data

Fixes #717 / Switching intersects() for nearby() returns data as expected

Rubocop: AlignParameters
ewlarson added a commit to geoblacklight/geoblacklight that referenced this issue Jan 29, 2019
Fixes #706 / FeatureLayer icons do not display correctly as a result of multiple issues. Vendorizing an asset_url "friendly" version of Leaflet 1.4.0 fixes our display issue.

Reasons for local vendorized leaflet:
* leaflet-rails icon path issue remains open: axyjo/leaflet-rails#81
* The leaflet-rails gem appears unmaintained
* Leaflet's icon "path" regex bug breaks image url path: Leaflet/Leaflet#4968
* Overriding L.Icon.Default icon options still runs into the "path" bug
* Rails asset pipeline fingerprinting complicates leaflet icon path

FeatureLayer: use nearby method to fetch feature data

Fixes #717 / Switching intersects() for nearby() returns data as expected

Rubocop: AlignParameters
eliotjordan added a commit to geoblacklight/geoblacklight that referenced this issue Jan 29, 2019
Fixes #706 / FeatureLayer icons do not display correctly as a result of multiple issues. Vendorizing an asset_url "friendly" version of Leaflet 1.4.0 fixes our display issue.

Reasons for local vendorized leaflet:
* leaflet-rails icon path issue remains open: axyjo/leaflet-rails#81
* The leaflet-rails gem appears unmaintained
* Leaflet's icon "path" regex bug breaks image url path: Leaflet/Leaflet#4968
* Overriding L.Icon.Default icon options still runs into the "path" bug
* Rails asset pipeline fingerprinting complicates leaflet icon path

FeatureLayer: use nearby method to fetch feature data

Fixes #717 / Switching intersects() for nearby() returns data as expected

Rubocop: AlignParameters
@ghost
Copy link

@ghost ghost commented Mar 17, 2019

Had to add anchor and size too to make it work, e.g.

   import icon from 'leaflet/dist/images/marker-icon.png';
   import iconShadow from 'leaflet/dist/images/marker-shadow.png';

   let DefaultIcon = L.icon({
      iconUrl: icon,
      shadowUrl: iconShadow,
      iconSize: [24,36],
      iconAnchor: [12,36]
    });

    L.Marker.prototype.options.icon = DefaultIcon; 
@mbrucher
Copy link

@mbrucher mbrucher commented Mar 29, 2019

I also have the same issue (webpack using Flask, so all the elements are supposed to be in a static folder), but @Giorgi-M fix is not enough, as I get a '"exports" is read-only' error (Firefox, seems to be linked to the png imports?).
I see that the issue is closed, but we still see issues with 1.4.0, so I wonder what the fix is?

@drakej
Copy link

@drakej drakej commented Apr 4, 2019

Seeing this issue with vue2-leaflet 2.0.2 and leaflet 1.4.0.

@disarticulate
Copy link

@disarticulate disarticulate commented Apr 14, 2019

this appears to have existed for quite awhile, and half of the presented solutions don't seem to work.

Has anyone figured out the root of this problem?

@skjortan23
Copy link

@skjortan23 skjortan23 commented Apr 14, 2019

i am having the same problem with versions "vue2-leaflet": "2.0.3" leaflet "leaflet": "1.4.0".

also running webpack.

@manelclos
Copy link

@manelclos manelclos commented Apr 14, 2019

We are successfully using vue2-leaflet 2.0.3 and leaflet 1.4.0 using a solution found in this same issue:

import L from 'leaflet'
require('../../node_modules/leaflet/dist/leaflet.css')

// FIX leaflet's default icon path problems with webpack
delete L.Icon.Default.prototype._getIconUrl
L.Icon.Default.mergeOptions({
  iconRetinaUrl: require('leaflet/dist/images/marker-icon-2x.png'),
  iconUrl: require('leaflet/dist/images/marker-icon.png'),
  shadowUrl: require('leaflet/dist/images/marker-shadow.png')
})
@drakej
Copy link

@drakej drakej commented Apr 15, 2019

I guess the better question that needs to be asked is why this wasn't fixed with the merged code which caused the issue to be closed? Since a workaround that works is great, but this should work out of the box and still needs to be an open issue.

@ghybs
Copy link
Collaborator

@ghybs ghybs commented Apr 15, 2019

Dear all,

Leaflet works out-of-the-box.

Webpack (and other build engines) combined with Leaflet does not work out-of-the-box.

Please refer to solution proposed in #4968 (comment): leaflet-defaulticon-compatibility

While I can understand that many developers would have preferred such feature to be incorporated directly in Leaflet core, it has been argued that the added code is useless to a majority of end-users. Therefore, aligning with Leaflet spirit of keeping its core simple, I decided to make it a plugin.

With webpack, another typical solution, once you have configured your loaders:

import L from 'leaflet';
delete L.Icon.Default.prototype._getIconUrl;

L.Icon.Default.mergeOptions({
  iconRetinaUrl: require('leaflet/dist/images/marker-icon-2x.png'),
  iconUrl: require('leaflet/dist/images/marker-icon.png'),
  shadowUrl: require('leaflet/dist/images/marker-shadow.png'),
});

There are other solutions proposed higher in this thread for other build engines and framework combinations.

To prevent these solutions from being further buried under comments, I am going to lock this thread.

Thank you all for the shared solutions! 👍

@Leaflet Leaflet locked as resolved and limited conversation to collaborators Apr 15, 2019
@cherniavskii cherniavskii pinned this issue Nov 5, 2019
@johnd0e johnd0e unpinned this issue May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.