From 69ed76d120fcdd02e7dbdeacc10d57310a9da307 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Fri, 1 Dec 2017 11:31:24 +0900 Subject: [PATCH 01/11] Fix autoFocus for hydration content when it is mismatched --- packages/react-dom/src/client/ReactDOM.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index f898bb673a418..8d8b071634cdc 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -700,11 +700,13 @@ const DOMRenderer = ReactFiberReconciler({ newProps: Props, internalInstanceHandle: Object, ): void { - ((domElement: any): - | HTMLButtonElement - | HTMLInputElement - | HTMLSelectElement - | HTMLTextAreaElement).focus(); + if (shouldAutoFocusHostComponent(type, newProps)) { + ((domElement: any): + | HTMLButtonElement + | HTMLInputElement + | HTMLSelectElement + | HTMLTextAreaElement).focus(); + } }, commitUpdate( From 3de8f10788e1f4f2c67eb9adfd870d58bc982983 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Sat, 2 Dec 2017 17:27:53 +0900 Subject: [PATCH 02/11] Add a test for mismatched content --- .../src/__tests__/ReactServerRendering-test.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index 9bb4c6129444e..b0c196767885d 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -372,6 +372,22 @@ describe('ReactDOMServer', () => { expect(element.firstChild.focus).not.toHaveBeenCalled(); }); + it('should not focus on either server or client with autofocus={false} even if the markup is mismatch', () => { + spyOnDev(console, 'error'); + + var element = document.createElement('div'); + element.innerHTML = ReactDOMServer.renderToString( + , + ); + expect(element.firstChild.autofocus).toBe(false); + + element.firstChild.focus = jest.fn(); + ReactDOM.hydrate(, element); + + expect(element.firstChild.focus).not.toHaveBeenCalled(); + expect(console.error.calls.count()).toBe(1); + }); + it('should throw with silly args', () => { expect( ReactDOMServer.renderToString.bind(ReactDOMServer, {x: 123}), From c04a922cce02498a60ff8a99ca6e24054c101d28 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Sat, 2 Dec 2017 21:05:51 +0900 Subject: [PATCH 03/11] Fix a test for production --- packages/react-dom/src/__tests__/ReactServerRendering-test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index b0c196767885d..b8861ff330c62 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -385,7 +385,9 @@ describe('ReactDOMServer', () => { ReactDOM.hydrate(, element); expect(element.firstChild.focus).not.toHaveBeenCalled(); - expect(console.error.calls.count()).toBe(1); + if (__DEV__) { + expect(console.error.calls.count()).toBe(1); + } }); it('should throw with silly args', () => { From c4f88f46f19cf1ef04df5e44d0255781aa2905c5 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Mon, 4 Dec 2017 08:29:58 +0900 Subject: [PATCH 04/11] Fix a spec description and verify console.error output --- .../react-dom/src/__tests__/ReactServerRendering-test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index b8861ff330c62..1ea8a39e59c6f 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -372,7 +372,7 @@ describe('ReactDOMServer', () => { expect(element.firstChild.focus).not.toHaveBeenCalled(); }); - it('should not focus on either server or client with autofocus={false} even if the markup is mismatch', () => { + it('should not focus on either server or client with autofocus={false} even if there is a markup mismatch', () => { spyOnDev(console, 'error'); var element = document.createElement('div'); @@ -387,6 +387,9 @@ describe('ReactDOMServer', () => { expect(element.firstChild.focus).not.toHaveBeenCalled(); if (__DEV__) { expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: Text content did not match. Server: "server" Client: "client"' + ); } }); From 716ef7076462bacc53a2be07d6967a46fc9314b1 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Tue, 5 Dec 2017 10:24:28 +0900 Subject: [PATCH 05/11] Run prettier --- packages/react-dom/src/__tests__/ReactServerRendering-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index 1ea8a39e59c6f..8c50ca2d4721c 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -388,7 +388,7 @@ describe('ReactDOMServer', () => { if (__DEV__) { expect(console.error.calls.count()).toBe(1); expect(console.error.calls.argsFor(0)[0]).toBe( - 'Warning: Text content did not match. Server: "server" Client: "client"' + 'Warning: Text content did not match. Server: "server" Client: "client"', ); } }); From 58edd228046bcafcbcd04a70cb5e78520b50a07e Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Tue, 5 Dec 2017 21:17:06 +0900 Subject: [PATCH 06/11] finalizeInitialChildren always returns `true` --- packages/react-dom/src/client/ReactDOM.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 8d8b071634cdc..d51be2eba6b23 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -625,7 +625,7 @@ const DOMRenderer = ReactFiberReconciler({ rootContainerInstance: Container, ): boolean { setInitialProperties(domElement, type, props, rootContainerInstance); - return shouldAutoFocusHostComponent(type, props); + return true; }, prepareUpdate( From fdc314fd85d812dc07d35048684fe7f940f9af07 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Tue, 5 Dec 2017 21:24:55 +0900 Subject: [PATCH 07/11] Revert "finalizeInitialChildren always returns `true`" This reverts commit 58edd228046bcafcbcd04a70cb5e78520b50a07e. --- packages/react-dom/src/client/ReactDOM.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index d51be2eba6b23..8d8b071634cdc 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -625,7 +625,7 @@ const DOMRenderer = ReactFiberReconciler({ rootContainerInstance: Container, ): boolean { setInitialProperties(domElement, type, props, rootContainerInstance); - return true; + return shouldAutoFocusHostComponent(type, props); }, prepareUpdate( From 4421877aabf750d0081ed45bcb3dfb48ca73db65 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Tue, 5 Dec 2017 23:41:14 +0900 Subject: [PATCH 08/11] Add a TODO comment --- packages/react-dom/src/client/ReactDOM.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 8d8b071634cdc..90e5ecaf37e12 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -700,6 +700,9 @@ const DOMRenderer = ReactFiberReconciler({ newProps: Props, internalInstanceHandle: Object, ): void { + // This check is required for hydration + // but this check run twice in a non hydration context + // TODO: Ensure to do this check only once. if (shouldAutoFocusHostComponent(type, newProps)) { ((domElement: any): | HTMLButtonElement From e60822d9b9eedfc1a2f2f7e6952ff826e6c0b39e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 6 Dec 2017 15:16:30 +0000 Subject: [PATCH 09/11] Update ReactServerRendering-test.js --- packages/react-dom/src/__tests__/ReactServerRendering-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index 8c50ca2d4721c..393ad5066e0d8 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -357,6 +357,7 @@ describe('ReactDOMServer', () => { expect(element.firstChild.focus).not.toHaveBeenCalled(); }); + // Regression test for https://github.com/facebook/react/issues/11726 it('should not focus on either server or client with autofocus={false}', () => { var element = document.createElement('div'); element.innerHTML = ReactDOMServer.renderToString( From 2e965267718f8402c72fffea2cf901544786fc76 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 6 Dec 2017 15:16:51 +0000 Subject: [PATCH 10/11] Update ReactServerRendering-test.js --- packages/react-dom/src/__tests__/ReactServerRendering-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index 393ad5066e0d8..a1e883b456f90 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -357,7 +357,6 @@ describe('ReactDOMServer', () => { expect(element.firstChild.focus).not.toHaveBeenCalled(); }); - // Regression test for https://github.com/facebook/react/issues/11726 it('should not focus on either server or client with autofocus={false}', () => { var element = document.createElement('div'); element.innerHTML = ReactDOMServer.renderToString( @@ -373,6 +372,7 @@ describe('ReactDOMServer', () => { expect(element.firstChild.focus).not.toHaveBeenCalled(); }); + // Regression test for https://github.com/facebook/react/issues/11726 it('should not focus on either server or client with autofocus={false} even if there is a markup mismatch', () => { spyOnDev(console, 'error'); From b1fcd7c9c0804f7970b6baaf455ff0b10185bee9 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 6 Dec 2017 15:22:11 +0000 Subject: [PATCH 11/11] Rewrite the comment --- packages/react-dom/src/client/ReactDOM.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 90e5ecaf37e12..0ba729d4cfded 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -700,9 +700,12 @@ const DOMRenderer = ReactFiberReconciler({ newProps: Props, internalInstanceHandle: Object, ): void { - // This check is required for hydration - // but this check run twice in a non hydration context - // TODO: Ensure to do this check only once. + // Despite the naming that might imply otherwise, this method only + // fires if there is an `Update` effect scheduled during mounting. + // This happens if `finalizeInitialChildren` returns `true` (which it + // does to implement the `autoFocus` attribute on the client). But + // there are also other cases when this might happen (such as patching + // up text content during hydration mismatch). So we'll check this again. if (shouldAutoFocusHostComponent(type, newProps)) { ((domElement: any): | HTMLButtonElement