Skip to content

Commit

Permalink
[Suspense] Use style.setProperty to set display
Browse files Browse the repository at this point in the history
Follow up to #15861.

Turns out you can't set `!important` using a normal property assignment.
You have to use `style.setProperty`.

Maybe Andrew *should* just learn CSS.

IE9 doesn't support `style.setProperty` so we'll fall back to setting
`display: none` without `important`, like we did before #15861 Our
advice for apps that need to support IE9 will be to avoid using
`!important`. Which seems like good advice in general, but IANACSSE.

Tested on FB and using our Suspense DOM fixture.
  • Loading branch information
acdlite committed Jun 14, 2019
1 parent e91dd70 commit 52240a6
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -83,25 +83,25 @@ describe('ReactDOMSuspensePlaceholder', () => {
<div ref={divs[1]}>
<AsyncText ms={500} text="B" />
</div>
<div style={{display: 'block'}} ref={divs[2]}>
<div style={{display: 'span'}} ref={divs[2]}>
<Text text="C" />
</div>
</Suspense>
);
}
ReactDOM.render(<App />, container);
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');
expect(window.getComputedStyle(divs[0].current).display).toEqual('none');
expect(window.getComputedStyle(divs[1].current).display).toEqual('none');
expect(window.getComputedStyle(divs[2].current).display).toEqual('none');

await advanceTimers(500);

Scheduler.flushAll();

expect(divs[0].current.style.display).toEqual('');
expect(divs[1].current.style.display).toEqual('');
expect(window.getComputedStyle(divs[0].current).display).toEqual('block');
expect(window.getComputedStyle(divs[1].current).display).toEqual('block');
// This div's display was set with a prop.
expect(divs[2].current.style.display).toEqual('block');
expect(window.getComputedStyle(divs[2].current).display).toEqual('span');
});

it('hides and unhides timed out text nodes', async () => {
Expand Down Expand Up @@ -156,14 +156,14 @@ describe('ReactDOMSuspensePlaceholder', () => {
ReactDOM.render(<App />, container);
});
expect(container.innerHTML).toEqual(
'<span style="display: none !important;">Sibling</span><span style=' +
'"display: none !important;"></span>Loading...',
'<span style="display: none;">Sibling</span><span style=' +
'"display: none;"></span>Loading...',
);

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

await advanceTimers(500);
Expand Down
7 changes: 6 additions & 1 deletion packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,12 @@ 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 !important';
const style = instance.style;
if (typeof style.setProperty === 'function') {
style.setProperty('display', 'none', 'important');
} else {
style.display = 'none';
}
}

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 !important');
expect(primaryChild.style.display).toBe('none');
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 !important');
expect(primaryChild.style.display).toBe('none');
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 !important');
expect(primaryChild.style.display).toBe('none');
expect(fallbackChild.textContent).toBe('Fallback 1');
expect(fallbackChild.style.color).toBe('red');
expect(fallbackChild.style.display).toBe('');
Expand Down

0 comments on commit 52240a6

Please sign in to comment.