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

Test for getComputedStyle is broken in browsers that implement the current Web IDL specification #39

Closed
bzbarsky opened this issue Sep 16, 2014 · 8 comments

Comments

@bzbarsky
Copy link

This test in computed-style.js:

'getComputedStyle' in Window.prototype

is broken in browsers that implement the current Web IDL specification for Window (e.g. Firefox 32), because per that specification the property lives on the window object directly, not on the prototype.

The right test is:

'getComputedStyle' in window

which should work in both browsers that put the property on the window directly and ones that put it on the prototype.

This issue causes aight to try to use the IE-only .currentStyle in Firefox 32, which of course doesn't work very well.

@shawnbot
Copy link
Owner

I believe that this is addressed in the v2 refactor (#38), which incorporates more rigorously tested polyfills, including window.getComputedStyle. Thanks for reporting. I'll keep this open until I've confirmed that it's fixed in v2.

@shawnbot
Copy link
Owner

Paging @WebReflection: I think this is your issue, since ie8 shims getComputedStyle via window.Window.prototype.

@WebReflection
Copy link

why is ie8 even offered to Firefox when the only right way to include it would be

<!--[if IE 8]><script src="ie8.js"></script><![endif]-->

or via CDN

<!--[if IE 8]><script
  src="//cdnjs.cloudflare.com/ajax/libs/ie8/0.2.2/ie8.js"
></script><![endif]-->

ie8 aim is to fix IE8, not Firefox … it is not there to feature detect anything else that is not IE8 … I am not sure once getComputedStyle gets fixed the rest would work.

@WebReflection
Copy link

OK, just to be clear, this has nothing to do with ie8, this is the first line of this file:
https://github.com/shawnbot/aight/blob/master/js/computed-style.js

and I think it should be !('getComputedStyle' in window) indeed

ie8 has nothing to do with this test but yes, it will patch for IE8 only 'getComputedStyle too.

@shawnbot
Copy link
Owner

Sorry, I should have been clear that this relates to #38. An IE conditional comment is the suggested usage, but I would prefer that aight not interfere with modern browsers even if it is included (for instance, so that users can test flags such as aight.browser.ie). I'm happy to just wrap the included ie8 code in an if statement if this complicates matters.

@shawnbot shawnbot added this to the v2 milestone Sep 22, 2014
@WebReflection
Copy link

ie8 first line is if(document.craeteEvent) return; so there's no way it can interfere with anything else and all browsers since ever, but IE < 9, has that in ;-)

I've shown you where is the problem and it's not in ie8 but I wanted to underline that you don't need to serve ie8.js to any browser that is not IE8 and conditional comments never interfered with anything but older IEs.

Last, you cannot test IE8 via any other browser different from real IE8 (tests fails in emulators, as examples, but pass as expected in real IE8) so you are free to aight.browser.ie but that won't produce any predictable or expected result in other browsers. Just as extra warning.

If there's anything else I can do please let me know, again I just wanted to clarify some possible misunderstanding.

Cheers

@shawnbot
Copy link
Owner

Sorry for the confusion, @WebReflection. This will be fixed in #38.

@shawnbot
Copy link
Owner

Fixed in #38!

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

No branches or pull requests

3 participants