Skip to content

Use offsetX and offsetY if available #2975

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

Merged
merged 2 commits into from
Dec 1, 2014

Conversation

fredj
Copy link
Member

@fredj fredj commented Nov 27, 2014

Use the mouse event offsetX and offsetY properties if available instead of computing the value in javascript. See http://www.w3.org/TR/cssom-view/#dom-mouseevent-pagex

Calling goog.style.getRelativePosition will cause a reflow (because getBoundingClientRect is called in this function)

@fredj fredj force-pushed the mouseevent-offset branch from 1d96037 to 2d41688 Compare November 27, 2014 09:17
@fredj
Copy link
Member Author

fredj commented Nov 27, 2014

And there's also less temporary goog.math.Coordinate instances created (3 per goog.style.getRelativePosition call)

@fredj
Copy link
Member Author

fredj commented Nov 27, 2014

Thanks for any review

@ahocevar
Copy link
Member

I have not done any testing (not on touch devices and not on IE), but from just looking at the code it looks good. If you can confirm that the relevant interactions and gestures still work on all target browsers and especially Windows Mobile devices, then please merge.

@@ -592,18 +592,20 @@ ol.Map.prototype.getEventCoordinate = function(event) {


/**
* Returns the map pixel position for a browser event.
* Returns the map pixel position for a browser event relative the viewport.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relative to the viewport.

@gberaudo
Copy link
Member

The code is local and looks reasonable.
If it works on popular browsers, I am in favour of merging it.

@fredj fredj force-pushed the mouseevent-offset branch from 2d41688 to e2ad38d Compare November 28, 2014 10:09
@elemoine
Copy link
Member

I don't get it. getEventPixel returns a pixel position that is relative to the map viewport (map.viewport_). My understanding of screenX and screenY is that they're relative to the screen. Am I missing something here?

@elemoine
Copy link
Member

Sorry we're talking about offsetX and offsetY here! Looking again…

@elemoine
Copy link
Member

And in the case where event.changedTouches is defined (touchend or touchcancel) why don't we use event.changedTouches[0].offsetX and event.changedTouches[1].offsetY instead of doing our own calculation based on clientX and clientY?

And also, is the else (where we use getRelativePosition) still useful/relevant? If so, in what condition?

@fredj
Copy link
Member Author

fredj commented Nov 29, 2014

And in the case where event.changedTouches is defined (touchend or touchcancel) why don't we > use event.changedTouches[0].offsetX and event.changedTouches[1].offsetY instead of doing our own calculation based on clientX and clientY?

Because offsetX and offsetY are not defined for touch events: http://www.w3.org/TR/touch-events/#touch-interface

And also, is the else (where we use getRelativePosition) still useful/relevant? If so, in what condition?

Yes; firefox (at least) does not provide offsetX and offsetY (see https://bugzilla.mozilla.org/show_bug.cgi?id=69787)

@elemoine
Copy link
Member

Thanks @fredj! This is very useful information. Information I'd add as comments in the code before merging! Please merge if you agree.

@fredj fredj force-pushed the mouseevent-offset branch from e2ad38d to 5c9c1c3 Compare December 1, 2014 08:13
@fredj fredj force-pushed the mouseevent-offset branch from 5c9c1c3 to fabf059 Compare December 1, 2014 09:56
fredj added a commit that referenced this pull request Dec 1, 2014
Use offsetX and offsetY if available
@fredj fredj merged commit 6790b86 into openlayers:master Dec 1, 2014
@fredj fredj deleted the mouseevent-offset branch December 1, 2014 10:23
@elemoine
Copy link
Member

elemoine commented Dec 1, 2014

Very nice. Thanks a lot for the comments you added to the code. This is very useful I think.

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.

4 participants