Make IE 11 not complain about non-crucial style attribute hydration mismatch #13534

mgol · 2018-09-01T23:20:29Z

IE 11 parses & normalizes the style attribute as opposed to other browsers.
The normalization added in this commit normalizes spacing which resolves most
irrelevant style prop warnings, though a warning will still happen if the style
attribute contains invalid CSS declarations.

Fixes #11807

gaearon · 2018-09-01T23:47:53Z

scripts/rollup/results.json

    },
    {
      "filename": "react.production.min.js",
      "bundleType": "UMD_PROD",
      "packageName": "react",
-      "size": 7217,
-      "gzip": 3050
+      "size": 7289,


Please revert changes to this file

@gaearon Reverted. I don't know what generated changes to this file, I was just trying to follow https://reactjs.org/docs/how-to-contribute.html...

Sorry, it's not currently documented, but you can just always revert it.

Yeah, I already did in this PR. I'll remember for the future. :)

pull-bot · 2018-09-02T11:02:09Z

Details of bundled changes.

Comparing: 877f8bc...56b4800

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.2%	644.36 KB	644.98 KB	150.9 KB	151.13 KB	UMD_DEV
react-dom.development.js	+0.1%	+0.1%	639.59 KB	640.22 KB	149.5 KB	149.71 KB	NODE_DEV
ReactDOM-dev.js	+0.1%	+0.2%	661.93 KB	662.55 KB	151.83 KB	152.06 KB	FB_WWW_DEV

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

mgol · 2018-09-02T15:27:50Z

packages/react-dom/src/__tests__/ReactServerRenderingHydration.js

+    );
+  });
+
+  it('should not warn when the style property differs on whitespace only', () => {


I'm not sure how useful this test is; it'd need to be run in IE to make sure there is no regression, other browsers (and jsdom) would pass it even without this PR's changes.

Maybe it's useful as a way of ensuring my changes don't break jsdom?

Is there anything in jsdom you can monkeypatch for this specific test to reproduce IE behavior? I agree it's kind of useless like this.

mgol · 2018-09-03T16:02:51Z

The renaming in #13540 introduced a merge conflict with this PR so I rebased it. Tests still pass.

gaearon · 2018-09-03T16:07:41Z

packages/react-dom/src/__tests__/ReactServerRenderingHydration.js

+    expect(element.firstChild.style.textDecoration).toBe('none');
+    expect(element.firstChild.style.color).toBe('black');
+
+    spyOnDevAndProd(console, 'error');


You don't need this. We check that every test doesn't warn by default. So you can remove it.

gaearon · 2018-09-03T16:08:47Z

packages/react-dom/src/client/ReactDOMComponent.js

+    // attribute contains invalid CSS declarations but the majority of
+    // false warnings comes from spacing issues.
+    // See https://github.com/facebook/react/issues/11807
+    return markupNormalized.replace(/\s*([:;])\s*/, '$1').replace(/;$/, '');


How confident are you in this? What if ; is part of an image URL or something?

That's an excellent point. I'm afraid the only certain solution would be to do CSS parsing of the declarations included in the style attribute but the grammar for that is not that simple:
https://www.w3.org/TR/css-syntax-3/#tokenizing-and-parsing
especially around unquoted URLs so this might be too much.

That said, spaces are not that frequent in URLs, especially around semicolons and colons so this change should have very few false positives as opposed to having false negatives for every style attribute.

If I understand correctly, this only affects development. How about keeping this logic but only if document.documentMode is truthy? Regular browsers would work as they are now and developing in IE would be a little less unpleasant.

I guess it's fine since we normalize it on both sides. So at most you'll miss some legitimate warnings, but you shouldn't get warnings about valid code. Is that accurate?

Your suggestion to gate this by document.documentMode sounds good to me too.

So at most you'll miss some legitimate warnings, but you shouldn't get warnings about valid code. Is that accurate?

Yes, that's exactly it. And in non-IE browsers you'll still get all the warnings.

gaearon · 2018-09-03T16:40:33Z

A few more points:

The original post in False positive warning about style mismatch when hydrating server markup in IE11 #11807 had just one rule. It didn't end in semicolon, whereas the server version did. This normalization wouldn't be sufficient to handle this case, would it?
You can test this by simply hardcoding the markup you'd get with ReactDOMServer in IE11 instead of actual ReactDOMServer calls, and then hydrating from there.

mgol · 2018-09-03T16:47:00Z

The original post in #11807 had just one rule. It didn't end in semicolon, whereas the server version did. This normalization wouldn't be sufficient to handle this case, would it?

I'm stripping the last semicolon from the normalized value if one exists and since both the client & the server ones are normalized, this should be fine. I can add a test for that, using the idea from your second point.

I can work on that once we resolve the discussion around the ; whitespace trimming: #13534 (comment)

gaearon · 2018-09-03T16:52:26Z

Your suggestion sounds fine. I didn't realize we normalize both strings. Let's add a test with how I described it, yes.

mgol · 2018-09-03T22:22:08Z

I've applied all the discussed changes but I've noticed IE changes the order of CSS declarations, perhaps it sorts them? That makes it more difficult, I need to think what to do... :/

Would splitting on ;, sorting and joining back with ; be acceptable for comparison purposes (just in IE)? It would sometimes result in invalid CSS but it doesn't matter in this context and - again - the result would be sometimes no warning in IE where there should be one instead of what we have now.

gaearon · 2018-09-03T22:43:36Z

I’m worried about perf in DEV. For projects using inline styles sorting every one of them might be pretty slow.

Maybe let’s just disable the warning completely for style attribute in IE11?

…ismatch IE 11 parses & normalizes the style attribute as opposed to other browsers. It adds spaces and sorts the properties in some non-alphabetical order. Handling that would require sorting CSS properties in the client & server versions or applying `expectedStyle` to a temporary DOM node to read its `style` attribute normalized. Since it only affects IE, we're skipping style warnings in that browser completely in favor of doing all that work. Fixes facebook#11807

mgol · 2018-09-04T08:30:37Z

PR updated, IE will not warn for style mismatches anymore.

There's a way to make it warn without manual sorting. We could keep a DOM node around, set the computed client-side style attribute on it and read it back. We should get the same thing we're getting parsed out from the server response then.

This shouldn't slow the code down massively as that's what we're already doing on the server-generated nodes - we read the style attribute from them so IE has to parse & sort it. That said, it's just IE and some minor slowdown may still happen so maybe it's not worth it. What do you think, @gaearon?

gaearon · 2018-09-04T11:16:51Z

packages/react-dom/src/__tests__/ReactServerRenderingHydration.js

+      element,
+    );
+
+    delete document.documentMode;


This should be in try-finally. Even if test fails you want to delete it to avoid breaking other tests

gaearon · 2018-09-04T11:18:56Z

packages/react-dom/src/client/ReactDOMComponent.js

+        // normalized. Since it only affects IE, we're skipping style warnings
+        // in that browser completely in favor of doing all that work.
+        // See https://github.com/facebook/react/issues/11807
+        if (!document.documentMode) {


Extract this to a top level variable like canDiffHydratedStyleForWarning

gaearon · 2018-09-04T13:27:01Z

I'll do follow-up fixes on master. Thanks!

mgol · 2018-09-04T13:50:33Z

Glad to help, thanks for landing. :)

…ismatch (facebook#13534) IE 11 parses & normalizes the style attribute as opposed to other browsers. It adds spaces and sorts the properties in some non-alphabetical order. Handling that would require sorting CSS properties in the client & server versions or applying `expectedStyle` to a temporary DOM node to read its `style` attribute normalized. Since it only affects IE, we're skipping style warnings in that browser completely in favor of doing all that work. Fixes facebook#11807

facebook-github-bot added the CLA Signed label Sep 1, 2018

mgol mentioned this pull request Sep 1, 2018

False positive warning about style mismatch when hydrating server markup in IE11 #11807

Closed

gaearon reviewed Sep 1, 2018

View reviewed changes

mgol force-pushed the ie11-style-warning branch from b7c9520 to 5211d2c Compare September 2, 2018 10:51

mgol force-pushed the ie11-style-warning branch from 5211d2c to 6b3e555 Compare September 2, 2018 12:29

mgol commented Sep 2, 2018

View reviewed changes

mgol force-pushed the ie11-style-warning branch 2 times, most recently from 0b0e01f to 4b1a413 Compare September 3, 2018 15:51

gaearon reviewed Sep 3, 2018

View reviewed changes

mgol force-pushed the ie11-style-warning branch from 4b1a413 to 923c9e7 Compare September 3, 2018 22:12

mgol force-pushed the ie11-style-warning branch from 923c9e7 to 56b4800 Compare September 4, 2018 08:26

gaearon reviewed Sep 4, 2018

View reviewed changes

gaearon merged commit 2d4705e into facebook:master Sep 4, 2018

mgol deleted the ie11-style-warning branch September 4, 2018 13:50

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

This was referenced Sep 5, 2024

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#65

Open

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make IE 11 not complain about non-crucial style attribute hydration mismatch #13534

Make IE 11 not complain about non-crucial style attribute hydration mismatch #13534

mgol commented Sep 1, 2018

gaearon Sep 1, 2018

mgol Sep 2, 2018

gaearon Sep 2, 2018

mgol Sep 2, 2018

pull-bot commented Sep 2, 2018 •

edited

Loading

react-dom

schedule

mgol Sep 2, 2018

gaearon Sep 3, 2018

mgol commented Sep 3, 2018

gaearon Sep 3, 2018

gaearon Sep 3, 2018

mgol Sep 3, 2018

gaearon Sep 3, 2018

mgol Sep 3, 2018

gaearon commented Sep 3, 2018

mgol commented Sep 3, 2018

gaearon commented Sep 3, 2018

mgol commented Sep 3, 2018

gaearon commented Sep 3, 2018

mgol commented Sep 4, 2018

gaearon Sep 4, 2018

gaearon Sep 4, 2018

gaearon commented Sep 4, 2018

mgol commented Sep 4, 2018

Make IE 11 not complain about non-crucial style attribute hydration mismatch #13534

Make IE 11 not complain about non-crucial style attribute hydration mismatch #13534

Conversation

mgol commented Sep 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented Sep 2, 2018 • edited Loading

react-dom

schedule

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Sep 3, 2018

mgol commented Sep 3, 2018

gaearon commented Sep 3, 2018

mgol commented Sep 3, 2018

gaearon commented Sep 3, 2018

mgol commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Sep 4, 2018

mgol commented Sep 4, 2018

pull-bot commented Sep 2, 2018 •

edited

Loading