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

FrameManager: Fix IE/Edge frame rendering #33

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

ItsJonQ
Copy link
Contributor

@ItsJonQ ItsJonQ commented Nov 6, 2018

FrameManager: Fix IE/Edge frame rendering

screen recording 2018-11-06 at 05 24 pm

(GIF: Demo'ing Frame 1 being unmounted (destroy) and remounted in Edge. No errors in console. Stays are working)

This update fixes the iFrame SCRIPT70 error that occurs in IE/Edge
when an iFrame unmounts/remounts.

The solution was to wrap the offending code in a try/catch.

The code in question is designed for performance, specifically to check
for cached instances of Emotion, and serves that instance instead of
creating a brand new one.

The caching mechanism will work for all browsers excluding IE/Edge, which
unfortunately, will create new emotion instances every time as unmounted
frames cannot be properly references.

Tested on Edge, IE, and Chrome

This update fixes the iFrame `SCRIPT70` error that occurs in IE/Edge
when an iFrame unmounts/remounts.

The solution was to wrap the offending code in a `try/catch`.

The code in question is designed for performance, specifically to check
for cached instances of Emotion, and serves that instance instead of
creating a brand new one.

The caching mechanism will work for all browsers excluding IE/Edge, which
unfortunately, will create new emotion instances every time as unmounted
frames cannot be properly references.
@ItsJonQ ItsJonQ added the bug Something isn't working label Nov 6, 2018
@ItsJonQ ItsJonQ self-assigned this Nov 6, 2018
@coveralls
Copy link

Pull Request Test Coverage Report for Build 201

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 200: 0.0%
Covered Lines: 76
Relevant Lines: 76

💛 - Coveralls

Copy link

@tinkertrain tinkertrain left a comment

Choose a reason for hiding this comment

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

Verified the workaround with Q, acknowledge the drawback in IE/Edge he mentioned and find it is a small price for fixing the issue. Ship it!

@ItsJonQ ItsJonQ merged commit 2877e28 into master Nov 7, 2018
@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented Nov 7, 2018

@tinkertrain Thank you!!!

@ItsJonQ ItsJonQ deleted the ie-edge-frame-rendering branch November 8, 2018 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants