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

attempt to address regressions from #1684, #1745 #1826

Merged
merged 3 commits into from
Jul 6, 2013

Conversation

scooterw
Copy link

@scooterw scooterw commented Jul 4, 2013

This pull request eliminates the need to call L.DomUtil.getViewportOffset(). The test suites run successfully, and I do not see any regressions in the major browsers.

I did not remove L.DomUtil.getViewportOffset() and its tests in this pull request. That is a decision better made by others.

This attempts to address regressions from #1684 and #1745 .

@mourner
Copy link
Member

mourner commented Jul 5, 2013

That looks great, exactly what I had in mind, that loops in getViewportOffset were crazy! Can you list the browsers/devieces you tested on? We need to make sure there are no regressions over the current approach if we want to include this in a minor version release like 0.6.3.

@jec006
Copy link

jec006 commented Jul 5, 2013

Thanks, this is awesome. I had just noticed a few issues with left right calculations and this seems to fix them for me.

@brunob
Copy link
Contributor

brunob commented Jul 5, 2013

In my case it doesn't fix the offset, now i've got an offset on south instead of north :p I'll post a fiddle as soon as their site will be available.

More infos about it, the offset is now equal to the scroll offset of the window.

Here is quick made exemple (just pasted the leaflet dist script with patch applied in the head) :

http://jsbin.com/etonoh/1/

If you click on the map a marker is added : at the right position if the window scroll is at top / at the wrong position if the window scroll is at bottom.

@jec006
Copy link

jec006 commented Jul 5, 2013

Ah, yah, I also have problems, but only when the page is scrolled. ( i had to resize the example above so it would scroll and then scroll down)

@jec006
Copy link

jec006 commented Jul 5, 2013

So changing:


x = e.pageX ? e.pageX : e.clientX + body.scrollLeft + docEl.scrollLeft,
y = e.pageY ? e.pageY : e.clientY + body.scrollTop + docEl.scrollTop,

to


x = e.pageX ? e.pageX - body.scrollLeft - docEl.scrollLeft: e.clientX,
y = e.pageY ? e.pageY - body.scrollTop - docEl.scrollTop: e.clientY,

Fixes the scroll issue for me. I've forked the jsbin above to show the fix here: http://jsbin.com/etonoh/2/ (Scroll to bottom, I just overrode the one function.)

I'll make a pull request to scooterw to add this fix

@brunob
Copy link
Contributor

brunob commented Jul 5, 2013

Nice one @jec006 this fix the bug with my layout :)

@scooterw
Copy link
Author

scooterw commented Jul 5, 2013

I will take a look. I do see where the change proposed by @jec006 fixes the issue when scroll is applied. Thanks! I will do some further testing, as I tend to overlook earlier IE versions that are still technically supported by Leaflet.

Fix issue with mouse position when page is scrolled
@scooterw
Copy link
Author

scooterw commented Jul 5, 2013

Tested (with scroll fix from @jec006) on:

OS X: Firefox, Chrome, Safari
Windows 7: Firefox, Chrome, IE 10, IE 9, IE 8, IE 7
iOS: Safari
Android (4.1.2): Chrome (27.0.1453.90)

I can also fire up a Linux machine and test there if necessary.

@brunob
Copy link
Contributor

brunob commented Jul 6, 2013

Ok on Ubuntu 13.04 with Firefox 22, Opera 12.16 & Chromium 28.0.1500.52

mourner added a commit that referenced this pull request Jul 6, 2013
@mourner mourner merged commit 8a0b9ec into Leaflet:master Jul 6, 2013
@mourner
Copy link
Member

mourner commented Jul 6, 2013

Awesome, thanks a lot!

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.

5 participants