fixes fractions handling in ui.position. #190

Closed
wants to merge 3 commits into from

4 participants

@inukshuk

As discussed in #6000 and in core #7730 it is better if ui position leaves the values returned by offset() untouched (i.e., no Math.round or parseInt -- if the browser reports fractions let the browser decide what to do with them).

@scottgonzalez
jQuery Foundation member

Does this fix the test that's commented out directly below the test you wrote? Also, is position currently broken with latest jQuery without this fix?

@inukshuk

I've added the markup for the consistency test back in. Basically, if you run the test now (i.e., with the patch and jQuery 1.5.1 that is inside the test environment), it will fail. However, if you replace parseInt with parseFloat in the jQuery file (those are the only two instances of parseInt that are left) that test passes, too. So basically, you want to get rid of the Math.round with the next core release that uses parseFloat instead of parseInt.

Right now, with the current core release and using Math.round, position works fine most of the time but can lead to off-by-one pixel errors and leads to errors if there are sub-pixel positions involved (for example, the test I added may not work using the current position and latest jQuery).

@scottgonzalez
jQuery Foundation member

Thanks, we'll probably wait till we update jQuery (which we should do soon) to land this.

@inukshuk

Sounds good. Meanwhile, I've uncommented the #5280 test as it is supposed to work with the updated jQuery.

@inukshuk

Any chance we can get this merged soon? If there are any issues left I'd gladly work on it; we've been patching ui for months and haven't noticed any problems --- it would be great if patching weren't necessary any longer ;-)

Thanks!

@scottgonzalez
jQuery Foundation member

We can land this in master, but I'm not sure what to do in 1-8-stable where we support 1.3.2 - current. Any ideas for a clean way to detect whether we should be rounding?

@inukshuk

It isn't pretty, but the only reasonable test I can think of is the version number: for jQuery below 1.6 we know that offset() will read CSS values using parseInt, therefore we could justify rounding the values in order to get consistent values on repeated calls. For 1.6 and later offset() should return fractions if they are set in CSS so it is OK to leave the values as they are. Does that sound reasonable?

@scottgonzalez
jQuery Foundation member

Can we create an element, set the offset to have fractions and then use jQuery to get the offset? Does that give us the information we need?

@inukshuk
@scottgonzalez
jQuery Foundation member

We'd normally put them in $.ui.support, but that would probably be a bit strange considering it's testing the behavior of jQuery itself. You can just store the result in a variable in the main closure of the position plugin and then do check against that. You can see how we do similar test here. Note that we'll only want to do this in 1-8-stable, not in master, since 1.9 will only support 1.6+. Because of that, we'll want a second pull request. We'll keep this one for master and then have a new one for 1-8-stable.

@kborchers
jQuery Foundation member

While working on better collision detection in position, I made these same changes to remove Math.round from position. The team asked that I take your unit tests and merge them into my pull request, which I have done.

@inukshuk

I've added a separate request here as you suggested. You'll find a jsfiddle link there, to quickly see the test in action using different jquery versions.

@jzaefferer jzaefferer closed this Nov 17, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment