-
Notifications
You must be signed in to change notification settings - Fork 158
Conversation
3877a21
to
6ee9b50
Compare
Can you change this so that no matter how many times the property is requested, |
6ee9b50
to
c40e16b
Compare
Updated. This still calculates both properties at the time they're requested, which might be different from their values at the time the event is triggered. Should only affect code that, for whatever reason, holds on to the object beyond the frame(?) where its originally handled. Not sure if that is an issue we need to worry about. |
Looks good to me. I still need to review the other PR that this builds on top of though. |
Sure, also
|
c40e16b
to
91421a4
Compare
if (inDict.offsetX === undefined) { | ||
Object.defineProperty(e, 'offsetX', { | ||
get: function() { | ||
return e.pageX - boundingClientRect().left; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
offsetX
is relative to the padding edge of the target per spec:
return the x-coordinate of the position where the event occurred relative to the origin of the padding edge of the target node, ignoring the transforms that apply to the element and its ancestors
However getBoundingClientRect()
returns the rect of border edge, so I think the calculation is incorrect here.
Additionally, "ignoring the transforms" part seems also unimplemented (and I think this is quite tricky to polyfill).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. Can you help fix the calculation to take the padding edge into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is return e.clientX - boundingClientRect().left - inDict.target.clientLeft
. However "ignoring the transforms" is still missing and I don't have a solution yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On an element with 10px padding, what value should offsetX
have when the pointer is above to the top left corner of the element? If my test is correct, currently it has 0, which seems correct to me. Same for the top right corner: On an element with 10px padding on all sides, 130px width, the top left corner has offsetX 149.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem occurs if an element has borders, not paddings.
91421a4
to
9d2a0f3
Compare
@duanyao thanks, I've added those subtractions for both properties. This results in a negative offsetX/Y for the border areas on the top and left. Is that intended? |
@jzaefferer I think so. |
if (inDict.offsetX === undefined) { | ||
Object.defineProperty(e, 'offsetX', { | ||
get: function() { | ||
return e.pageX - boundingClientRect().left - inDict.target.clientLeft; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think e.pageX
should be e.clientX
, or the result is incorrect when the page is scrolled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks again!
WIP: No idea how to write a unit test for this, so for now it only extends the simple sample. Tested with the sample on Chrome, Firefox and Safari (all OSX) and Android 5 (BrowserStack) and iOS 9.1 (iOS Simulator). Fixes jquery-archive#217
9d2a0f3
to
74fae8e
Compare
@jacobrossi @jdalton @RByers @mbrubeck Can one of you confirm that this is the correct implementation? |
Sorry I missed the pings. The basic approach seems right to me, but I see a few potential issues (really I wouldn't believe it was entirely accurate without some tests of the special cases):
|
What if we start with eagerly loading the values so they're 100% correct, then if we notice perf issues we can switch to lazy evaluation and document when this can cause issues? |
Maybe with offsetTop and offsetParent. |
@zcorpan Are you able to provide either an explanation of how we would properly calculate those values or a code sample showing how to calculate the values given an element? |
I tried playing around a bit at It seems to mostly work, though might have rounding differences. However I can't explain where the 8 for body and 1 for |
Hi everyone, what is the status on this one? |
This is what I usually do to polyfill those properties (including support for scaling transforms)
didn't try with rotation transforms though (I'm not even sure if the original offetX/Y take those into account) Though IMHO just exposing offsetX/offsetY if available natively would be an awesome start. Same with movementX, movementY. |
@jzaefferer is this good to go? I could merge, broken CI and all, but it conflicts ... |
at this point, lacking sufficient knowledge to fully evaluate/update this, closing as stale |
Builds on top of #232, so ignore the first three commits.
PointerEvent: Polyfill offsetX/Y as getter
WIP: No idea how to write a unit test for this, so for now it only extends the simple sample.
Tested with the sample on Chrome, Firefox and Safari (all OSX) and Android 5 (BrowserStack) and iOS 9.1 (iOS Simulator).
Fixes #217