input autoFocus causes focus to be emitted before ref

[syranide]

https://jsfiddle.net/nnwd2c34/

Can reproduce on Chrome, but not in IE11.

[LeZuse]

Also came here to report this. Prevents a very usual usecase for focus/select:

<input
  autoFocus
  type="text"
  ref="valueInput"
  onFocus={this.onFocus}
/>

// this.onFocus:
this.refs.valueInput.select();
      ^ refs are not available yet

Using a workaround now:

// this.onFocus:
ReactDOM.findDOMNode(this).children[0]

[syranide]

@LeZuse You can just use event.target as a simpler workaround FYI.

[LeZuse]

@syranide ha, true! Thanks!

[gaearon]

Still happens on 16.

[oliviertassinari]

@syranide The event.target workaround works great. For everybody else, you might need event.currentTarget, be aware of the difference, and pick the one that fit your needs.

[gaearon]

Not sure what the right fix would be. Tagging as good first issue for somebody to investigate.

[RichieAHB]

The issue here seems to be that Ref effects are called after Update effects below (the latter being the effect to do the mounting). Although, if this is the issue, I'm not sure why it would work in IE11 🤔 ...

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

Lines 312 to 342 in 13c5e2b

	function commitAllLifeCycles() {
	while (nextEffect !== null) {
	const effectTag = nextEffect.effectTag;
	if (effectTag & (Update | Callback)) {
	recordEffect();
	const current = nextEffect.alternate;
	commitLifeCycles(current, nextEffect);
	}
	if (effectTag & Ref) {
	recordEffect();
	commitAttachRef(nextEffect);
	}
	if (effectTag & Err) {
	recordEffect();
	commitErrorHandling(nextEffect);
	}
	const next = nextEffect.nextEffect;
	// Ensure that we clean these up so that we don't accidentally keep them.
	// I'm not actually sure this matters because we can't reset firstEffect
	// and lastEffect since they're on every node, not just the effectful
	// ones. So we have to clean everything as we reuse nodes anyway.
	nextEffect.nextEffect = null;
	// Ensure that we reset the effectTag here so that we can rely on effect
	// tags to reason about the current life-cycle.
	nextEffect = next;
	}
	}

I'm assuming this is to ensure that all refs are guaranteed to be mounted, which seems to make sense. On the off-chance that this isn't a guarantee that React provides then we could just flip the order of effects processing (although this breaks two tests around refs being called at the correct time, suggesting this is a hard constraint). Otherwise, it seems autofocussing would need to be broken away from the mounting process to solve this issue, which seems like an odd thing to do for other reasons.

Edit:
To be clear: the update gets marked here if finalizeInitialChildren returns true (which it does only if the host component needs an autofocus).

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

Lines 505 to 518 in 8d336aa

	// Certain renderers require commit-time effects for initial mount.
	// (eg DOM renderer supports auto-focus for certain elements).
	// Make sure such renderers get scheduled for later work.
	if (
	finalizeInitialChildren(
	instance,
	type,
	newProps,
	rootContainerInstance,
	currentHostContext,
	)
	) {
	markUpdate(workInProgress);
	}

That then gets fired on commitMount in the reconciler, which again, makes sense.

[moe091]

Nobody else has picked up this issue? I'd love to give it a try if not

[RichieAHB]

I had a look at this but I feel like it would mean a relatively big change and could probably do with a bit of feedback from @gaearon before pushing on.

[moe091]

Yeah good point, it looks like this would require changing the order in which different effects are applied to elements when they are created/updated, and changing that would obviously have a lot of side-effects.

I'll leave this be and find something else to work on for now

[RichieAHB]

Exactly, unless I’ve misunderstood something. If you can find another way I’d be interested to hear!

…

On Sun, 4 Feb 2018 at 13:16, Portfolio-rails ***@***.***> wrote: Yeah good point, it looks like this would require changing the order in which different effects are applied to elements when they are created/updated, and changing that would obviously have a lot of side-effects. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7769 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABk124JjxK9pYO-wgsbz2iYTec8L-1NHks5tRa29gaJpZM4KA9JI> .

[eragon512]

hey guys, I'd like to take up this issue if its available

[RichieAHB]

Another broad idea to fix this (a few years on) is to swap the order of Ref and Update but delay hooks until the Ref effects have run. No idea how feasible this would be.

[eps1lon]

Encountered this issue when using useImperativeHandle. The issue is that refs can be set with useImperativeHandle after useLayoutEffects run. This can become an issue if we execute callbacks in useLayoutEffect that use the ref set by useImperativeHandle e.g.

https://codesandbox.io/s/dank-darkness-podtr

const Child = React.forwardRef(({ callback }, ref) => {
  React.useLayoutEffect(callback);
  React.useImperativeHandle(ref, () => () =>
    console.log("handled me imperatively")
  );

  return null;
});

function Parent() {
  const childRef = React.useRef(null);
  // crash unless order of effects in Child is swapped
  return <Child callback={() => childRef.current()} ref={childRef} />;
}

Tried @RichieAHB proposed fix which resulted in 2 test failures which are desired IMO:

diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js
index d3c5cbc1f..450f053e5 100644
--- a/packages/react-dom/src/__tests__/ReactComponent-test.js
+++ b/packages/react-dom/src/__tests__/ReactComponent-test.js
@@ -356,10 +356,10 @@ describe('ReactComponent', () => {
       'start mount',
       'inner 1 render',
       'inner 2 render',
-      'inner 1 componentDidMount',
       'ref 1 got instance 1',
-      'inner 2 componentDidMount',
+      'inner 1 componentDidMount',
       'ref 2 got instance 2',
+      'inner 2 componentDidMount',
       'outer componentDidMount',
       'start update',
       // Previous (equivalent) refs get cleared
@@ -368,10 +368,10 @@ describe('ReactComponent', () => {
       'inner 2 render',
       'ref 1 got null',
       'ref 2 got null',
-      'inner 1 componentDidUpdate',
       'ref 1 got instance 1',
-      'inner 2 componentDidUpdate',
+      'inner 1 componentDidUpdate',
       'ref 2 got instance 2',
+      'inner 2 componentDidUpdate',
       'outer componentDidUpdate',
       'start unmount',
       'outer componentWillUnmount',
diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
index fd286f1e6..355112e0b 100644
--- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
+++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
@@ -1046,12 +1046,12 @@ describe('ReactComponentLifeCycle', () => {
     expect(log).toEqual([
       'will mount',
       'render',
-      'did mount',
       'ref',
+      'did mount',
 
       'render',
-      'did update',
       'ref',
+      'did update',
     ]);
   });

Basically: Refs are called before didUpdate/didMount which should make more sense. If the component already did mount and did update then I should already have the updated refs and not was is currently after-didUpdate.

The issue becomes more apparent with forwardRef. If I pass forwarded refs to host components then those are set before layout effects run. But if I manually handle them with useImperativeHandle and not carefully check that I place this at the top of my render function I get a slightly different behavior.

Is this something the core team would be interested in or should this be re-opened when a breaking change is scheduled?