Do not render blocks or fields if the workspace is hidden

[fsih]

Resolves

#929

Proposed Changes

Check if workspace is visible when rendering fields

[rachel-fenichel]

From past experience, shuffling around the timing of rendering passes usually breaks Blockly for edge and IE10/11.

I think there's a different root cause, involving our show function sometimes being too lazy, and I'd rather solve that one.

[fsih]

I'm not sure what you mean by show being too lazy. Isn't the issue that show is being called too often, and it's caching a width that doesn't exist yet because the workspace is hidden?

[rachel-fenichel]

@picklesrus

[picklesrus]

I don't think show is being called too often, but more than render is being called when the workspace is invisible. We could try and catch all the places that call render and check for visibility, but I had been thinking of this as show's problem. Lots of other things may have changed (window size, browser zoom level, etc.) while the workspace was hidden. #912 is essentially that problem too - we do not recalculate the position of the delete area when the workspace is set back to be visible. @rachel-fenichel just sent out a fix that might actually fix this too.

All that said, re-rendering everything on show is a heavier hammer that we might want to avoid if it caused performance problems. If the browser is smart for us and not spending a bunch of time rendering stuff while it is invisible, doing it when the workspace is shown again seems a bit easier?

[picklesrus]

Hmmm, but everything I said before doesn't make a ton of sense now that I see workspace.render is already being called in show... Do you know at what level the render pass stops when show is called? Looks like it should get called on each block - does it get called on each field? Is the text width somehow cached? I was under the impression the cache was clear at that point, but maybe not?

[fsih]

It's this line here:
https://github.com/LLK/scratch-blocks/blob/develop/core/field.js#L489

When render happens when the workspace is hidden, the field gets its width set to something that doesn't include text width. Then when the workspace is shown, the field already has a width, so getSize returns the old width.

[fsih]

@rachel-fenichel @picklesrus do you guys have advice on a better way to fix this bug?

[rachel-fenichel]

@fsih sorry, I haven't yet had time to fully understand this issue. I'm hoping to get to it later today, or on monday at the latest.

[picklesrus]

sorry for the delayed response @fsih The end of last week ended up 100% filled with meetings.

I think this fix is ok, but I worry that there are other ways the workspace will be invisible that we won't know about - I'm pretty sure we ran into one with the snowflake project we did. Let me dig a little into what the width is on that line you mentioned - I'd hoped it would have been 0, but it must not be.

[picklesrus]

Ah. One interesting bit - blockly actually sets with width back to 0 when fields are invisible:
https://github.com/google/blockly/blob/master/core/field.js#L307 It doesn't look like scratch-blocks does that in field's render. I know that isn't much different from what you've added, would adding that bit of code to scratch-blocks solve the problem? At least then it is in the same place?

[fsih]

No problem, thanks for helping look at this bug!

Adding the

  if (!this.visible_) {
    this.size_.width = 0;
    return;
  }

clause actually doesn't work, because field.visible_ is set to true. The field "visibility" has to do with whether the block is collapsed and not whether the workspace is visible. Should I switch it to check for workspace visibility there?

[picklesrus]

Ah, I see. That's unfortunate. I do think workspace visibility might be the right longer term way to do this since I'd rather refine the signal for when we need to clear the text size cache than avoid rendering (which is sprinkled all over) whenever we think there might be a problem. I'm not sure using the workspace visibility won't cause you more bugs right now though - this code is brittle :(
I know you have some play testing coming up though so don't hold off your merge and let me block you from getting the things back into a working state!

[fsih]

Just FYI, Ray suggests we circumvent this bug from the GUI side by clearing out the workspace instead of just hiding it when the tab isn't visible, so I'm looking at that alternative.

[fsih]

Just merged scratchfoundation/scratch-gui#459 where the GUI reloads the workspace when it is set to visible. So we can feel free to pursue a longer-term better solution here.

[paulkaplan]

So since this isn't blocking us anymore, @rachel-fenichel @picklesrus how do you want to handle this? Should we close the PR and reopen at a later date?

[thisandagain]

@paulkaplan @fsih @rachel-fenichel @picklesrus How would you folks like to resolve this? Should we close for now?

[paulkaplan]

Let's close this for now.