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

Properly calculate the y position of the element when parent is positioned relatively #1745

Merged
merged 1 commit into from
Jun 11, 2013
Merged

Properly calculate the y position of the element when parent is positioned relatively #1745

merged 1 commit into from
Jun 11, 2013

Conversation

jec006
Copy link

@jec006 jec006 commented Jun 7, 2013

An attempt to deal with #1744 - this adds in calculation for the top offset when the item is positioned relative. This fixes an issue with mouse events not getting the correct lat/lng when a parent of the map had a vertical offset.

@Camwyn
Copy link

Camwyn commented Jun 8, 2013

In trying to implement this on my site, I found to get the (default) pin to place properly, I had to comment out line 3276 (in leaflet-src.js) like so:
if (anchor) {
img.style.marginLeft = (-anchor.x) + 'px';
//img.style.marginTop = (-anchor.y) + 'px';
}

The latlng appeared to be correct, but the pin placement was off...

The above made the positioning better, but the default pin was still not placed right - went from too low (south) to too high (north). FWIW, this is in a WP admin interface within nested position:relative elements.

@mourner
Copy link
Member

mourner commented Jun 9, 2013

@jfirebaugh hey John, can you take a look at this issue? It's related to your earlier PR.

@Camwyn that's quite weird, and commenting the anchor is definitely not the right solution here... Can you set up a JSFiddle that shows the marker problem?

@jec006
Copy link
Author

jec006 commented Jun 10, 2013

I've made 2 jsfiddles to demonstrate the issue and then the pull request code fixing it (for my use case):

This one: http://jsfiddle.net/jec006/Q4Wpn/ shows the issue with the offset occurring using the current master leaflet-src.js

This second one: http://jsfiddle.net/jec006/UVqWM/ is using the code with the change from this pull request and the markers appear in the correct position.

@Camwyn perhaps you can fork the second one to demonstrate the issue you're still having?

@Camwyn
Copy link

Camwyn commented Jun 10, 2013

I did fork the second, but placing the entire wordpress admin page in jsfiddle seems a bit...awkward.

I'll try to replicate tonight to demo for you - I'll try to match position and style as close as possible.
In the end I managed to fix it by removing position:relative from one of the parent elements.

@mourner
Copy link
Member

mourner commented Jun 11, 2013

@Camwyn you don't need to replicate the style as close as possible, just try to create the most simple and minimal test case where the problem is reproducible.

@mourner
Copy link
Member

mourner commented Jun 11, 2013

I'll merge the pull, but lets see if we can find a better solution for this that replaces the whole looped code — it has grown to be too complex and potentially has performance implications, so perhaps we should investigate more (e.g. try applying the D3.js approach).

BTW I it's @scooterw that made the original fix, not @jfirebaugh. @scooterw could you take a look at this?

mourner added a commit that referenced this pull request Jun 11, 2013
Properly calculate the y position of the element when parent is positioned relatively
@mourner mourner merged commit 043fa18 into Leaflet:master Jun 11, 2013
lubberscorrado referenced this pull request Jun 11, 2013
calculate x position with postion:relative and width/maxWidth
@jec006 jec006 deleted the calculate-y-mouse branch June 11, 2013 14:56
@scooterw
Copy link

I see this is closed now. Sorry I didn't see this sooner. Do you still want me to take a look at it, or does this merged PR fix the issue satisfactorily? I apologize for breaking the scrollwheel functionality / mouse events. For some reason, I did not notice this when I was testing out the commit that broke it.

@mourner
Copy link
Member

mourner commented Jun 11, 2013

No worries :) Just wanted to check if the fix looks good to you, since you're familiar with this part of the code now.

@lubberscorrado
Copy link

Well, to be honest this fix still need a bit of adjusting. When i now point to the anchor of a marker and use mousewheel the marker position is not exactly positioned to the anchor anymore. The marker moves a bit away the more i use the zooming functionality. This was not the case before #1744. Before #1744 the postion was everytime exactly accurate.

@mourner
Copy link
Member

mourner commented Jun 12, 2013

@lubberscorrado please provide a JSFiddle every time you point out a bug or a regression.

@brunob
Copy link
Contributor

brunob commented Jul 3, 2013

Hi @mourner i'm also encountering this bug with Leaflet in SPIP backend. Here is a simple fiddle that show the offset generated :

http://jsfiddle.net/8CMjZ/4/

This bug is the last thing that stop me to put Leaflet 0.6.x in GIS plugin for SPIP, hope we'll manage to find a fix :)

@scooterw
Copy link

scooterw commented Jul 4, 2013

@brunob, @mourner - A couple of things seem to be going on here. Adding r.top to the top value does not appear to be necessary. Commenting out that line and zeroing out the margin on body, div, and h1 elements seems to put the point back in the proper place. It seems to add some combination of margin values if the parent is positioned relative. I would not expect everyone to zero their margins, so this will likely need to be accounted for in some way (if this is indeed part of the problem). I have not figured out a satisfactory solution to this, so if anyone has ideas, or thinks I may be barking up the wrong tree, please let me know.

@gpbl
Copy link

gpbl commented Jul 4, 2013

There's a similar issue by setting the padding:
http://jsfiddle.net/ZXDkF/3/
#1826 seems to solve the problem to me

mourner added a commit that referenced this pull request Jul 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants