@samitouri / QOS-React-2 / commits / 198ed661c5

[Suspense] Use !important to hide Suspended nodes (#15861)

Suspended nodes are hidden using an inline `display: none` style. We do this instead of removing the nodes from the DOM so that their state is preserved when they are shown again. Inline styles have the greatest specificity, but they are superseded by `!important`. To prevent an external style from overriding React's, this commit changes the hidden style to `display: none !important`. MaYBE AnDREw sHOulD JusT LEArn Css I attempted to write a unit test using `getComputedStyle` but JSDOM doesn't respect `!important`. I think our existing tests are sufficient but if we were to decide we need something more robust, I would set up an e2e test.

Andrew Clark committed Jun 11, 2019 at 11:40 UTC 198ed661c51ab2319897eaccd368d21e5ec9a9a6
3 files changed +11 -9
packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js
+7 -5
@@ -90,9 +90,9 @@ describe('ReactDOMSuspensePlaceholder', () => {
90 );
91 }
92 ReactDOM.render(<App />, container);
93 - expect(divs[0].current.style.display).toEqual('none');
94 - expect(divs[1].current.style.display).toEqual('none');
95 - expect(divs[2].current.style.display).toEqual('none');
93 + expect(divs[0].current.style.display).toEqual('none !important');
94 + expect(divs[1].current.style.display).toEqual('none !important');
95 + expect(divs[2].current.style.display).toEqual('none !important');
96
97 await advanceTimers(500);
98
@@ -156,12 +156,14 @@ describe('ReactDOMSuspensePlaceholder', () => {
156 ReactDOM.render(<App />, container);
157 });
158 expect(container.innerHTML).toEqual(
159 - '<span style="display: none;">Sibling</span><span style="display: none;"></span>Loading...',
159 + '<span style="display: none !important;">Sibling</span><span style=' +
160 + '"display: none !important;"></span>Loading...',
161 );
162
163 act(() => setIsVisible(true));
164 expect(container.innerHTML).toEqual(
164 - '<span style="display: none;">Sibling</span><span style="display: none;"></span>Loading...',
165 + '<span style="display: none !important;">Sibling</span><span style=' +
166 + '"display: none !important;"></span>Loading...',
167 );
168
169 await advanceTimers(500);
packages/react-dom/src/client/ReactDOMHostConfig.js
+1 -1
@@ -589,7 +589,7 @@ export function hideInstance(instance: Instance): void {
589 // TODO: Does this work for all element types? What about MathML? Should we
590 // pass host context to this method?
591 instance = ((instance: any): HTMLElement);
592 - instance.style.display = 'none';
592 + instance.style.display = 'none !important';
593 }
594
595 export function hideTextInstance(textInstance: TextInstance): void {
packages/react-fresh/src/__tests__/ReactFresh-test.js
+3 -3
@@ -1359,7 +1359,7 @@ describe('ReactFresh', () => {
1359 const fallbackChild = container.childNodes[1];
1360 expect(primaryChild.textContent).toBe('Content 1');
1361 expect(primaryChild.style.color).toBe('green');
1362 - expect(primaryChild.style.display).toBe('none');
1362 + expect(primaryChild.style.display).toBe('none !important');
1363 expect(fallbackChild.textContent).toBe('Fallback 0');
1364 expect(fallbackChild.style.color).toBe('green');
1365 expect(fallbackChild.style.display).toBe('');
@@ -1373,7 +1373,7 @@ describe('ReactFresh', () => {
1373 expect(container.childNodes[1]).toBe(fallbackChild);
1374 expect(primaryChild.textContent).toBe('Content 1');
1375 expect(primaryChild.style.color).toBe('green');
1376 - expect(primaryChild.style.display).toBe('none');
1376 + expect(primaryChild.style.display).toBe('none !important');
1377 expect(fallbackChild.textContent).toBe('Fallback 1');
1378 expect(fallbackChild.style.color).toBe('green');
1379 expect(fallbackChild.style.display).toBe('');
@@ -1397,7 +1397,7 @@ describe('ReactFresh', () => {
1397 expect(container.childNodes[1]).toBe(fallbackChild);
1398 expect(primaryChild.textContent).toBe('Content 1');
1399 expect(primaryChild.style.color).toBe('red');
1400 - expect(primaryChild.style.display).toBe('none');
1400 + expect(primaryChild.style.display).toBe('none !important');
1401 expect(fallbackChild.textContent).toBe('Fallback 1');
1402 expect(fallbackChild.style.color).toBe('red');
1403 expect(fallbackChild.style.display).toBe('');