anselmh / object-fit

Polyfill (mostly IE) for CSS object-fit property to fill-in/fit-in images into containers.
MIT License
996 stars 92 forks source link

Unresponsive script up to Firefox 35 #22

Closed ghost closed 9 years ago

ghost commented 9 years ago

It just hangs and doesn't apply the polyfill.

ghost commented 9 years ago

Seems to be in the first line of the _matchesSelector function.

anselmh commented 9 years ago

Hi, do you have a test case to see the error? And would you mind providing me these details:

Thanks, Anselm

ghost commented 9 years ago

@anselmh It's hard to pinpoint the cause, I'm only using it with a single image but there is quite a lot of css, it seems to be struggling when matching selectors:

Error: Script terminated by timeout at:
_matchesSelector@http://localhost:9292/assets/main.js:2486:7
window.getMatchedCSSRules@http://localhost:9292/assets/main.js:2551:9
objectFit.getMatchedStyle@http://localhost:9292/assets/main.js:2682:15
objectFit.processElement@http://localhost:9292/assets/main.js:2781:14
objectFit.process@http://localhost:9292/assets/main.js:2763:4
objectFit.init@http://localhost:9292/assets/main.js:2953:4
objectFit.polyfill@http://localhost:9292/assets/main.js:2966:5

This doesn't cause the error but it also doesn't work: http://codepen.io/thelucid/pen/gbRKrJ

IE9 cases the following error:

SCRIPT438: Object doesn't support property or method 'matchMedia' 
anselmh commented 9 years ago

Thanks, I’ll have a look into it later on and try to find a solution. The IE9 thing seems to be unrelated to this but I’ll have a look at this, too. Thanks for sharing the details.

anselmh commented 9 years ago

Okay, I’ve started to look into it. When using the test markup from my repository this error isn’t reproducible. No script terminated in Firefox v35.0. Even when I use the CodePen code, which I assume you’re using on your local development setup, it works as expected.

Weirdly, it doesn’t work on the Codepen itself. I assume, but really, that’s only an assumption as copepen lacks an internal console as far as I can see, this has to do with the cross site scripting restrictions that affect this polyfill (https://github.com/anselmh/object-fit#security--mixed-content-issues-and-3rd-party-css). I’m not sure if I’m able to figure out more here as I don’t know how Codepen assigns the CSS and JS to the code and generates the output. If done with iframes or similar tools it is the security issue.

The IE thing is something different and I’ll have a look into it soon.

fluke commented 9 years ago

I'm seeing the same issue on Firefox v35. Using the v0.3.1 will check the v0.3.7.

fluke commented 9 years ago

Still seeing the error.

anselmh commented 9 years ago

Hi @kartikluke can you please provide me with more details on how you use the polyfill and on which markup and in which environment (sth like codepen or a simple localhost non-iframe page)?

fluke commented 9 years ago

Using a localhost page. With images from S3. That may be the issue checking up on the Access header. Crashing on v34 of Firefox as well so version doesn't seem to work.

fluke commented 9 years ago
_matchesSelector/matcher@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:148:1
_matchesSelector@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:161:10
window.getMatchedCSSRules@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:201:9
objectFit.getMatchedStyle@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:332:15
objectFit.processElement@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:431:14
objectFit.process@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:413:4
objectFit.init@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:604:4
objectFit.polyfill@http://jobspire.com:3000/assets/polyfill.object-fit.js?body=1:617:5
polyfillObjectFit@http://jobspire.com:3000/assets/application.js?body=1:1043:1
jQuery.event.dispatch@http://jobspire.com:3000/assets/jquery.js?body=1:4665:15
jQuery.event.add/elemData.handle@http://jobspire.com:3000/assets/jquery.js?body=1:4334:6
triggerEvent@http://jobspire.com:3000/assets/turbolinks.js?body=1:334:12
changePage@http://jobspire.com:3000/assets/turbolinks.js?body=1:203:5
fetchReplacement/xhr.onload@http://jobspire.com:3000/assets/turbolinks.js?body=1:107:1
fluke commented 9 years ago

And I've set the CORS configuration as I'm doing S3 upload and other actions on the images.

anselmh commented 9 years ago

Thanks for the details, @kartikluke. I’ll try to re-create your test case. If in the meantime you find anything helpful or a solution, please let me know.

fluke commented 9 years ago

This is my CORS configuration in AWS:

<CORSRule>
        <AllowedOrigin>*</AllowedOrigin>
        <AllowedMethod>GET</AllowedMethod>
        <AllowedMethod>POST</AllowedMethod>
        <AllowedMethod>PUT</AllowedMethod>
        <MaxAgeSeconds>3000</MaxAgeSeconds>
        <AllowedHeader>*</AllowedHeader>
</CORSRule>

And a sample image from my bucket (https://jobspiredevelopment.s3.amazonaws.com/uploads/1424025710417-y35384s6gvo-6002121c817ebce8faa41cb504d0b6a1/3magine-office-toronto-1.jpg)

fluke commented 9 years ago

Checked it out. Tried making some test pages the unresponsive error doesn't come but it doesn't seem to work on Firefox v35.

fluke commented 9 years ago

Okay seems to be working now. Was a mistake in my code. Still have an issue with unresponsive script. I think I have an idea about what's causing it. I'll test it and get back.

fluke commented 9 years ago

SecurityError: The operation is insecure.

This is what's causing the error. There were multiple calls to the function causing it to simply crash. My Rails server isn't setting the headers for the css that's being sent. Thought it was related to the images, my bad. Will try and figure it out.

fluke commented 9 years ago

I'm unable to set the headers is it possible to go around this?

anselmh commented 9 years ago

You’re unable to set CSP? Well, in that case the script would need to skip all external URL CSS which I wanted to do but didn’t found the time in the past days. It should be relatively easy to do though. Perhaps you can come up with a PR for that or I might get a chance to do this over the next days.

fluke commented 9 years ago

Alright. That sounds like a great workaround. Could you outline how you'd go about it? I'll give it a shot if I find the time.

https://github.com/anselmh/object-fit/issues/19 How is this going? Will it fix the problem?

anselmh commented 9 years ago

For #19 see: https://github.com/anselmh/object-fit/issues/19#issuecomment-74855268. Concept: We’d need to extend the stylesheet fetcher in here to match against the current domain and only fetch them. Ideally, this test should be only applied by a setting (so that it’s optional for the dev if he wants cross-site stylesheets to be parsed or not).

fluke commented 9 years ago

Still getting unresponsive for Firefox. I think for a large number of styles the CSS Parser starts to fail.

anselmh commented 9 years ago

Yah, this is a limitation of the CSS parser. How many images do you want to have the polyfill applied on that page? I figured that about more than 5 cause issues. Or, how much CSS is parsed? All stats help tracking the issue down.

fluke commented 9 years ago

I included the CSS of my application in the pull request. It recreates the crash on just the one image in the tests. I need object fit to work on around 5 to 6 images. I'm working on shifting to using background-image and the background-size property together to recreate object-fit.

fluke commented 9 years ago

The CSS is close to 9000 lines. I've put the minified version in.

anselmh commented 9 years ago

Uh, that sounds bad (not your CSS but the fact that the polyfill is only applied to one img). 9000lines is pretty much CSS but anyways it should ideally not affect it. Sadly we’re searching the whole thing at the moment. This catches all edge cases but makes it super slow.

Interim solution: You can emulate object-fit also with SVG (if you don’t need to support IE9) with the preserveAspectRatio="". Or as you mentioned use a background-image.

At least your experience helps tracking down where the real issue lies in the polyfill at the moment.

fluke commented 9 years ago

SVG isn't really an option as these are uploaded images.

In the end this turned out to be an extension of https://github.com/anselmh/object-fit/issues/13 but the slowness is leading to timeout.

How can we go about making a faster parser?

anselmh commented 9 years ago

Okay. Well, we have a few options:

I’m not a JavaScript performance expert but currently with a few images in place we’re at under 10 frames per second which is way far away from the optimal 60fps. I do think there is potential to optimize the way reflows and repaints as well as the calculations are handled at the moment.

To be honest, I’m not too keen on doing this myself but I’m happy if anyone is willed to help me out on this.

fluke commented 9 years ago

Could maybe use https://github.com/postcss/postcss as the parser. Claims to be fast is there any way we could integrate it.

anselmh commented 9 years ago

Just a quick update to all of you: I released v0.4.1 today which is increasing performance by about 28%.

jbschrades commented 9 years ago

I was having the unresponsive script issue with FF v35. However, I just upgraded to v36 and I'm no longer experiencing the problem.

fluke commented 9 years ago

That's because v36 supports object-fit natively. You don't need the polyfill for it.

On Fri, 27 Feb 2015 at 22:46 Jeff Schroeder notifications@github.com wrote:

I was having the unresponsive script issue with FF v35. However, I just upgraded to v36 and I'm no longer experiencing the problem.

— Reply to this email directly or view it on GitHub https://github.com/anselmh/object-fit/issues/22#issuecomment-76432149.

jbschrades commented 9 years ago

Bonus! Thanks for the info @kartikluke.

fluke commented 9 years ago

Does leave cause for concern. There's a good chance there are people out there with v35 and below.

anselmh commented 9 years ago

Exactly. It also affects IE still (while IE does render significantly better than FF35-)

anselmh commented 9 years ago

So, added another performance commit which reduces the pain a bit. Still slow but can’t do much more for now. Therefore closing this particular issue now.