-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Positions: added conditional suppression of position rounding #419
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
Conversation
…presses rounding if it does
This appears to fix the fraction issue in IE, and Chrome's tests still pass but it breaks the fraction unit test in Firefox 6. It throws a body is null error but even when I tried a few things to fix that, the test still fails because it rounds when it shouldn't. |
@inukshuk could you check Firefox? Would be great to finally land this. |
Absolutely. I'll take a look at it later tonight. |
The problem is that FF6 (correctly) has no body defined, because we're loading the scripts in the header, so the test is executed before is defined. There should be a number of approaches to solve this issue – I'll take a look around the sources if a problem like this is already addressed somewhere. If not, perhaps the most elegant solution would be to lazily execute the fractions test (i.e., the first time its value is requested) instead of at load time. |
Alright, I found the solution: this is jQuery 101 for me ;-) |
I guess deferring to document-ready is an option. @kborchers, thoughts on that? Aparts from that: Why the commeted/duplicated testcode? Looks like an oversight. |
I think that should be fine. I'll admit I haven't tested it. I might be able to get to it tomorrow. |
Actually, the assignment of the test result didn't work that way; that's fixed now. However, I'm confused: I just ran the tests with older versions of jQuery to make sure everything works as expected and it seems as if a) the support.fraction test works in that the results get rounded only if you use old versions, but that b) many of the tests fail because they seem to expect fractional values. Now obviously this isn't directly related to the fractions test (if you take out the code in this commit the tests also fail with older versions), so I'm wondering if these test cases are actually meant to work on 1-8-stable with older jQuery versions? The tests in question are:
And 'consistent results' (the test I un-commented): this is a test expected to pass if support.fractions is true and fail otherwise. How would you want to deal with such tests? And do you know anything about the other tests and whether or not they are expected to pass with jQuery versions older than 1.6? @jzaefferer, shall I uncomment the #5280 test again? As regards the support.fractions test: that should work now. If @kborchers could confirm this, that'd be awesome. |
Your change fixed the FF issue but causes the #5280 test to fail in IE9. |
For reference here is the code in jQuery core: https://github.com/jquery/jquery/blob/1.6.4/src/support.js#L141-165 |
@kborchers, well spotted, thanks! After playing around with the IE9 debugger it seems that IE9 now supports fractions as CSS pixel values but rounds the numbers when placing them; therefore the test result was a false positive (both numbers were rounded up). Using slightly smaller numbers (below the 0.5 threshold) the test returns false and Math.round will be used, giving us consistent results. @gnarf37 thanks for pointing that out! Do you think it is worth it to create the body element for this test? If so, we should perhaps move the test to ui.support after all (in ui.support there is a similar test that relies on body and is not executed until document ready). |
Well - creating the body to run the test has the advantage of executing immediately as opposed to waiting for document ready. It is a minor difference, but imagine the following scenario:
At this point - the supports fractions test hasn't run yet... |
OK, so I pulled your solution into my own branch and made a few modifications. That will make it easier to pull since there have been a lot of changes to position and the unit tests since you first started working with this branch. Here is my pull #460 |
@kborchers, what changes would conflict with the original pull request? Good idea to test for upper and lower bounds, by the way – that should make the test more robust. Meanwhile, I'm closing this pull request. |
It wasn't so much a conflict as it will just be easier for the team to merge the changes if the branch is up to date. |
As discussed in Pull Request 190.
To see the test in action see this fiddle: returns true for jQuery 1.6+ and false for older versions.
I've added a variable 'support' to the postion closure, in case we ever need to add other tests. All position tests still pass for me.
Let me know if you want me to change anything!