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

Transform SVG in RTL direction with IE11 and Edge #5371

Closed
ludohe opened this issue Mar 6, 2017 · 9 comments
Closed

Transform SVG in RTL direction with IE11 and Edge #5371

ludohe opened this issue Mar 6, 2017 · 9 comments
Labels
ie Internet Explorer

Comments

@ludohe
Copy link

ludohe commented Mar 6, 2017

How to reproduce

  • Leaflet version I'm using: 1.0.3
  • Browser (with version) I'm using: Internet Explorer 11 & edge
  • OS/Platform (with version) I'm using: windows

What behaviour I'm expecting and which behaviour I'm seeing

I use the leaflet.draw plugin version 0.4.2 to draw shape and IE11
When we draw a circle in rtl direction, the footprint moves and isn't displayed at the right place.

Minimal example reproducing the issue

Using http://jsfiddle.net/ludohe/tpkt9kyu/15/ and create a circle with IE11.

It's probably the same problem of #4754 issue

Proposition of changes

I think that IE11 doesn't accept transform inside the style tag of SVG.
I propose to change:

  1. L.DomUtil.setTransform to manage SVG:

    setTransform: function (el, offset, scale) {
    
    	var pos = offset || new L.Point(0, 0);
    	if(el.tagName !== 'svg'){
    		el.style[L.DomUtil.TRANSFORM] =
    			(L.Browser.ie3d ?
    				'translate(' + pos.x + 'px,' + pos.y + 'px)' :
    				'translate3d(' + pos.x + 'px,' + pos.y + 'px,0)') +
    			(scale ? ' scale(' + scale + ')' : '');
    	} else {
    		el.transform =
    			(L.Browser.ie3d ?
    				'translate(' + pos.x + 'px,' + pos.y + 'px)' :
    				'translate3d(' + pos.x + 'px,' + pos.y + 'px,0)') +
    			(scale ? ' scale(' + scale + ')' : '');
    	}
    }
  2. L.SVG._update to modify viewBox attribute of SVG:
    container.setAttribute('viewBox', [0, 0, size.x, size.y].join(' '));

What do you think about this proposition. Its' works with Firefox 47.0.1 and IE11 in LTR and RTL direction

@HarelM
Copy link

HarelM commented Apr 17, 2017

I think I have the same issue with leaflet 0.7.x.
As a workaround I set the dir=ltr for the map div and this seemed to solve the issue, although I haven't tested it thoroughly.

@HarelM
Copy link

HarelM commented Apr 21, 2017

Also happens on leaflet 1.0.x. The workaround described above is not so good as you'll need to set the direction of every tooltip/popover etc with the dir=rtl otherwise they will inherit it from the map element.

@perliedman
Copy link
Member

@HarelM is this only in IE11 (as the bug title indicates), or reproducible in other browsers as well?

@HarelM
Copy link

HarelM commented Apr 21, 2017

I'm not supporting IE, but it happens on Edge as well. Doesn't happen on Chrome or FF.

@perliedman perliedman changed the title Transform SVG in RTL direction with IE11 Transform SVG in RTL direction with IE11 and Edge Apr 21, 2017
@ludohe
Copy link
Author

ludohe commented May 3, 2017

And what do you think about my proposition of changes of SVG

@HarelM
Copy link

HarelM commented Jun 28, 2017

I so wished this was fixed as part of 1.10... :-( any updates on this though?
Congrats on the release!

@perliedman
Copy link
Member

No updates here.

It might seem like picking at details, but submitting an issue with suggested code changes is significantly more work for maintainers compared to a pull request: I have to manually figure out where these changes go, what version their made against, if they actually contain valid code, if the pass unit tests, and so on.

My suggestions if you want this to move ahead:

  • Submit a pull request with any changes required
  • Test this pull request thoroughly (different browsers, use test cases from the debug directory) - changes to a central class/method like DomUtil.setTransform is scary since it's easy to introduce breaking changes and/or performance regressions
  • add unit tests if applicable

Leaflet is perhaps more than ever a community effort, so just waiting around for someone to step up and solve issues important to you is most likely not a good strategy.

@HarelM
Copy link

HarelM commented Jun 28, 2017

@perliedman Thanks for the info, I appreciate your point of view.
I personally have no clue what is the root cause of the bug or how to fix it.
I can agree that @ludohe 's code changes might be better as a pull request rather than a snippet here, but he did ask for a feedback regarding his code and where he wants to fix it - I believe from someone who has experience with leaflet and might have a clue if this kind of fix is in the right place or not.
Your comment on the change itself is very important, but came 4 month after the original question, which might cause the original developer to abandon his eagerness to help fix this.
I believe that your response is correct but very late - letting the developer know that you would appreciate it as a pull request, or in general what you think is the right approach when contacting leaflet team would also contribute to the community effort :-)

@Falke-Design Falke-Design added the ie Internet Explorer label Feb 16, 2022
@jonkoops
Copy link
Collaborator

Closing this issue as we have decided to drop support for Internet Explorer in a future version of Leaflet (see #6136).

@jonkoops jonkoops closed this as not planned Won't fix, can't repro, duplicate, stale Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ie Internet Explorer
Projects
None yet
Development

No branches or pull requests

5 participants