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
[Suspense] Use !important to hide Suspended nodes
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 committed Jun 11, 2019
commit fcc3652a77a42847ee5101b3fd944db0b11df8e1
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 @@ -1357,7 +1357,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 @@ -1371,7 +1371,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 @@ -1395,7 +1395,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