Delay require.js startup until conditions are satisfied #764

jonathanolson · 2019-06-27T23:46:51Z

In phetsims/tasks#1002, it was noticed that when the simulation is loaded in a hidden iframe, our SVG text bounds method does not work. Other minor things (like zero width/height for the window) also happen.

I'm going to add a way where we wait until the simulation is ready to load (checking window resizes for now, with setTimeout as a backup method (in case we for some reason don't get window resizes).

…and phetsims/tasks#1002

jonathanolson · 2019-06-27T23:56:49Z

Commit tagged above. @ariel-phet I'd highly recommend this to be code reviewed (@samreid perhaps?), and I'm going to create a QA issue for some more specific testing.

jonathanolson · 2019-06-27T23:58:13Z

Adapted HTML from @jessegreenberg's minimal reproducible example at phetsims/tasks#1002 (comment):

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

  <div hidden id="container">
    <iframe width="800" height="600" src="https://phet-dev.colorado.edu/html/charges-and-fields/1.1.0-dev.7/phet/charges-and-fields_en_phet.html" id="iframe" allowfullscreen scrolling="no" title="Some 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>

jonathanolson · 2019-06-28T00:02:56Z

QA issue tagged above.

Once things look good (QA/review done), this seems like a candidate for maintenance releasing (potentially with @chrisklus tagging along). @ariel-phet does that sound like a plan?

samreid · 2019-06-29T03:24:36Z

Is #764 (comment) expected to error out, or is dev.7 patched? It's not showing symptoms that I can see in Mac/Chrome. How many platforms does the "Can't GetBBox of SVG when hidden" affect?

UPDATE: Also, should the setTimeout be changed to setInterval+clearInterval, so it will check more than once?

jonathanolson · 2019-07-01T17:20:37Z

Is #764 (comment) expected to error out, or is dev.7 patched? It's not showing symptoms that I can see in Mac/Chrome.

I can reproduce the issues if I check out the 1.0 branch of masses-and-springs-basics, replace the URL in the HTML template to point to the non-assert require.js form (http://localhost/masses-and-springs-basics/masses-and-springs-basics_en.html?brand=phet for me), and patch Sim.js' resize() with width = width || 800; height = height || 600; before it fires the resizedEmitter.

Then the result of un-hiding the sim shows the buggy (unpatched) state:

This is happening in the latest Chrome on Mac (Chrome 75).

UPDATE: Also, should the setTimeout be changed to setInterval+clearInterval, so it will check more than once?

It looks like this could also be used, but wouldn't save much (the check to see if we should add a timeout would be replaced with one to clear the interval).

jonathanolson · 2019-07-01T17:21:37Z

Is #764 (comment) expected to error out, or is dev.7 patched?

I did patch things and then deploy dev.7, so that should be the fixed behavior.

ariel-phet · 2019-07-01T18:01:50Z

@jonathanolson yes, agreed should be code reviewed (unclear if that already happened, but yes please have @samreid hop in on that).

Also agreed it is good for a maintenance release.

samreid · 2019-07-01T22:17:50Z

I won't be able to get to this right away, so @ariel-phet asked me to reassign to him to find another reviewer.

UPDATE: I projected an 80% chance I could get to this by the end of Friday, so @ariel-phet asked me to go for it.

samreid · 2019-07-04T03:09:41Z

I followed the steps in #764 (comment) and additionally discovered that you have to wait some 5-10 seconds before pressing "Toggle Hidden" in order to trigger the problem case.

samreid · 2019-07-04T04:27:56Z

Review notes:

I tested that the proposed solution works nicely! Nice work!

Some comments:

(1) Once runRequireJS is called, can we be certain that the entire startup sequence from that point is synchronous? What if the parent div becomes hidden during the startup process? What if the sim launches, then the parent div becomes hidden, then the PhET sim creates a SCENERY/Text while the parent is hidden, then parent becomes non-hidden. Won't the new SCENERY/Text have incorrect bounds and layout? Should we prevent any simulation activity while bounds cannot be obtained? If so, how could that be done efficiently, without creating SVG nodes and throwing them away every requestAnimationFrame?

(2) Does chipper-startup.js run during requirejs mode? It appears that it doesn't. Do we want our build to deviate from requirejs mode in this way?

(3) requireBuild.js wrapPath requires documentation. It acts like a boolean but default to null and is passed in as a string. Should it have a different variable name too?

(4) I'm having trouble tracking down requirejs wrap option documentation, can you please add a reference to it?

(5) Is it important to use document.createTextNode( '' ) with element.lastChild.nodeValue = 'm'; instead of document.createTextNode( 'm' )

(6) It looks like there is a little interference between the setTimeout and the addEventListener( 'resize'. For instance, if attemptStart is initially false, then a timeout is scheduled for 1000ms later, then resize triggers at 500ms, then attemptStart is called later unnecessarily. I know at that point it becomes a no-op, but it seems asymmetrical to not have the events types cancel each other. Not sure how much more complicated this would make it though. Something like:

  // We can call this anytime to attempt to start things (if they haven't been started already).
  function attemptStart() {
    if ( !started && isReady() ) {
      started = true;
      timeoutStillRunning && clearTimeout();
      windowListenerStillRunning && removeWindowListener();

      phet.chipper.runRequireJS();
    }
  }

For instance, with this patch:

Index: templates/chipper-startup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- templates/chipper-startup.js	(revision 0e1cf7b7f51776bca20379e777bc36200b9b3757)
+++ templates/chipper-startup.js	(date 1562348254000)
@@ -62,7 +62,10 @@
 
   // We can call this anytime to attempt to start things (if they haven't been started already).
   function attemptStart() {
+    console.log( 'attempt start' );
     if ( !started && isReady() ) {
+
+      console.log( 'started' );
       started = true;
 
       phet.chipper.runRequireJS();
@@ -71,6 +74,7 @@
 
   // Attempt to start based on timeouts (in case other methods don't work). This will call attemptStart() at least once.
   ( function timeoutListener() {
+    console.log( 'timeoutListener' );
     attemptStart();
 
     if ( !started ) {
@@ -82,10 +86,12 @@
 
     // Attempt to start on window resizes
     window.addEventListener( 'resize', function resizeListener() {
+      console.log( 'resize' );
       attemptStart();
 
       if ( started ) {
         window.removeEventListener( 'resize', resizeListener );
+        console.log( 'resize.started' );
       }
     } );
   }

I saw this console.log trace:

timeoutListener
attempt start
timeoutListener
attempt start
timeoutListener
attempt start
timeoutListener
attempt start
timeoutListener
attempt start
resize
attempt start
started
resize.started
timeoutListener
attempt start

Notice that even after resize and started, there is still another attempt to start.

(7) The current implementation requires that chipper-startup.js be loaded last, right? Should that be documented as such? What code could be safely added after this point without disrupting things?

(8) Is there a way to get (almost all of) chipper-startup.js to run as a standard preload, referenced from chipper/build.json? That would help unify this new code with existing code, and get rid of the code like this:

  const startupJS = grunt.file.read( '../chipper/templates/chipper-startup.js' );
  const productionStartupJS = minify( startupJS, minifyOptions );
  const debugStartupJS = minify( startupJS, debugMinifyOptions );

Moving toward this strategy may help us reach the goal of running this new code in requirejs + built mode?

(9) I don't know if this is attainable with Promises, but once we drop support for IE, would it be possible to do the equivalent of

await isReady();
phet.chipper.runRequireJS()

(10) The signature of setAttribute is (string,string), perhaps element.setAttribute( 'font-size', 16 ); should be changed to have string 2nd arg?

(11) Here and in TextBounds.js, why does the container have a width, height of 2x2? Is it important? It should be documented.

(12) . What would have to happen in order to factor out the duplicated code between TextBounds.js and chipper-startup.js? Even if we can't easily factor out 100% of the duplicated code, could we factor out 80% of the code into a preload?

jonathanolson · 2019-07-06T00:53:20Z

Once runRequireJS is called, can we be certain that the entire startup sequence from that point is synchronous?

No, we might be waiting on images to load also.

What if the parent div becomes hidden during the startup process? What if the sim launches, then the parent div becomes hidden, then the PhET sim creates a SCENERY/Text while the parent is hidden, then parent becomes non-hidden. Won't the new SCENERY/Text have incorrect bounds and layout? Should we prevent any simulation activity while bounds cannot be obtained? If so, how could that be done efficiently, without creating SVG nodes and throwing them away every requestAnimationFrame?

I tested this, and a timeout of 20ms between displaying and hiding again was able to produce this problem. I don't see any good way of handling this effectively, but I think it's unlikely to be encountered in the wild. Probably the only good way of handling this would be to switch to a different method of acquiring text bounds, but it's highly likely there is another way that gives the same results (so we may see a shift in spacing for text). It's a can of worms that requires a ton of browser testing, but if this becomes more of a problem, it could be looked into.

(2) Does chipper-startup.js run during requirejs mode? It appears that it doesn't. Do we want our build to deviate from requirejs mode in this way?

I don't think we ever plan to give require.js versions out to be embedded, and it would require a different solution. I have no problem with things differing, do you have a different opinion?

(3) requireBuild.js wrapPath requires documentation. It acts like a boolean but default to null and is passed in as a string. Should it have a different variable name too?

I'm open to naming suggestions. I fixed the string replacement (so the string value is used) and documented it in the above commit.

(4) I'm having trouble tracking down requirejs wrap option documentation, can you please add a reference to it?

Added in a comment to the general section, but https://github.com/requirejs/r.js/blob/master/build/example.build.js documents all of the options we have at our disposal.

(5) Is it important to use document.createTextNode( '' ) with element.lastChild.nodeValue = 'm'; instead of document.createTextNode( 'm' )

That looks like it could potentially be cleaner. I'm a little hesitant to change things too much from the scenery implementation (since it would require more testing). What are your thoughts?

(6) It looks like there is a little interference between the setTimeout and the addEventListener( 'resize'. For instance, if attemptStart is initially false, then a timeout is scheduled for 1000ms later, then resize triggers at 500ms, then attemptStart is called later unnecessarily. I know at that point it becomes a no-op, but it seems asymmetrical to not have the events types cancel each other. Not sure how much more complicated this would make it though.

It seems like more effort to independently track the state of all listeners to be able to clear them. Is there an advantage?

(7) The current implementation requires that chipper-startup.js be loaded last, right? Should that be documented as such? What code could be safely added after this point without disrupting things?

Not necessarily, there could conceivably be things in the future that we would want to run after the require.js step (say, if we ever wanted to move analytics code there, although it's currently a preload because there is no concept of "postload"). Should we document all of the order dependencies for the script order? For instance, initializationScript, splashScript, mipmapsJavaScript, ...productionPreloads, productionStringsJS, productionJS, productionStartupJS has some implicit information that I know about (mipmaps could only be swapped with preloads, but otherwise I think order changes would cause problems). build.json also doesn't have documentation about preload order that might be good to have. Do you have suggestions for how to document this? (Should this be it's own issue?). I added some bits of documentation in the commit above.

(8) Is there a way to get (almost all of) chipper-startup.js to run as a standard preload, referenced from chipper/build.json? That would help unify this new code with existing code, and get rid of the code like this: <<code>> Moving toward this strategy may help us reach the goal of running this new code in requirejs + built mode?

There is definitely opportunity for simplification (I'm including some cleanup in the above commit). We independently minify the four of these in both production and debug ways: ...productionPreloads, productionStringsJS, productionJS, productionStartupJS. The strings JS (chipper-strings.js) is only for built mode, as we have a different way of loading strings in require.js, so I don't feel like there is a fully unified approach.

(9) I don't know if this is attainable with Promises, but once we drop support for IE, would it be possible to do the equivalent of

Yes, although async/await was added to browsers after Promises. That will be a nice day when that's possible!

(10) The signature of setAttribute is (string,string), perhaps element.setAttribute( 'font-size', 16 ); should be changed to have string 2nd arg?

The documentation says it converts non-strings to strings. Maybe this is similar to scenery's Text, where it converts them? This is potentially a change for a lot of scenery code (and maybe we should open a separate issue if changes are recommended).

(11) Here and in TextBounds.js, why does the container have a width, height of 2x2? Is it important? It should be documented.

IIRC, non-empty and it seems to work? Not sure, and I should have documented it at the time.

(12) . What would have to happen in order to factor out the duplicated code between TextBounds.js and chipper-startup.js? Even if we can't easily factor out 100% of the duplicated code, could we factor out 80% of the code into a preload?

This seems like a big maintainability issue to factor it out. They are used in very different places, and have different use cases. I prefer mentioning the other usages, and not attempting to factor things out.

samreid · 2019-07-12T13:04:02Z

I'll create separate issues for the ideas that need further discussion.

samreid · 2019-07-12T15:27:05Z

I created issues for follow-up discussion or work. The proposed solution in master is an improvement over the previous status quo and seems stable enough to take simulations to RC. In my opinion, the blocks-publication label can be removed from this issue or this issue can be closed. None of the follow-up issues seem to be inherently blocking, unless we are concerned that addressing them over time will create complication for maintenance releases. Reassigning to @jonathanolson and @ariel-phet to see if this issue can be closed or changed to non-blocking.

jonathanolson · 2019-07-15T16:33:09Z

I've removed the blocks-publication label, but I want to keep this issue open to be the "master" issue for handling the maintenance release.

jonathanolson self-assigned this Jun 27, 2019

jonathanolson added a commit that referenced this issue Jun 27, 2019

Adding a delayed sim startup to wait for suitable conditions, see #764 …

e40da9a

…and phetsims/tasks#1002

jonathanolson added the status:blocks-publication label Jun 27, 2019

jonathanolson assigned ariel-phet Jun 27, 2019

jonathanolson mentioned this issue Jun 28, 2019

Spot checks for delayed chipper startup phetsims/qa#354

Closed

ariel-phet assigned jonathanolson and unassigned jonathanolson and ariel-phet Jul 1, 2019

jonathanolson assigned samreid and unassigned jonathanolson Jul 1, 2019

samreid assigned ariel-phet and samreid and unassigned samreid and ariel-phet Jul 1, 2019

samreid added a commit that referenced this issue Jul 5, 2019

Format with IDEA formatter, see #764

74925a8

samreid added a commit that referenced this issue Jul 5, 2019

Whitespace and author annotation, see #764

0e1cf7b

samreid assigned jonathanolson and unassigned samreid Jul 5, 2019

samreid added the status:ready-for-review label Jul 5, 2019

jonathanolson added a commit that referenced this issue Jul 6, 2019

Build improvements for preloads/minification and startup logic, see #764

ea74603

jonathanolson assigned samreid Jul 6, 2019

samreid assigned ariel-phet and unassigned samreid Jul 12, 2019

samreid mentioned this issue Jul 12, 2019

Plan for publication with fuse phetsims/circuit-construction-kit-common#500

Closed

9 tasks

jonathanolson removed the status:blocks-publication label Jul 15, 2019

ariel-phet removed their assignment Jul 17, 2019

ariel-phet added dev:enhancement and removed status:ready-for-review labels Jul 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay require.js startup until conditions are satisfied #764

Delay require.js startup until conditions are satisfied #764

jonathanolson commented Jun 27, 2019

jonathanolson commented Jun 27, 2019

jonathanolson commented Jun 27, 2019

jonathanolson commented Jun 28, 2019

samreid commented Jun 29, 2019 •

edited

Loading

jonathanolson commented Jul 1, 2019

jonathanolson commented Jul 1, 2019

ariel-phet commented Jul 1, 2019

samreid commented Jul 1, 2019 •

edited

Loading

samreid commented Jul 4, 2019

samreid commented Jul 4, 2019 •

edited

Loading

jonathanolson commented Jul 6, 2019 •

edited by samreid

Loading

samreid commented Jul 12, 2019

samreid commented Jul 12, 2019

jonathanolson commented Jul 15, 2019

Delay require.js startup until conditions are satisfied #764

Delay require.js startup until conditions are satisfied #764

Comments

jonathanolson commented Jun 27, 2019

jonathanolson commented Jun 27, 2019

jonathanolson commented Jun 27, 2019

jonathanolson commented Jun 28, 2019

samreid commented Jun 29, 2019 • edited Loading

jonathanolson commented Jul 1, 2019

jonathanolson commented Jul 1, 2019

ariel-phet commented Jul 1, 2019

samreid commented Jul 1, 2019 • edited Loading

samreid commented Jul 4, 2019

samreid commented Jul 4, 2019 • edited Loading

jonathanolson commented Jul 6, 2019 • edited by samreid Loading

samreid commented Jul 12, 2019

samreid commented Jul 12, 2019

jonathanolson commented Jul 15, 2019

samreid commented Jun 29, 2019 •

edited

Loading

samreid commented Jul 1, 2019 •

edited

Loading

samreid commented Jul 4, 2019 •

edited

Loading

jonathanolson commented Jul 6, 2019 •

edited by samreid

Loading