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

Adding support for transform-origin to the 2d library #21

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

heygrady
Copy link

The transform-origin animation is a little superfluous but fully supporting transform-origin in IE8 and below is very important to do. Although it looks like a lot of code I took special care to make sure it minifies well. It adds about 2.5k when minified using uglifyjs.

I also corrected an error in your matrix library where some numbers were transposed because of a confusion between column-major and row-major order.

I'm hoping to retire my library because I prefer your approach but I wish you supported transform-origin more completely. Let me know what you think.

@louisremi
Copy link
Owner

Hi Grady,

I'll have a look at it after the holidays, don't hesitate to remember it to me if I take too much time.

Thanks for your work, and a merry Christmas,
Lr

rslt[2] = prev[0] * curr[2] + prev[2] * curr[3];
rslt[3] = prev[1] * curr[2] + prev[3] * curr[3];
rslt[4] = prev[0] * curr[4] + prev[2] * curr[5] + prev[4];
rslt[5] = prev[1] * curr[4] + prev[3] * curr[5] + prev[5];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide an example of transform that was failing with previous code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having issues with translate: skew(25deg) rotate(25deg) being transformed as if the functions were transposed. And the results were different than what native CSS rendered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's some fiddles to show that matrix issue. Fiddle doesn't work the best in IE8 so it will be easier to see locally but this should show that there are some differences.

Your code: http://jsfiddle.net/YP43N/8/
My patch: http://jsfiddle.net/YP43N/9/

@louisremi
Copy link
Owner

After looking at your code I'm convinced it could be packaged as a "plugin to my plugin" in a separate file.
When loaded, this file would overwrite the transform hook.

This would allow people to use conditional comments or conditional scripts loaders to only load the origin hook in IE678.
This solution will result in a slightly larger file size for IE678 because of overlapping code, but it seems better than sending an additional 2.5k to every browser.

I'm willing to patch my plugin to make your work easier (make some functions and variables public if required), to add your file to this repository and make you a contributor.

I hope you find this solution acceptable (and don't forget about my test case for the matrix product).
Happy new year!

@heygrady
Copy link
Author

heygrady commented Jan 4, 2012

I'm willing to do whatever works best for you. My primary goal is to make my code for supporting proper origin in IE available.

I will say that I disagree with your argument about splitting the file to avoid punishing compliant browsers with a greater filesize. IE8 is the most popular browser in the world -- it doesn't really matter that it's not the best browser. Any solution that would strive for wide adoption would need to support IE8 and below as transparently as possible. Transform Origin is integral to supporting the CSS 2d Transform specification. (Admittedly the origin animation code is a bit of an edge case that could be moved to a different location for people that absolutely need it.)

But I can see where you're coming from about file-size as a general concern and I know this is a big discussion coming from the jQuery community in general right now. On one hand we're talking about a 1KB gzipped difference, on the other hand we're talking about a 50% increase in code size.

Ultimately, I think that supporting transform origin is a worthwhile concern and is central to supporting transforms in IE. So, of course I think the extra code is worth it. I can look at reducing the code further in size but I doubt it will get too much smaller.

Let me know about next steps. I'll commit the change with single quotes in a minute.

@louisremi
Copy link
Owner

Hi again,
Sorry it took me so long to give a second look to this PR.

I just patched the script to fix the matrix transposition bug as you can see. Regarding the transform-origin patch, I'm still convinced that having to script that can be either conditionally loaded or concatenated is the best thing to do.
Do you think there would be any difficulty to do that?

Regards,
Lr

@heygrady
Copy link
Author

That's fine. A fork for modern browsers wold be MUCH smaller than the code necessary to support IE. That's why the 3d plugin is so much smaller. I still stink that origins should be supported in IE because it's possible to do and it's useful.

It's up to you if you want to integrate it into your library.

@louisremi
Copy link
Owner

Sure, I want to integrate it to the project as a third script. Do you want to "pluginize" this PR yourself or do you want me to handle it?

@heygrady
Copy link
Author

can you please. I'm not sure how you want to approach it.

@louisremi
Copy link
Owner

I'm just giving a closer look at your code and I just noticed $.fx.step.transformOrigin = ...
That is something I'd have to add to the "main" script, but I'm wondering if there are any practical use case to animate the transform origin. Have you ever needed such feature?

@heygrady
Copy link
Author

You could ditch animating the transform origine. It's not exactly useful; I only included it for completeness. It is definitely an edge case.

@louisremi
Copy link
Owner

Hey,
I've created a branch with an initial version of jquery.transformOrigin.oldIE.js built from your code. You can give it a look here: https://github.com/louisremi/jquery.transform.js/tree/transformOrigin (there's a test page in the test folder).
The script doesn't work yet and I haven't had enough time to investigate.

@bmcorser
Copy link

Interested to see if you guys were going to pick this up again. I use this script in a couple of places, some iE8 support would be pretty cool.

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

Successfully merging this pull request may close these issues.

3 participants