fix: move style element to document head

[mbrookes]

Motivation

Test plan

https://validator.w3.org/

[satya164]

This change has several issues:

1. Adding styles after mount means there'll be unstyled content before Javascript gets to execute and component finishes mounting, which is not desirable.
2. SSR will no longer work for the styles. With a style tag, it's automatic and requires no config.
3. This change never removes the styles after adding it. While it can be fixed, letting React manage it is much simpler.

I don't really care about making some validator happy. All browsers I have tested this in support this pattern, and not doing this only causes issues.

[oliviertassinari]

I agree, I don't think that we can accept this approach. How about we add an opt-in flag to remove the default <style> injection and export the styles as a variable? This way, developers that are willing to do the extra mile can move the style into the head, e.g. using emotion Global component.

In the case of Material-UI, it's important that the output of https://validator.w3.org/ doesn't shallow issues with the components. In this case <style> element inside the body is discouraged by the spec because it leads to reflow during the initial rendering, degrading performance.

Alternatively, @mbrookes did you consider not rendering react-simple-code-editor in the initial SSR output?

[oliviertassinari]

Also, note that type="text/css" is no longer encouraged. It can/should be removed.

[mbrookes]

satya164

1 & 2a: Since the CSS sets the font color of placeholder text for the underlying textfield, before which it's transparent, there's no FOUC or re-layout. What's the underlying concern?
2b: This doesn't require configuration?
3: That was a conscious decision.

@oliviertassinari

did you consider not rendering react-simple-code-editor in the initial SSR output?

"We add HTML validation warnings. [...] They can all be fixed in react-simple-code-editor"

So, no.

How about we add an opt-in flag to remove the default <style> injection and export the styles as a variable?

I don't think having a prop to enable rendering valid HTML, with the added dev overhead of injecting the styles locally, is a good tradeoff. NoSsr will do.

[oliviertassinari]

What's the underlying concern?

@mbrookes From what I understand, the issue with the placeholder color is that, when the source is empty, the placeholder would flash at the hydration time. So the current solution doesn't work.

[oliviertassinari]

How about we:

• Move the styles to componentDidMount, we probably don't need to unmount them, as emotion is doing. It should help with performance (already done).
• Only keep the CSS for IE (remove the logic for modern browsers)
• Only apply the -webkit-text-fill-color style, with inline-style if the sources are not empty

? Only IE would get a degraded experience.

[satya164]

Since the CSS sets the font color of placeholder text for the underlying textfield, before which it's transparent, there's no FOUC or re-layout. What's the underlying concern?

CSS does 2 things:

1. Set color of placeholder to be visible. Adding styles in JS will make it so that no placeholder will be visible until content has mounted.
2. Makes sure that text is transparent on IE. Otherwise there'll be highlighted text and normal text overlaid on top of each other creating a weird effect.

This doesn't require configuration

It does if you want to solve the above issues when doing SSR

That was a conscious decision

If a component does some side-effect like adding elements to the page, it should also cleanup the elements after it's no longer there. The problem with cleanup here is that there's one global style inserted by the component, so removal won't be possible cleanly, but it's still a tradeoff.

[satya164]

Only IE would get a degraded experience.

While I understand that IE supprting IE is no longer a priority, IMO degrading the experience just to make a validator/linter happy is a reasonable tradeoff. Linters are supposed to be helpful to identify potential errors, not an be an authority over what to do.

Right now IE is a supported browser and the experience was as expected before the change. If we explicitly provide a degraded experience in a browser when we could avoid it, then we don't really support it.

Also keep in mind that many other projects use the package, and they might need to support IE (though I have no data), and this change will break their UX. So we need to be mindful of such breaking changes. What's not important for you maybe important for others.

How about we take another route? Keep the IE specific style in <style> tag and introduce a prop to not add the style (e.g. ieSupport={true} or something else) and note the prop in README with an explanation that it'll insert a style tag which will not pass the w3c validator. Maybe in next major release, the prop can be false by default and users would need to enable it explicitly if they want.

This will make the linter happy for your case, while also having an option to provide good UX for people who need IE support.

[oliviertassinari]

The proposed approach with the boolean prop sounds great 👌

From my perspective, root of the issue is that body > style is not a great practice: whatwg/html#1605 (comment), it's discouraged for the very same regression the current changes introduce for IE SSR users: FOUC. I would imagine this package targets an audience of developers, hence IE support is not important (e.g. CodeSandbox/Next.js not supporting it for development).

[satya164]

it's discouraged for the very same regression the current changes introduce for IE SSR users: FOUC.

While generally it can be bad for performance, from my understanding, this isn't an issue in this particular case.

I would imagine this package targets an audience of developers, hence IE support is not important

The thing is that it already supported IE properly, and we don't know if certain users realy on this. So regressing that would be a breaking change.

Anyway, let's add the prop for now.

[mbrookes]

we don't know if certain users realy on this

Okay, let's take a step back here and be pragmatic. Nobody but nobody who is using IE11, an 8 year old browser, cares a bit about, or will even notice, a tiny FOUC. Half the internet doesn't work for them already.

If it really, really worries you that much, let's drop IE11 support entirely, and bump the major version. IE11 is EOS in eight months. Microsoft are dropping support for it in O365 in six. It's dead Jim.

[satya164]

Nobody but nobody who is using IE11, an 8 year old browser, cares a bit about, or will even notice, a tiny FOUC.

Doesn't matter. I care about the quality of UX in projects I work on.

[mbrookes]

Sounds like you're saying you prefer option two?

[satya164]

I'd prefer not to drop support outright since it doesn't really take any work to maintain this and I would like to avoid breaking changes unless we have good reasons (e.g. maintainability etc.).

So I propose to introduce a prop which addresses both use cases: not do a breaking change, and also passes HTML validation for those who don't need IE support.

[satya164]

And if it causes more issues in future, then we should definitely drop support for IE and do a major version bump.

[mbrookes]

Just realised it's still version 0 anyway, so no need for a version bump for breaking changes according to SemVer. We can add the prop now, and could rip it out again in October with no qualms.

[oliviertassinari]

I do like the idea of degrading the experience for IE users, to create an incentive to upgrade and not penalize modern browsers.

We have done that many times in Material-UI, basically a best effort. What we have done is that if the support comes with a non reasonable cost for modern browsers (here bundle size, mainly) we degrade the UX or DX (if it helps).

@mbrookes What's wrong with the proposal for the opt-out prop for IE (outside of forcing bundle size)? #81 (comment)

[satya164]

@oliviertassinari Personally I'd much prefer to just drop IE support instead of intentionally degrading the UX. If you strongly feel about not supporting IE then feel free to do a major release removing IE support.

[satya164]

Also, bundle size will not be affected here either way unless we drop support entirely.

[oliviertassinari]

Also, bundle size will not be affected here either way unless we drop support entirely.

Do you mean not significantly? Removing the whole style and associated CSS should be less code. Completely removing IE might be too bold for the us cases of Material-UI. It would effectively mean that our docs doesn't render in IE at all (right now, it's only Next.js that broke it in 2-3 places in dev mode).

I would personally add a prop and move to something else. I don't think that the problem deserves a lot of attention.

[satya164]

Do you mean not significantly? Removing the whole style and associated CSS should be less code.

I mean this PR still keeps the CSS, so it won't change much. But removing whole style and associated CSS means dropping support entirely in which case it'd be a minor improvement.

[satya164]

I would personally add a prop and move to something else. I don't think that the problem deserves a lot of attention.

Agreed. It's simplest solution and makes everyone happy.

[oliviertassinari]

Closing as it has been stuck for over a year.