Skip to content

Commit

Permalink
[Suspense] Use !important to hide Suspended nodes (facebook#15861)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored and rickhanlonii committed Jun 25, 2019
1 parent d87115e commit d224e8e
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ describe('ReactDOMSuspensePlaceholder', () => {
);
}
ReactDOM.render(<App />, container);
expect(divs[0].current.style.display).toEqual('none');
expect(divs[1].current.style.display).toEqual('none');
expect(divs[2].current.style.display).toEqual('none');
expect(divs[0].current.style.display).toEqual('none !important');
expect(divs[1].current.style.display).toEqual('none !important');
expect(divs[2].current.style.display).toEqual('none !important');

await advanceTimers(500);

Expand Down Expand Up @@ -156,12 +156,14 @@ describe('ReactDOMSuspensePlaceholder', () => {
ReactDOM.render(<App />, container);
});
expect(container.innerHTML).toEqual(
'<span style="display: none;">Sibling</span><span style="display: none;"></span>Loading...',
'<span style="display: none !important;">Sibling</span><span style=' +
'"display: none !important;"></span>Loading...',
);

act(() => setIsVisible(true));
expect(container.innerHTML).toEqual(
'<span style="display: none;">Sibling</span><span style="display: none;"></span>Loading...',
'<span style="display: none !important;">Sibling</span><span style=' +
'"display: none !important;"></span>Loading...',
);

await advanceTimers(500);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ export function hideInstance(instance: Instance): void {
// TODO: Does this work for all element types? What about MathML? Should we
// pass host context to this method?
instance = ((instance: any): HTMLElement);
instance.style.display = 'none';
instance.style.display = 'none !important';
}

export function hideTextInstance(textInstance: TextInstance): void {
Expand Down
6 changes: 3 additions & 3 deletions packages/react-fresh/src/__tests__/ReactFresh-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ describe('ReactFresh', () => {
const fallbackChild = container.childNodes[1];
expect(primaryChild.textContent).toBe('Content 1');
expect(primaryChild.style.color).toBe('green');
expect(primaryChild.style.display).toBe('none');
expect(primaryChild.style.display).toBe('none !important');
expect(fallbackChild.textContent).toBe('Fallback 0');
expect(fallbackChild.style.color).toBe('green');
expect(fallbackChild.style.display).toBe('');
Expand All @@ -1373,7 +1373,7 @@ describe('ReactFresh', () => {
expect(container.childNodes[1]).toBe(fallbackChild);
expect(primaryChild.textContent).toBe('Content 1');
expect(primaryChild.style.color).toBe('green');
expect(primaryChild.style.display).toBe('none');
expect(primaryChild.style.display).toBe('none !important');
expect(fallbackChild.textContent).toBe('Fallback 1');
expect(fallbackChild.style.color).toBe('green');
expect(fallbackChild.style.display).toBe('');
Expand All @@ -1397,7 +1397,7 @@ describe('ReactFresh', () => {
expect(container.childNodes[1]).toBe(fallbackChild);
expect(primaryChild.textContent).toBe('Content 1');
expect(primaryChild.style.color).toBe('red');
expect(primaryChild.style.display).toBe('none');
expect(primaryChild.style.display).toBe('none !important');
expect(fallbackChild.textContent).toBe('Fallback 1');
expect(fallbackChild.style.color).toBe('red');
expect(fallbackChild.style.display).toBe('');
Expand Down

0 comments on commit d224e8e

Please sign in to comment.