Renderers can queue commit effects for initial mount #8646

bvaughn · 2016-12-27T21:08:36Z

Auto-focus was previously managed by ReactDOMFiberComponent.setInitialProperties (which gets called during the complete phase). This did not work because host components are not yet present in the DOM until the entire tree has been mounted (which happens during the commit phase).

This PR resolves that by making a few changes:

The HostConfig finalizeInitialChildren method now returns a boolean. Renderers should return true to indicate that custom effects should be processed during commit (once host components have been mounted). eg ReactDOMFiber uses this to queue auto-focus.
A new HostConfig method, commitMount, has been added for performing this type of work. (See this discussion thread for why a new method was added instead of repurposing commitUpdate.) Existing renderers have been updated with noops.

Tests

I was able to easily reproduce the broken focus behavior in a browser and verify the fix. However it was a bit awkward to catch in a unit test because PhantomJS does not require DOM elements to be mounted in order to track their focused state. I was able to add a test that caught the broken behavior and verifies the fix, but it's kind of a hack.

IE8 compatibility note

Note that this PR also drops use of the focusNode helper method since it was in place for IE8 and IE8 support was officially dropped in January 2016.

aweary · 2016-12-27T21:12:27Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -371,6 +379,8 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
    // In the second pass we'll perform all life-cycles and ref callbacks.
    // Life-cycles happen as a separate pass so that all placements, updates,
    // and deletions in the entire tree have already been invoked.
+    // This pass also manages focus since components must be in the DOM first,
+    // And this does not happen untilt he entire tree is mounted.


Small typo!

untilt he

edvinerikson · 2016-12-27T21:13:54Z

I am not 100% this works but shouldn't you be able to use prepareForCommit/resetAfterCommit to do this?

edvinerikson · 2016-12-27T21:21:36Z

IMO we should not have the reconciler managing focusing state. It should be up to the renderer to do that if we can't do that with currently exposed methods from the reconciler we should introduce new API methods such as shouldFocusHostComponent(type, props) and focusHostComponent(instance).

bvaughn · 2016-12-27T21:24:38Z

Hey @edvinerikson 😄

Yeah, I believe this could also be done in resetAfterCommit in terms of timing. That method signature would need to be updated to accept instance/props but I agree with you that it seems more appropriate to move focus logic into the HostConfig. Let me play around with that idea.

gaearon · 2016-12-27T21:24:47Z

src/renderers/shared/fiber/ReactFiberCompleteWork.js

+            case 'button':
+            case 'input':
+            case 'select':
+            case 'textarea':


These types are specific to DOM renderer so they shouldn’t be in the reconciler.

gaearon · 2016-12-27T21:24:57Z

src/renderers/shared/fiber/ReactFiberCompleteWork.js

+            case 'input':
+            case 'select':
+            case 'textarea':
+              if (newProps.autoFocus) {


As is this particular prop name.

Right 👍 Sorry. This wasn't quite ready for review yet. I probably shouldn't have opened a PR yet. I just like to track my thoughts-in-progress via PR. Once it's ready I'll assign a reviewer.

Yeah no worries. A convention I've been using is to tag PR with [WIP] and maybe mention in the description what isn't ready yet.

Good call! I've updated the description to be more clear. 😁

I thought everyone else was on holiday today!

I sort of am but having a lazy evening. 😛

gaearon · 2016-12-27T21:26:10Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+
+      // Auto-focus host components once they have been mounted.
+      if (nextEffect.effectTag & Focus) {
+        focusNode(nextEffect.stateNode);


Similarly this is a DOM-specific utility function and shouldn't get called in non-DOM environments.

bvaughn · 2016-12-27T22:20:32Z

Very helpful initial feedback by @edvinerikson. (Thanks!)

After looking at resetAfterCommit- I think it's better to add new HostConfig methods that determine if focus should be done. resetAfterCommit is called at the right time, but it's not called for all elements in the tree and I'm hesitant to add another tree-walking pass for it.

So I've updated this PR to add 2 new HostConfig methods:

shouldFocusHostComponent(type : string, props : Props) : boolean - Called during the complete phase, after host component creation.
focusHostComponent(domElement : Instance) : void - Called during the commit phase, after insertions.

bvaughn · 2016-12-27T22:30:44Z

I believe this PR is ready for review now. 😁

I don't want to assign it to @gaearon since you're on holiday, but if you are bored and want to take a look I'd value your feedback. 😉 Else maybe @acdlite can pick it up tomorrow?

I'm out for the evening to catch a movie with my parents. Will check back in a few hours!

edvinerikson · 2016-12-27T22:40:00Z

Another thought: What if we store these DOM nodes in the rootHostContainer when they are created / updated and use that in resetAfterCommit? Should be possible if we send the rootHostContainer to resetAfterCommit.

Something like: rootHostContainer._focusedElement.focus() in resetAfterCommit.

Or maybe this will cause memory leaks if we don't remove instances if work gets aborted.

acdlite · 2016-12-27T22:44:16Z

Don't have time to look at this closely until tomorrow, but it'd be best to solve this without a new effect type. @sebmarkbage's idea was to reuse the Update type.

bvaughn · 2016-12-27T22:56:44Z

This can hold until tomorrow @acdlite. I'm done for the night in Virginia anyway. 😄

Maybe we can chat when you get in tomorrow. I don't initially see how we can re-use the Update type without calling shouldFocusHostComponent twice (once in complete phase and once again in commit phase) which feels hacky, eg something like this:

I don't know what Sebastian may have had in mind about this. All I had to go off of was the TODO comment in ReactDOMFiberComponent which suggested a new effect type.

bvaughn · 2016-12-27T22:57:47Z

@edvinerikson Yes, that's what I was worried about- cleaning up after cases where we get interrupted before completion, etc. Seemed safer this way.

sophiebits · 2016-12-28T06:15:32Z

Even "focus" is a pretty specific concept that likely does not apply to all renderers. Having some way for components in the host tree to opt into a commit-time effect for initial mount makes sense to me.

acdlite · 2016-12-28T06:42:36Z

@bvaughn As Ben said, what we need is a "commit-time effect for initial mount" — like componentDidMount for host components.

I don't think adding another method to the host config is necessary. We already have finalizeInitialChildren. Have it return a boolean. If it returns true, schedule an Update effect.

acdlite

Remove Focus side-effect. The Update effect should be sufficient.
Instead of a new host config method, have finalizeInitialChildren return a boolean to indicate if a host component needs an Update effect on initial mount.

bvaughn · 2016-12-28T13:56:50Z

Even "focus" is a pretty specific concept that likely does not apply to all renderers.

Great point.

Thanks for the suggestions Andrew and Ben. I'll implement them sometime this morning and get back. 😄

bvaughn · 2016-12-28T14:56:00Z

I don't think adding another method to the host config is necessary.

Remove Focus side-effect. The Update effect should be sufficient.

Couple of snags with this:

prepareUpdate and commitUpdate are only used for updates that occur after the initial mount and are bypassed completely if there is no alternate yet. To use commitUpdate we would need to relax the parameter types to make oldProps nullable. We would also need a way to differentiate between the first commit/mount and subsequent ones (since effects like auto-focus should only apply after initial mount). We could use the null oldProps value to differentiate but overall this feels fragile. This change would also introduce some asymmetry between prepareUpdate and commitUpdate which seems undesirable.
The timing is wrong. commitUpdate is invoked before host insertions complete (aka before the full tree is mounted) which makes it unacceptable for effects like auto-focus since that requires an element to be in the DOM.

My thoughts are that we should add a new renderer method (eg commitInitialEffects or similar). We could still use the return value of finalizeInitialChildren and the Update side effect type for queueing up the work though.

We also need to wait to call this method until mounting has finished. I don't feel strongly about the best way to handle this, although I lean toward using commitAllLifeCycles for this (a) the timing is right and (b) it avoids another walk through the tree. If we do so, we might consider renaming this method to better reflect its expanded purpose.

I'd love to know your thoughts on the above. 😄

Update: I expect some discussion and changes will be required but I'm going to go ahead and push what I suggested above for discussion purposes (see 09229ef).

acdlite · 2016-12-28T18:42:01Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+      // aka when there is no current/alternate.
+      if (
+        !current &&
+        nextEffect.effectTag & Update


Need an additional check that the tag is HostComponent.

acdlite · 2016-12-28T18:44:10Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -285,12 +289,23 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
    }
  }

-  function commitAllLifeCycles() {
+  function commitAllLifeCyclesAndApplyInitialEffects() {


Let's keep the previous name. What you're calling an "initial effect" could also be used to describe componentDidMount. I think calling this a "lifecycle" is sufficient.

acdlite · 2016-12-28T18:44:29Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+        const props = nextEffect.memoizedProps;
+        const instance : I = nextEffect.stateNode;
+        const rootContainerInstance = getRootHostContainer();
+        commitInitialEffects(instance, type, props, rootContainerInstance, nextEffect);


How about commitInitialMount?

acdlite · 2016-12-28T18:46:40Z

To use commitUpdate we would need to relax the parameter types to make oldProps nullable

That's fine

We also need to wait to call this method until mounting has finished. I don't feel strongly about the best way to handle this, although I lean toward using commitAllLifeCycles for this (a) the timing is right and (b) it avoids another walk through the tree. If we do so, we might consider renaming this method to better reflect its expanded purpose.

Conceptually what we're adding is componentDidMount for host components. So commitAllLifeCycles is the correct place to put it. I don't think a rename is necessary.

acdlite · 2016-12-28T18:51:07Z

src/renderers/dom/fiber/ReactDOMFiber.js

+  ) : void {
+    if (shouldAutoFocusHostComponent(type, newProps)) {
+      (domElement : any).focus();
+    }


Could you explain why we need this extra method? Could we not unify this with commitUpdate? We can determine if it's the initial mount by checking if oldProps is null. That's how we detect the initial mount elsewhere.

In fact the code to handle this is already in master: https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L609 You deleted it in a previous commit but I think if you add it back, it will work now that the host component is scheduled to update on initial mount.

I attempted to explain my reasoning in my previous comment. Some of it is subjective.

That change would introduce asymmetry between prepareUpdate and commitUpdate. commitUpdate would be called for newly-mounted components as well as pre-existing ones but prepareUpdate would only be called for existing ones. (Technically we could call prepareUpdate during initial mount as well but it seems unnecessary.)

commitUpdate would be called during different times, depending on whether the component was initially mounting (in which case it would be called during commitAllLifeCycles) or whether it was an updated to a pre-existing component (in which case it would be called during commitWork).

Adding checks to handle possible null values for oldProps for all renderers just to support a specific on-mount use case for one renderer felt (subjectively) wrong.

Makes sense. Let's go ahead and call that method commitMount then. Initial is redundant.

In fact the code to handle this is already in master. You deleted it in a previous commit but I think if you add it back, it will work now that the host component is scheduled to update on initial mount.

The previous code was in setInitialProperties which is only called by finalizeInitialChildren which happens too early (during complete not commit).

If you are referring to moving it into updateDOMProperties, then correct me if I'm wrong, but doing that would cause us to call domElement.focus() more than just on initial mount- which is not the way autoFocus should work.

Yeah you're right, I misunderstood when setInitialProperties was called.

acdlite · 2016-12-28T18:55:38Z

Also since the original commits have essentially been reverted, would you mind either rebasing or opening up a new PR?

bvaughn · 2016-12-28T18:56:59Z

Also since the original commits have essentially been reverted, would you mind either rebasing or opening up a new PR?

There's a lot of useful discussion on this PR (so opening a new PR seems overkill). I'm leaving the commits in place to make it easier for anyone interested in doing an incremental review. I rebase and squash things at the end of a review, before merging. This is part of my normal workflow.

acdlite · 2016-12-28T19:08:05Z

Yeah I just mean before it's merged.

bvaughn · 2016-12-28T19:08:52Z

Yeah I just mean before it's merged.

Oh, gotcha. Yup! Squash before merge, for sure.

acdlite · 2016-12-28T19:17:20Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+      // aka when there is no current/alternate.
+      if (
+        !current &&
+        nextEffect.tag === HostComponent &&


Hmm how about we move this to commitLifeCycles? Avoids an extra check. https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberCommitWork.js#L434

acdlite · 2016-12-28T19:57:08Z

src/renderers/native/ReactNativeFiber.js

+    return false;
+  },
+
+  focusHostComponent(instance : Instance) : void {


Shoot. Thanks!

acdlite · 2016-12-28T19:59:39Z

Delete that one leftover method and this is good to merge. We can continue bikeshedding on the name and come back to it later.

The finalizeInitialChildren HostConfig method now utilizes a boolean return type. Renderers can return true to indicate that custom effects should be processed at commit-time once host components have been mounted. This type of work is marked using the existing Update flag. A new HostConfig method, commitMount, has been added as well for performing this type of work. This change set is in support of the autoFocus prop.

acdlite · 2016-12-28T20:05:10Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

+        // aka when there is no current/alternate.
+        if (
+          !current &&
+          finishedWork.effectTag & Update


Oh shoot, missed this. Don't need to check for Update effect because it was already checked by the time commitLifeCycles is called.

If that's the case, why does the ClassComponent case check for it:

react/src/renderers/shared/fiber/ReactFiberCommitWork.js

Line 406 in 9e461c7

if (finishedWork.effectTag & Update) {

Because it also gets called if there's a Callback effect, which host components don't have. I guess it's a bit weird either way, so I'm fine with keeping this as-is.

Ah, right. That makes sense. Ok!

bvaughn · 2016-12-28T20:16:55Z

Huge thanks to @acdlite and others for the above review. Learned a lot!

sebmarkbage · 2017-01-11T18:10:44Z

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -50,10 +50,11 @@ export type HostConfig<T, P, I, TI, C, CX> = {

  createInstance(type : T, props : P, rootContainerInstance : C, hostContext : CX, internalInstanceHandle : OpaqueNode) : I,
  appendInitialChild(parentInstance : I, child : I | TI) : void,
-  finalizeInitialChildren(parentInstance : I, type : T, props : P, rootContainerInstance : C) : void,
+  finalizeInitialChildren(parentInstance : I, type : T, props : P, rootContainerInstance : C) : boolean,


We could consider returning an constant enum number tag here. Like other tags. It's not less efficient as long as it gets inlined and the constants make it a bit more readable. It's also more extensible if we want to add more tags here.

True! What other sorts of work do you think finalizeInitialChildren might want to schedule? (I'd prefer to think of at least one other likely use case before adding a constant.)

sebmarkbage · 2017-01-11T18:33:43Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    case 'input':
+    case 'select':
+    case 'textarea':
+      return !!(props : any).autoFocus;


We should expand the Props type to include this as optional instead of relying on any here.

sebmarkbage · 2017-01-11T18:35:29Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    internalInstanceHandle : Object,
+  ) : void {
+    if (shouldAutoFocusHostComponent(type, newProps)) {
+      (domElement : any).focus();


You can cast this to something more specific that has the focus method on it.

Such as ((domElement : any) : HTMLInputElement). That way we know what it should be if we properly typed this.

You are no longer using the focusNode helper which means that this will start throwing in IE8 for hidden elements which is a breaking change. Do we really want that?

Aye, the PR summary noted that:

IE8 compatibility note

Note that this PR also drops use of the focusNode helper method since it was in place for IE8 and IE8 support was officially dropped in January 2016.

Seemed wrong to keep an abstraction around solely for compat with a browser version we no longer support.

sebmarkbage · 2017-01-11T18:38:44Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    rootContainerInstance : Container,
+    internalInstanceHandle : Object,
+  ) : void {
+    if (shouldAutoFocusHostComponent(type, newProps)) {


Is this extra shouldAutoFocusHostComponent really necessary? Won't this only be scheduled if the first one is true? At least after #8685.

This check is still necessary. commitMount will be called for host components with refs even if finalizeInitialChildren returns false. (The "preserves focus" test in ReactDOM-test covers this case indirectly.)

But #8685 added a special tag for refs so it shouldn't be necessary for it to call commitMount in that case anymore, right?

Hm, I think you're right, but there's this:

react/src/renderers/shared/fiber/ReactFiberCompleteWork.js

Lines 240 to 243 in 3bc5595

if (workInProgress.ref) {

// If there is a ref on a host node we need to schedule a callback

markUpdate(workInProgress);

}

I think this is actually not intentional. I'll follow up on it. 😄

Yeah that was an oversight on my part

Follow-up on #8779

Also removed an unnecessary conditional check and improved a flow cast. Relates originally to PRs #8646 and #8685

facebook-github-bot added the GH Review: review-needed label Dec 27, 2016

bvaughn force-pushed the focus-side-effect branch from c443eed to 4d1d0c4 Compare December 27, 2016 21:10

sebmarkbage mentioned this pull request Dec 27, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

aweary reviewed Dec 27, 2016

View reviewed changes

bvaughn force-pushed the focus-side-effect branch from 4d1d0c4 to 0c1595d Compare December 27, 2016 21:15

gaearon reviewed Dec 27, 2016

View reviewed changes

bvaughn changed the title ~~Focus side effect type added; auto-focus moved to commit phase~~ [WIP] Focus side effect type added; auto-focus moved to commit phase Dec 27, 2016

bvaughn force-pushed the focus-side-effect branch from 265f7e7 to b5ed656 Compare December 27, 2016 22:23

bvaughn changed the title ~~[WIP] Focus side effect type added; auto-focus moved to commit phase~~ Focus side effect type added; auto-focus moved to commit phase Dec 27, 2016

acdlite requested changes Dec 28, 2016

View reviewed changes

bvaughn force-pushed the focus-side-effect branch 2 times, most recently from d2dd297 to 1d23b4e Compare December 28, 2016 18:04

acdlite reviewed Dec 28, 2016

View reviewed changes

bvaughn changed the title ~~Focus side effect type added; auto-focus moved to commit phase~~ Renderers can schedule commit-time effects for initial mount Dec 28, 2016

acdlite reviewed Dec 28, 2016

View reviewed changes

bvaughn force-pushed the focus-side-effect branch 2 times, most recently from 5c95952 to 5f4e681 Compare December 28, 2016 19:31

bvaughn changed the title ~~Renderers can schedule commit-time effects for initial mount~~ Renderers can queue commit effects for initial mount Dec 28, 2016

acdlite reviewed Dec 28, 2016

View reviewed changes

acdlite approved these changes Dec 28, 2016

View reviewed changes

bvaughn force-pushed the focus-side-effect branch from 5f4e681 to a101313 Compare December 28, 2016 20:03

acdlite reviewed Dec 28, 2016

View reviewed changes

bvaughn merged commit df6ca0e into facebook:master Dec 28, 2016

bvaughn deleted the focus-side-effect branch December 28, 2016 20:16

sebmarkbage reviewed Jan 11, 2017

View reviewed changes

bvaughn pushed a commit to bvaughn/react that referenced this pull request Jan 13, 2017

FlowType tweaks from Sebastian's feedback RE facebook#8646

7096772

bvaughn mentioned this pull request Jan 13, 2017

autoFocus related tweaks #8779

Merged

bvaughn pushed a commit to bvaughn/react that referenced this pull request Jan 13, 2017

FlowType tweaks from Sebastian's feedback RE facebook#8646

5c48f68

bvaughn added a commit that referenced this pull request Jan 13, 2017

Replaced an Update effect in complete phase with a Ref effect

7817eb0

Also removed an unnecessary conditional check and improved a flow cast. Relates originally to PRs #8646 and #8685

	if (workInProgress.ref) {
	// If there is a ref on a host node we need to schedule a callback
	markUpdate(workInProgress);
	}

Renderers can queue commit effects for initial mount #8646

Renderers can queue commit effects for initial mount #8646

Conversation

bvaughn commented Dec 27, 2016 • edited Loading

Tests

IE8 compatibility note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edvinerikson commented Dec 27, 2016

edvinerikson commented Dec 27, 2016

bvaughn commented Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Dec 27, 2016

bvaughn commented Dec 27, 2016

edvinerikson commented Dec 27, 2016

acdlite commented Dec 27, 2016

bvaughn commented Dec 27, 2016 • edited Loading

bvaughn commented Dec 27, 2016

sophiebits commented Dec 28, 2016

acdlite commented Dec 28, 2016

acdlite left a comment

Choose a reason for hiding this comment

bvaughn commented Dec 28, 2016 • edited Loading

bvaughn commented Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Dec 28, 2016

bvaughn commented Dec 28, 2016 • edited Loading

acdlite commented Dec 28, 2016

bvaughn commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IE8 compatibility note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Dec 27, 2016 •

edited

Loading

bvaughn commented Dec 27, 2016 •

edited

Loading

bvaughn Dec 27, 2016 •

edited

Loading

bvaughn commented Dec 27, 2016 •

edited

Loading

bvaughn commented Dec 28, 2016 •

edited

Loading

bvaughn commented Dec 28, 2016 •

edited

Loading

bvaughn commented Dec 28, 2016 •

edited

Loading