getMousePosition off due to borders and relative/absolute positioning

[chrelad@…]

I've managed to create a test case reproducing a bug I found while developing with OpenLayers. The test case shows that when the map div is contained by a node with both a border and position relative set, getMousePosition ends up with an incorrect offset.

[chrelad@…]

Removed extraneous table and containing div with no CSS properties

[chrelad@…]

You can get around this by removing either the border from the element that contains position relative, or removing the position relative from the element containing the border. If I figure out a way to completely resolve the issue, I'll post a patch.

[ahocevar]

The issue reported here turned out to be a serious rabbit hole:

• OpenLayers.Util.pagePosition returns incorrect positions if a map div with position: relative or position: absolute is nested in a stack of containers that have borders. The attached patch fixes this issue by replacing OpenLayers.Util.pagePosition with a function inspired by  http://code.google.com/p/doctype/wiki/ArticlePageOffset, which uses code from Yahoo's getXY method.
• The map div's border - set to 1px in all our examples except fullScreen.html - causes an offset of 1px. Thicker borders (like in attachment:mouseoffset.html ) increase the offset. This is independent of the positioning method of the map div. The patch fixes this by attaching the Events instance to the map's viewPortDiv instead of the div. This is perfectly sane because the map div just serves as a container.
• The page's scroll offset only works correctly due to adjustments in OpenLayers.Event.getMousePosition. By adding a new OpenLayers.Util.getViewportElement function, which determines the viewport element (which page position calculations are based on) the same way the new pagePosition method does, we now have a reliable base for scrollTop and scrollLeft. This function uses code taken from  http://code.google.com/p/doctype/wiki/ArticleClientViewportElement.

The patch adds a new manual test (tests/manual/page-position.html), which reveals all issues. Unit tests continue to pass in FF3.6, Safari 5 and IE8. The manual test displays fine in the same browsers, and also in Opera 10.6 and Chrome 7.

Thanks for any review.

[chrelad]

ahocevar, Seems to work fine with IE 6/8, Chromium 7.0.517.41, and Firefox 3.6.11. Thanks for your time on this :) Haven't checked this with IE 7 yet.

[erilem]

Hi Andreas. Nice patch! I'd like to make sure this change doesn't affect panning performance negatively. It looks like pagePosition isn't on the hot path but I'll do some profiles to verify that.

[tschaub]

Nice, I wanted to make the div -> viewportDiv fix some time ago.

While it doesn't have to be before pulling this in, please consider #2911 first. It's really silly how many times we check navigator.userAgent.

[ahocevar]

Updated after #2911

[ahocevar]

Patch updated to use internal BROWSER_NAME and IS_GECKO constants.

[erilem]

I can observe extra overhead with this patch applied, I'd like to take some time and see if we can optimize things a bit. I'm not sure this is easily doable, but I'd like to try.

[ahocevar]

@erilem: this surprises me: pagePosition should be more efficient in most cases, because it does not always have to bubble up the offsetParent hierachy. The only impact I could imagine would be the extra function call of getViewportElement in getMousePosition, but this should only be called once after scroll position or page size changes, and have no impact on drag performance. I'd be interested to hear where you experience extra overhead.

[erilem]

Replying to ahocevar:

@erilem: this surprises me: pagePosition should be more efficient in most cases, because it does not always have to bubble up the offsetParent hierachy. The only impact I could imagine would be the extra function call of getViewportElement in getMousePosition, but this should only be called once after scroll position or page size changes, and have no impact on drag performance. I'd be interested to hear where you experience extra overhead.

I've also been surprised, with the same reasoning as you. I'll look into it in more detail, but I'm busy with more urgent matters right now.

[erilem]

Ok, here are the actual profile results in FF3:

With patch
----------
getMousePosition 1646 calls, 9.16%, 154ms own time, 215ms time, 0.131ms avg
pagePosition 24 calls, 0.16%, 2.7ms own time, 2.9ms time, 0.124ms avg
getViewportElement 48 calls, 0.04%, 0.59ms own time, 0.59ms time, 0.012ms avg

Without patch
-------------
getMousePosition 1336 calls, 9.38%, 147ms own time, 175ms time, 0.132ms avg
pagePosition 21 calls, 0.35%, 5.52ms own time, 11.54ms time, 0.55ms avg

So the patch doesn't have a negative impact on performance, it even has a slight positive impact!

Yet, lots of time is spent in getMousePosition (w/ and w/o the patch), and it would be great to optimize that, I'm not sure it's possible though.

Thanks for the patch Andreas, it all looks good to me, please commit.

[erilem]

Yet, lots of time is spent in getMousePosition (w/ and w/o the patch), and it would be great to optimize that, I'm not sure it's possible though.

And for the record, lots of time is spent in getMousePosition because it's called many times when panning, and not because it's particularly inefficient, so optimizing wouldn't be easy.

[ahocevar]

r10871

[erilem]

And for the record, lots of time is spent in getMousePosition because it's called many times when panning, and not because it's particularly inefficient, so optimizing wouldn't be easy.

See #2880 for an optimization.

[fredj]

this patch broke the overview map control:  http://www.openlayers.org/dev/examples/overviewmap.html

[ahocevar]

use viewPortDiv for extentRectangle

[ahocevar]

Tests still pass. Thanks for any review.

[fredj]

Thanks Andreas, you patch fixes the overview map issue.

I just found there is another issue with the PanZoomBar control:  http://www.openlayers.org/dev/examples/controls.html

A click in the slider zone has no effect.

[ahocevar]

pagePosition has returned [0,0] when called with something different than a DOMElement. Now it returns undefined. The above patch removes an unneeded line with an incorrect argument from OpenLayers.Control.PanZoomBar.

[ahocevar]

Tests still pass. Thanks for any review.

[fredj]

works like a charm ! thanks Andreas

[ahocevar]

r10878

[fredj]

the 'pagePositionFailed' key needs to be removed from the i18n files

[fredj]

remove pagePositionFailed from Lang/*.js

[fredj]

please review attachment:"2247.i18n.patch"

[fredj]

no a regression

[fredj]

(In [12208]) remove 'pagePositionFailed' from Lang/*.js (closes #2247). Non-functional change.