Bug: event.preventDefault is wrecking havoc with startTransition

[arackaf]

React version: 241c446

Steps To Reproduce

https://codesandbox.io/s/romantic-gates-2hrjp

1. Click "Show A" to render the lazy component.
2. Type into the input whose placeholder says it works. startTransition will call, inline loading will show for 2 seconds, then the Suspense boundary will trigger.
3. Click the "update" button - same as above - everything works
4. Now type into the input whose placeholder says it doesn't work. Note that the inline placeholder stays up for the entire time; the Suspense boundary never shows.
5. Now note that steps 2 or 3 will likely immediately show the suspense boundary. The bypassed rendering of the Suspense boundary from step 4 seems to be backed up, stuck somewhere, and is now unleashed.

Note that steps 2 and 3 are optional. No matter what, the preventDefault that's called in step 4 causes this incorrect behavior no matter whether steps 2 or 3 have been exercised.

Link to code example: see above

The current behavior

startTransition should suspend after the timeout even if event.preventDefault is called.

The expected behavior

it does not

[gaearon]

I don't understand what is happening here. This is odd.

[arackaf]

@gaearon - you don't understand what incorrect behavior my repro is showing, or are you saying you can't tell why / how React is broken? You added the "Bug" label and removed "Unconfirmed" so I assume the latter but let me know if you need me to clarify

[jddxf]

Here's what is going on:

The call of preventDefault keeps input values unchanged, so the pending discrete updates are not flushed at the end of the keydown event.

react/packages/legacy-events/ReactGenericBatching.js

Lines 41 to 45 in 241c446

	if (controlledComponentsHavePendingUpdates) {
	// If a controlled event was fired, we may need to restore the state of
	// the DOM node back to the controlled value. This is necessary when React
	// bails out of the update without touching the DOM.
	flushDiscreteUpdatesImpl();

Instead, they are flushed at the beginning of the keyup event. At that time, a timeout to commit the suspended root has already been scheduled. But it is cancelled since the flush needs to prepare a fresh stack to perform work.

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines 1243 to 1249 in 241c446

	if (timeoutHandle !== noTimeout) {
	// The root previous suspended and scheduled a timeout to commit a fallback
	// state. Now that we have additional work, cancel the timeout.
	root.timeoutHandle = noTimeout;
	// $FlowFixMe Complains noTimeout is not a TimeoutID, despite the check above
	cancelTimeout(timeoutHandle);
	}

And no timeout would be scheduled again for some reason. Thus the placeholder is never showed.

[gaearon]

@jddxf Would you be interested in writing a failing test for this? Recent bugfixes like #18411 and #18412 might provide you with some inspiration.

[jddxf]

@gaearon I'd love to if it's not too late to do it this weekend.

[gaearon]

Anytime! In general, all issues tagged with Concurrent Mode would benefit from failing tests. Even if we don't get to fixes soon.

[arackaf]

Woo hoo - thanks everyone!!! 👊👌💯