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

fix #2434 - wrong calculation of offset #2437

Merged
merged 2 commits into from Aug 10, 2015

Conversation

@rrelmy
Copy link
Contributor

rrelmy commented Oct 19, 2012

Fixes the bug described in #2434

Offsets are rounded correctly now

@rrelmy
Copy link
Contributor Author

rrelmy commented Mar 3, 2014

Does something block this commit to be merged?

@ibolmo
Copy link
Member

ibolmo commented Mar 3, 2014

Are you using sub pixels when setting the left and top?

@ibolmo
Copy link
Member

ibolmo commented Mar 3, 2014

Update the PR to use String.toFloat instead.

@rrelmy
Copy link
Contributor Author

rrelmy commented Mar 3, 2014

The problem mostly occurs on slideshows if you use an element centered with auto margins within an element of odd width or using percents in width/padding.

The pull request has been updated.

@ibolmo
Copy link
Member

ibolmo commented Mar 3, 2014

This is going to be on hold, until we get a spec to check for this. You're welcome to add one yourself, or wait about a week to get a hold of the pending PR that reduces the complexity to test MooTools.

@rrelmy
Copy link
Contributor Author

rrelmy commented Jul 15, 2015

I have rebased the previous commit and written a test spec.

PhantomJS version 1.9.8 used by the karma runner is based on a webkit version released around 2011 before subpixel alignment was added to webkit.
EDIT: I broke the builds again, tested locally against phantomjs v2 …

Is a test spec needed for this change? If the browser itself does not support subpixel alignment it is just fine, but I can't think of a way to test this usefully without breaking the PhantomJS build …

If something is missing please let me know.

@kentaromiura
Copy link
Member

kentaromiura commented Jul 16, 2015

LGTM but travis failed

@SergioCrisostomo
Copy link
Member

SergioCrisostomo commented Jul 16, 2015

Travis fails on PhantomJS tests. We could add a bypass to phantom.js in the specs for this.

@rrelmy
Copy link
Contributor Author

rrelmy commented Jul 16, 2015

The test case has been modified to compare the result of getPosition with the output of getBoundingClientRect.
All tests pass now without testing for PhantomJS

SergioCrisostomo added a commit that referenced this pull request Aug 10, 2015
fix #2434 - wrong calculation of offset
@SergioCrisostomo SergioCrisostomo merged commit b1830c0 into mootools:master Aug 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ickata added a commit to ickata/mootools-core that referenced this pull request Sep 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.