Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 4, 2018

Conversation

@mgol
Copy link
Contributor

@mgol mgol commented Sep 1, 2018

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

},
{
"filename": "react.production.min.js",
"bundleType": "UMD_PROD",
"packageName": "react",
"size": 7217,
"gzip": 3050
"size": 7289,
Copy link
Member

@gaearon gaearon Sep 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert changes to this file

Loading

Copy link
Contributor Author

@mgol mgol Sep 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Loading

Copy link
Member

@gaearon gaearon Sep 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor Author

@mgol mgol Sep 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@mgol mgol force-pushed the ie11-style-warning branch from b7c9520 to 5211d2c Sep 2, 2018
@pull-bot
Copy link

@pull-bot pull-bot commented Sep 2, 2018

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

Loading

@mgol mgol force-pushed the ie11-style-warning branch from 5211d2c to 6b3e555 Sep 2, 2018
);
});

it('should not warn when the style property differs on whitespace only', () => {
Copy link
Contributor Author

@mgol mgol Sep 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member

@gaearon gaearon Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@mgol mgol force-pushed the ie11-style-warning branch 2 times, most recently from 0b0e01f to 4b1a413 Sep 3, 2018
@mgol
Copy link
Contributor Author

@mgol mgol commented Sep 3, 2018

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

Loading

expect(element.firstChild.style.textDecoration).toBe('none');
expect(element.firstChild.style.color).toBe('black');

spyOnDevAndProd(console, 'error');
Copy link
Member

@gaearon gaearon Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

// 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(/;$/, '');
Copy link
Member

@gaearon gaearon Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor Author

@mgol mgol Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@gaearon gaearon Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor Author

@mgol mgol Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Sep 3, 2018

A few more points:

  • 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?

  • 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.

Loading

@mgol
Copy link
Contributor Author

@mgol mgol commented Sep 3, 2018

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)

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Sep 3, 2018

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

Loading

@mgol mgol force-pushed the ie11-style-warning branch from 4b1a413 to 923c9e7 Sep 3, 2018
@mgol
Copy link
Contributor Author

@mgol mgol commented Sep 3, 2018

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.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Sep 3, 2018

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?

Loading

…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 mgol force-pushed the ie11-style-warning branch from 923c9e7 to 56b4800 Sep 4, 2018
@mgol
Copy link
Contributor Author

@mgol mgol commented Sep 4, 2018

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?

Loading

element,
);

delete document.documentMode;
Copy link
Member

@gaearon gaearon Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

// 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) {
Copy link
Member

@gaearon gaearon Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract this to a top level variable like canDiffHydratedStyleForWarning

Loading

@gaearon gaearon merged commit 2d4705e into facebook:master Sep 4, 2018
1 check passed
Loading
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 4, 2018

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

Loading

@mgol mgol deleted the ie11-style-warning branch Sep 4, 2018
@mgol
Copy link
Contributor Author

@mgol mgol commented Sep 4, 2018

Glad to help, thanks for landing. :)

Loading

jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants