Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,7 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js
* should purge the DOM cache when removing nodes
* allow React.DOM factories to be called without warnings
* preserves focus
* calls focus() on autoFocus elements after they have been mounted to the DOM

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
* should handle className
Expand Down
6 changes: 5 additions & 1 deletion src/renderers/art/ReactARTFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,10 @@ const ARTRenderer = ReactFiberReconciler({
// Noop
},

commitMount(instance, type, newProps) {
// Noop
},

commitUpdate(instance, type, oldProps, newProps) {
instance._applyProps(instance, newProps, oldProps);
},
Expand Down Expand Up @@ -457,7 +461,7 @@ const ARTRenderer = ReactFiberReconciler({
},

finalizeInitialChildren(domElement, type, props) {
// Noop
return false;
},

insertBefore(parentInstance, child, beforeChild) {
Expand Down
29 changes: 28 additions & 1 deletion src/renderers/dom/fiber/ReactDOMFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,20 @@ function validateContainer(container) {
}
}

function shouldAutoFocusHostComponent(
type : string,
props : Props,
) : boolean {
switch (type) {
case 'button':
case 'input':
case 'select':
case 'textarea':
return !!(props : any).autoFocus;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return false;
}

var DOMRenderer = ReactFiberReconciler({

getRootHostContext(rootContainerInstance : Container) : HostContext {
Expand Down Expand Up @@ -173,8 +187,9 @@ var DOMRenderer = ReactFiberReconciler({
type : string,
props : Props,
rootContainerInstance : Container,
) : void {
) : boolean {
setInitialProperties(domElement, type, props, rootContainerInstance);
return shouldAutoFocusHostComponent(type, props);
},

prepareUpdate(
Expand All @@ -197,6 +212,18 @@ var DOMRenderer = ReactFiberReconciler({
return true;
},

commitMount(
domElement : Instance,
type : string,
newProps : Props,
rootContainerInstance : Container,
internalInstanceHandle : Object,
) : void {
if (shouldAutoFocusHostComponent(type, newProps)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that was an oversight on my part

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on #8779

(domElement : any).focus();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},

commitUpdate(
domElement : Instance,
type : string,
Expand Down
19 changes: 0 additions & 19 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ var ReactDOMFiberTextarea = require('ReactDOMFiberTextarea');
var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber');

var emptyFunction = require('emptyFunction');
var focusNode = require('focusNode');
var invariant = require('invariant');
var isEventSupported = require('isEventSupported');
var setInnerHTML = require('setInnerHTML');
Expand Down Expand Up @@ -605,36 +604,18 @@ var ReactDOMFiberComponent = {
isCustomComponentTag
);

// TODO: All these autoFocus won't work because the component is not in the
// DOM yet. We need a special effect to handle this.
switch (tag) {
case 'input':
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
inputValueTracking.trackNode((domElement : any));
ReactDOMFiberInput.postMountWrapper(domElement, rawProps);
if (props.autoFocus) {
focusNode(domElement);
}
break;
case 'textarea':
// TODO: Make sure we check if this is still unmounted or do any clean
// up necessary since we never stop tracking anymore.
inputValueTracking.trackNode((domElement : any));
ReactDOMFiberTextarea.postMountWrapper(domElement, rawProps);
if (props.autoFocus) {
focusNode(domElement);
}
break;
case 'select':
if (props.autoFocus) {
focusNode(domElement);
}
break;
case 'button':
if (props.autoFocus) {
focusNode(domElement);
}
break;
case 'option':
ReactDOMFiberOption.postMountWrapper(domElement, rawProps);
Expand Down
33 changes: 33 additions & 0 deletions src/renderers/dom/shared/__tests__/ReactDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,4 +235,37 @@ describe('ReactDOM', () => {
]);
document.body.removeChild(container);
});

it('calls focus() on autoFocus elements after they have been mounted to the DOM', () => {
const originalFocus = HTMLElement.prototype.focus;

try {
let focusedElement;
let inputFocusedAfterMount = false;

// This test needs to determine that focus is called after mount.
// Can't check document.activeElement because PhantomJS is too permissive;
// It doesn't require element to be in the DOM to be focused.
HTMLElement.prototype.focus = function() {
focusedElement = this;
inputFocusedAfterMount = !!this.parentNode;
};

const container = document.createElement('div');
document.body.appendChild(container);
ReactDOM.render(
<div>
<h1>Auto-focus Test</h1>
<input autoFocus={true}/>
<p>The above input should be focused after mount.</p>
</div>,
container,
);

expect(inputFocusedAfterMount).toBe(true);
expect(focusedElement.tagName).toBe('INPUT');
} finally {
HTMLElement.prototype.focus = originalFocus;
}
});
});
14 changes: 13 additions & 1 deletion src/renderers/native/ReactNativeFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,16 @@ const NativeRenderer = ReactFiberReconciler({
);
},

commitMount(
instance : Instance,
type : string,
newProps : Props,
rootContainerInstance : Object,
internalInstanceHandle : Object
) : void {
// Noop
},

commitUpdate(
instance : Instance,
type : string,
Expand Down Expand Up @@ -197,7 +207,7 @@ const NativeRenderer = ReactFiberReconciler({
type : string,
props : Props,
rootContainerInstance : Container,
) : void {
) : boolean {
// Map from child objects to native tags.
// Either way we need to pass a copy of the Array to prevent it from being frozen.
const nativeTags = parentInstance._children.map(
Expand All @@ -210,6 +220,8 @@ const NativeRenderer = ReactFiberReconciler({
parentInstance._nativeTag, // containerTag
nativeTags // reactTags
);

return false;
},

getRootHostContext() {
Expand Down
8 changes: 6 additions & 2 deletions src/renderers/noop/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,18 @@ var NoopRenderer = ReactFiberReconciler({
parentInstance.children.push(child);
},

finalizeInitialChildren(domElement : Instance, type : string, props : Props) : void {
// Noop
finalizeInitialChildren(domElement : Instance, type : string, props : Props) : boolean {
return false;
},

prepareUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : boolean {
return true;
},

commitMount(instance : Instance, type : string, newProps : Props) : void {
// Noop
},

commitUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : void {
instance.prop = newProps.prop;
},
Expand Down
16 changes: 16 additions & 0 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ module.exports = function<T, P, I, TI, C, CX>(
) {

const {
commitMount,
commitUpdate,
resetTextContent,
commitTextUpdate,
Expand Down Expand Up @@ -434,6 +435,21 @@ module.exports = function<T, P, I, TI, C, CX>(
case HostComponent: {
const instance : I = finishedWork.stateNode;
attachRef(current, finishedWork, instance);

// Renderers may schedule work to be done after host components are mounted
// (eg DOM renderer may schedule auto-focus for inputs and form controls).
// These effects should only be committed when components are first mounted,
// aka when there is no current/alternate.
if (
!current &&
finishedWork.effectTag & Update
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (finishedWork.effectTag & Update) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. That makes sense. Ok!

) {
const type = finishedWork.type;
const props = finishedWork.memoizedProps;
const rootContainerInstance = getRootHostContainer();
commitMount(instance, type, props, rootContainerInstance, finishedWork);
}

return;
}
case HostText: {
Expand Down
9 changes: 8 additions & 1 deletion src/renderers/shared/fiber/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,15 @@ module.exports = function<T, P, I, TI, C, CX>(
currentHostContext,
workInProgress
);

appendAllChildren(instance, workInProgress);
finalizeInitialChildren(instance, type, newProps, rootContainerInstance);

// 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)) {
workInProgress.effectTag |= Update;
}

workInProgress.stateNode = instance;
if (workInProgress.ref) {
Expand Down
3 changes: 2 additions & 1 deletion src/renderers/shared/fiber/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


prepareUpdate(instance : I, type : T, oldProps : P, newProps : P, hostContext : CX) : boolean,
commitUpdate(instance : I, type : T, oldProps : P, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void,
commitMount(instance : I, type : T, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void,

shouldSetTextContent(props : P) : boolean,
resetTextContent(instance : I) : void,
Expand Down
1 change: 1 addition & 0 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ 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 triggers any renderer-specific initial effects.
nextEffect = firstEffect;
while (nextEffect) {
try {
Expand Down