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

PHeT simulations throwing uncaught JS errors #1002

Open
SteelWagstaff opened this issue May 26, 2019 · 10 comments
Open

PHeT simulations throwing uncaught JS errors #1002

SteelWagstaff opened this issue May 26, 2019 · 10 comments

Comments

@SteelWagstaff
Copy link

Users of our Pressbooks authoring tool have reported some issues with embedded PhET simulations when placed within a "collapsible subsection." You can see an example at https://phetcollapse.textopress.com/chapter/chapter-1/

If you view the JavaScript console in the example book you'll see JavaScript prints two fatal errors (one for each hidden PhET simulation).

Screen Shot 2019-05-26 at 10 32 07 AM

It appears that these PHeT simulations are attempting to run an invisible/hidden/{non-existent until expanded} property, which fails. Not sure if this is the right place to report this issue, so if it needs to be moved or placed elsewhere, please let us know.

@jessegreenberg
Copy link

Thanks for reporting the issue @SteelWagstaff. I see the errors in the link you provided, as well as in the following simple HTML:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>Hidden Sim test</title>
  </head>
  <body>

  <div hidden id="container">
    <iframe src="https://phet.colorado.edu/sims/html/masses-and-springs-basics/latest/masses-and-springs-basics_en.html" id="iframe" allowfullscreen scrolling="no" title="Interact with {{PHET_SIM_TITLE}}"></iframe>
  </div>

  <button id="testButton">Toggle Hidden</button>
  
  </body>

  <script type="text/javascript">
    const container = document.getElementById( 'container' );
    const button = document.getElementById( 'testButton' );
    button.addEventListener( 'click', ( event ) => {
      if ( container.hidden ) {
        container.removeAttribute( 'hidden' );
      }
      else {
        container.hidden = true;
      }
    } );
  </script>
</html> 

When assertions are enabled, we first hit

Assertion failed: sim should have a nonzero area

@samreid
Copy link
Member

samreid commented Jun 13, 2019

I'm able to reproduce the problem on the masses and springs version referenced above, but not in a freshly built Faraday's law.

@jessegreenberg
Copy link

Assigning to @ariel-phet and @oliver-phet, can you please review this issue?

@ariel-phet
Copy link

@SteelWagstaff when I try the provided link, everything seems to work? Did @connerbw make a fix, or do we need to investigate further?

Also does Pressbooks currently have a more formal relationship with our project? If not, could you give us the contact info of the correct person to discuss partnerships with?

@dac514
Copy link

dac514 commented Jun 17, 2019

Did @connerbw make a fix

We worked on a hack:

https://github.com/pressbooks/pressbooks-book/blob/8357b6ec5f43476e1757ff07c1cfbd981e5098cb/assets/src/scripts/collapse-sections.js#L107

When I try the provided link, everything seems to work? ... do we need to investigate further?

Did you look at the network console? I can still reproduce. The bug is still there.

image

@ariel-phet
Copy link

@jonathanolson is going to spend about an hour seeing if he can identify what is going on here.

@jonathanolson
Copy link
Contributor

The root cause is that when running in an invisible iframe, Chrome is showing window.innerWidth and window.innerHeight as both 0. We have some graceful fallbacks, but PhetButton's onResize is missing the fallbacks and hits a hard error.

This is easy enough to patch, however there's a more fundamental tricky issue to resolve when our iframe is hidden: our text bounds detection method fails, so our sim handles layout with all text acting as if it takes zero height:

Screen Shot 2019-06-18 at 1 08 38 PM

I've traced that back to the SVG-based method we use to get the height of a font, which uses getBBox(). It always returns a 0-size rectangle when the iframe we're running in is hidden (and workarounds like https://stackoverflow.com/questions/28282295/getbbox-of-svg-when-hidden of "put the SVG content somewhere not hidden" is unavailable if everything under our html element is hidden). We need correct values for this at sim startup, because (a) we cache the font heights for performance, and (b) many times we lay things out statically based on the text height.

We can definitely detect when we're in this situation, but I don't see any great options of what to do. It would be possible to compute font/text heights using another method, but the values would be slightly different, and so the sim would look somewhat "different" in layout depending on whether it loaded in a hidden iframe or not (less than ideal).

The best solution I can think of on our end would be to delay sim startup until we have a well-defined width/height (and the SVG text method works). This means the sim wouldn't load in the background when hidden, but would trigger loading when it was shown. On our end, we could detect the SVG issue and delay startup if that is the case (and listen for a resize event and recheck and startup then).

Alternatively, having the sims start up in a way where they are not marked as "hidden" is the best workaround for currently-published simulations that I'm aware of.

@ariel-phet, thoughts?

@jonathanolson jonathanolson removed their assignment Jun 18, 2019
@ariel-phet
Copy link

@jonathanolson what is wrong with delaying sim startup? That seems totally reasonable to me, you open the frame, and the sim "boots" as it were. Seems like acceptable behavior. If we delayed start-up, it sounds like that solves the various issues?

Should we also make a separate issue for the PhETButton Resize fallbacks?

@jonathanolson
Copy link
Contributor

Should we also make a separate issue for the PhETButton Resize fallbacks?

This actually looks like it was resolved in master since we branched masses-and-springs-basics.

If we delayed start-up, it sounds like that solves the various issues?

Yes, it seems like the best option to me. It will likely require some changes in chipper to our HTML and loading process (we can't use the main image/phet-io loading delay section, as code could already have executed buggily). This seems generally useful to me.

@ariel-phet should I proceed with changes to implement that?

@jonathanolson jonathanolson removed their assignment Jun 19, 2019
@ariel-phet
Copy link

@jonathanolson yes please proceed. Can you document progress in a separate chipper issue and tag this one?

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

7 participants