Fix text-editor-componen annoyingt bug

[Psychosynthesis]

Hi everyone!

didScrollDummyScrollbar() has a bug - in some cases (I'm not sure which ones) element is not defined, which causes an Uncaught TypeError.

The bug is described here: #1228

I couldn't pinpoint the exact reason, but it's been popping up too often for me lately and it's starting to get annoying.

I suspect it has something to do with projects being closed in the previous window, but again, it doesn't always happen, so I'm not sure.

There may be a better solution, but this should also prevent the error.

[Psychosynthesis]

@savetheclocktower please take a look

[savetheclocktower]

A couple suggestions. This will convert to an approval very quickly.

I do get this error sometimes as well. In general, I'm OK with this, because we haven't managed to narrow down reproduction steps. But the core issue is that these refs apparently don't exist sometimes, so I would expect similar errors to occur in other places whose root cause is trying to do something with this.refs.verticalScrollbar or this.refs.horizontalScrollbar when they don't exist.

Still, this is fine for now. But ultimately this will be fixed one of two ways:

• We decide that it's a bug for the scrollbar refs not to be present, and figure out how to guarantee they'll be present. (Maybe methods that use those refs are designed not to be called before the refs are available.)
• We decide that it's valid for the scrollbar refs not to be present at any given time — and that the bug is our assumption that those refs will be present. Then the fix would be to make the other usages as defensive as this one, always accounting for the possibility that the refs don't exist.

[savetheclocktower]

Suggested change

	this.refs.verticalScrollbar ? this.refs.verticalScrollbar.element.scrollTop : 0
	this.refs.verticalScrollbar?.element.scrollTop ?? 0

[Psychosynthesis]

I also like the syntax with ?. better, but I'm not deep enough into the build process and wasn't sure what version of ES was being used in the repo (this operator isn't used anywhere in this file), so I wrote it differently.

If ?. is acceptable, I'll certainly change it to it.

[savetheclocktower]

Yup. You can check process.versions in the console to find out what version of Node we're using; right now it's 14.16.0 (hopefully to be updated very soon), but that's new enough for optional chaining. (And for nullish coalescing.)

[savetheclocktower]

Suggested change

	this.refs.horizontalScrollbar ? this.refs.horizontalScrollbar.element.scrollLeft : 0
	this.refs.horizontalScrollbar?.element.scrollLeft ?? 0

[Psychosynthesis]

@savetheclocktower

* We decide that it's a bug for the scrollbar refs not to be present, and figure out how to guarantee they'll be present. (Maybe methods that use those refs are designed not to be called before the refs are available.)

* We decide that it's valid for the scrollbar refs not to be present at any given time — and that the bug is our assumption that those refs will be present. Then the fix would be to make the other usages as defensive as this one, always accounting for the possibility that the refs don't exist.

If I understand the logic correctly, refs.verticalScrollbar and refs.horizontalScrollbar are initialized only in renderDummyScrollbars. The rest of the logic implies that in some cases re-rendering is not performed (it is difficult for me to estimate under what conditions, the logic is tangled here and I do not know all the cases in which updateSyncBeforeMeasuringContent can be called), i.e. if the class is not recreated, and re-rendering was not performed, then verticalScrollbar and horizontalScrollbar` may not exist.

Therefore, if we go the first way, we either need to check the entire re-rendering process (I don't have that much free time now, unfortunately), or remove the check in renderDummyScrollbars and perform the creation of refs every time, but for some reason the shouldRenderDummyScrollbars flag was added?

Therefore, I suggest going the second way and checking the existence of verticalScrollbar and horizontalScrollbar everywhere before using.

[savetheclocktower]

Therefore, I suggest going the second way and checking the existence of verticalScrollbar and horizontalScrollbar everywhere before using.

You're probably right, but the good news is that we don't have to figure it out now. I'll take a closer look at some point.

[savetheclocktower]

What's strange about it is that didScrollDummyScrollbar is only called as an event handler for the scroll event on a dummy scrollbar. That suggests that, when this exception is thrown, either (a) these scrollbars did exist, but currently do not exist; (b) the scrollbars still exist, but etch has set this.refs.verticalScrollbar and/or this.refs.horizontalScrollbar to null before a dummy scrollbar was removed from the DOM.

Pulsar will skip rendering of the scrollbars if it decides that the scrollbars should be re-measured — and then only for a short while. (This is a common enough pattern; it suggests that the presence of the scrollbar can possibly make the container wider or taller than it naturally would be, and if you're unsure what the intrinsic height/width of the editor is, you'd want to remove the scrollbars for at least one tick so you can measure without their influence.)

In updateSyncBeforeMeasuringContent, if the scrollbars need to be re-measured, Pulsar will do a synchronous component update that omits the dummy scrollbars. It will then re-render the scrollbars on the next render — which happens asynchronously because of the scheduler, but in practice won't take too long to happen.

Explanation A seems unlikely. I'm not clear on how didScrollDummyScrollbar can be called at a time when the dummy scrollbar isn't on the page. A scroll event does not fire asynchronously in any circumstances, and etch itself doesn't do any magic in order to make it fire asynchronously.

So I think it's explanation B: when this bug happens, one of the dummy scrollbars still exists on the page — but the reference to the scrollbar has been prematurely nulled out. (But that suggests that a strange bug exists in etch, and I find that pretty unsatisfying as an explanation, since that'd be a silly bug. Still, I can't be sure.)

I do see that there are other places where updateSyncBeforeMeasuringContent is called, but none of them seem to involve a code path in which remeasureScrollbars is true — and that's the only scenario under which the scrollbars might temporarily be removed during the render cycle.