Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Fix for issue 149 #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix for issue 149 #203

wants to merge 1 commit into from

Conversation

gruber76
Copy link

currentStyle is Async in IE, and/or may be null if an element is created using createElement. Either way, PIE needs to make sure currentStyle is present before accessing its methods.

(see http://stackoverflow.com/questions/3722736/weird-ie-behavior-currentstyle-returns-null)

currentStyle is Async in IE, and/or may be null if an element is created using createElement.  Either way, PIE needs to make sure currentStyle is present before accessing its methods.

(see http://stackoverflow.com/questions/3722736/weird-ie-behavior-currentstyle-returns-null)
@lojjic
Copy link
Owner

lojjic commented Mar 19, 2012

Thanks for the patch. I'm not sure this would work though -- currentStyle.getAttribute is a function so I'd expect it to throw a "getAttribute is not a function" error if it's set to a plain object.

Do you have a testcase you've been able to test this fix against? That's been the only issue keeping me from fixing this myself, I don't have a way to reproduce the issue so I don't know if a fix works or not.

@gruber76
Copy link
Author

Yeah, that was a hack of a patch, I realized about ten seconds after I hit "request." Sorry. I'll get a test for you tomorrow, as I have it breaking 100% on a pie.js loop.

@lojjic
Copy link
Owner

lojjic commented Mar 19, 2012

Awesome, thanks. :)

Since we're discussing it I do have some additional thoughts about this issue...

  1. Since PIE really can't do much of anything without currentStyle being present, I'd prefer to short-circuit as soon as possible (and only once), for instance perhaps in Element#update().

  2. If we're preventing PIE from completing an update, then we need to make sure that we don't leave it hanging like that and have elements remain unstyled. If the process of IE's async currentStyle initialization doesn't itself trigger PIE to re-update once currentStyle becomes available, then we need to queue something up to try again after a delay.

@lojjic
Copy link
Owner

lojjic commented Mar 25, 2012

@gruber76 Any word on that testcase? I'd like to get a proper fix in asap.

@lojjic
Copy link
Owner

lojjic commented Mar 25, 2012

FYI here's my potential fix -- https://github.com/lojjic/PIE/tree/currentStyle-check

This checks for the presence of currentStyle at the start of each update, and if it's not present then it exits right away and sets up a timer to attempt the update again after a delay.

This is, of course, untested since I don't yet have a testcase. ;)

@gruber76
Copy link
Author

Ugh, sorry I got this error in an Angular project... it may be that the script is trying to run .attach before the element is actually IN the DOM, but after spending longer trying to pare it down than I really had time for I had to get back to paid work. Here's a snippet that creates the PIE error in IE9:
https://gist.github.com/2202107

@lojjic
Copy link
Owner

lojjic commented Mar 26, 2012

See my comment on your gist, there seems to be an error in the gist code and fixing that allows IE9 to render without error.

Unfortunately without a testcase I'll have to close this one as not reproducible. :/

@balu980
Copy link

balu980 commented Apr 17, 2012

You can reproduce this by setting -pie-poll to true for an element and remove the element frmo the DOM tree by calling innerHTML = "" in the parent or.

add another class to the element, using .className += " newClass" and the new class has a different background color

#210

@lojjic
Copy link
Owner

lojjic commented Apr 29, 2012

@balu980 I tried to create a testcase that does the steps you described, but was unable to trigger an error.

Are you able to create a testcase, or point me to a URL that triggers it?

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

Successfully merging this pull request may close these issues.

3 participants