search

microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.95k stars 598 forks source link

[load-themed-styles] unnecessary type attribute on style tags leads to HTML validator warning #1673

Closed karanbirsingh closed 4 years ago

karanbirsingh commented 4 years ago

Our team uses load-themed-styles via office-ui-fabric-react, please feel free to redirect if needed - thanks!

Is this a feature or a bug?

Please describe the actual behavior.

loadTheme() creates style elements with a type attribute: https://github.com/microsoft/rushstack/blob/667e22f332988f57e77ec4651ab8f22a3ec08eba/libraries/load-themed-styles/src/index.ts#L398

When running the resulting HTML through the "Nu HTML Checker", we see the following warning:

The type attribute for the style element is not needed and should be omitted.

This occurs because the WHATWG HTML spec treats this as "obsolete but conforming behavior":

Authors should not specify a type attribute on a style element. (link)

The Mozilla docs say:

type This attribute defines the styling language as a MIME type (charset should not be specified). This attribute is optional and defaults to text/css if it is not specified — there is very little reason to include this in modern web documents. (link)

If the issue is a bug, how can we reproduce it? Please provide detailed steps and include a GitHub branch if applicable. Your issue will get resolved faster if you can make it easy to investigate.

This codepen uses office-fabric theming - inspecting the iframe head style tags shows text/css types. Running the nu validator on this page or any page with text/css types produces the warning.

This branch removes the line above. When testing locally via office-fabric theming, the functionality seemed identical & the warning disappeared. PR here if the change seems appropriate.

What is the expected behavior?

Generated style tags should not have the text/css type set - this will help clients of the library when testing for HTML validation

If this is a bug, please provide the tool version, Node.js version, and OS.

octogonz commented 4 years ago

Fixed by PR https://github.com/microsoft/rushstack/pull/1674