Skip to content

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

Closed
wants to merge 7 commits into from

Conversation

inukshuk
Copy link

@inukshuk inukshuk commented Aug 3, 2011

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!

@kborchers
Copy link
Member

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.

@jzaefferer
Copy link
Member

@inukshuk could you check Firefox? Would be great to finally land this.

@inukshuk
Copy link
Author

inukshuk commented Sep 7, 2011

Absolutely. I'll take a look at it later tonight.

@inukshuk
Copy link
Author

inukshuk commented Sep 7, 2011

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.

@inukshuk
Copy link
Author

inukshuk commented Sep 7, 2011

Alright, I found the solution: this is jQuery 101 for me ;-)

@jzaefferer
Copy link
Member

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.

@kborchers
Copy link
Member

I think that should be fine. I'll admit I haven't tested it. I might be able to get to it tomorrow.

@inukshuk
Copy link
Author

inukshuk commented Sep 8, 2011

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:

  • position: multiple elements
  • position: positions
  • position: using

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.

@kborchers
Copy link
Member

Your change fixed the FF issue but causes the #5280 test to fail in IE9.

@gnarf
Copy link
Member

gnarf commented Sep 14, 2011

jQuery.support creates a body if one doesn't exist to do its tests... We should probably do something similar.

For reference here is the code in jQuery core: https://github.com/jquery/jquery/blob/1.6.4/src/support.js#L141-165

@inukshuk
Copy link
Author

@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).

@gnarf
Copy link
Member

gnarf commented Sep 14, 2011

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:

<head>
<script src="jquery.js"></script>
<script src="jquery-ui.js"></script>
<style>
    div { width: 100px; height: 100px; background-color: #bada55; }
</style>
</head>
<body>
    <div id="test"></div>
    <div id="test2"></div>
    <script>
        $("#test2").position({
            my: "top left",
            at: "bottom right",
            of: "#test"
        });
    </script>
</body>

At this point - the supports fractions test hasn't run yet...

@kborchers
Copy link
Member

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

@inukshuk
Copy link
Author

@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.

@inukshuk inukshuk closed this Sep 16, 2011
@kborchers
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants