New: Add hint to check if inline CSS styles are used

[bolah2009]

Pull request checklist

Make sure you:

• Signed the Contributor License Agreement
• Followed the commit message guidelines

For non-trivial changes, please make sure you also:

• Added/Updated related documentation.
• Added/Updated related tests.

Short description of the change(s)

This PR adds a new hint that checks if the HTML is using inline CSS styles.

Why is this important?

The use of inline CSS styles prevent the reuse of the styles anywhere else. The html markup of the page becomes hard to read for the naked eye. The inline CSS styles are hard to maintain and does not provide consistency since they are not stored in a single place. The inline styles are repeated downloaded by the client on every request since it does not provide you with browser cache advantage. Inline styles take precedence of external stylesheets, this could accidentally override styles that you did not intend to overwrite.

What does the hint check?

This hint checks if the HTML is using inline CSS styles.

Examples of inline CSS styles

<div style="color: blue;"></div>

<style></style>

It checks that no element has the attribute style. It also checks that no internal styles <style> is used.

Full details in the readme.

[sarvaje]

Thanks @bolah2009 for your PR!!!

I like the idea of this hint, but we need to be careful, because it can report a lot of false positives.

The attribute style can be generated by some javascript's in the page or for a 3rd party library. Also, please correct me if I'm wrong, but for shadow elements is very common to use the style tag.

IMO, this is a good hint to have, but the severity has to be changed for hint or info, and It should be disabled by default in configuration-web-recommended, and maybe enabled by default in configuration-development.

@molant @antross thoughts?

[sarvaje]

Just a few comments

[bolah2009]

Thanks for the review @sarvaje 🙂

IMO, this is a good hint to have, but the severity has to be changed for hint or info, and It should be disabled by default in configuration-web-recommended, and maybe enabled by default in configuration-development.

I agree that the severity can be changed, I will change it to hint.

For the configuration aspect, if it's not added to the configuration-web-recommended (index.json file) will it be enabled by default? Also, should I add it to the configuration-development?

[bolah2009]

@sarvaje I changed the severity from error to hint, also removed the CHANGELOG file. I have a question about the configuration, see #4161 (comment).

Thanks! 😊

[sarvaje]

For the configuration aspect, if it's not added to the configuration-web-recommended (index.json file) will it be enabled by default? Also, should I add it to the configuration-development?

If I remember correctly, these files should be modify in a separate PR to enable the hint. @antross am I right?

I agree that the severity can be changed, I will change it to hint.

I have been thinking about this and I think it makes a lot of sense when you are developing, so you don't add any inline styles to your source code, so maybe we can increase the severity for the configuration-development maybe to warning? @antross @molant thoughts?

[bolah2009]

Pinging @sarvaje @antross @molant as a reminder. 🙂

[antross]

Thanks again for the PR @bolah2009! Sorry for the slow response over the holidays.

Given the potential for false-positives, even during development, I'm hesitant to add this to any of our default configurations, though I think it could be a great hint for people to opt-in to when they want it.

Out of curiosity, are you aware of any external blogs or other resources discussing why this should be a recommendation? I don't see this as blocking unless we decide to add the hint to a default configuration, but it'd be great to reference a few sources in the README if we can.

[bolah2009]

@antross Thanks for the review!

Yes, I am aware of external source discussing why this should be a recommendation. Some are:

• https://www.nomensa.com/blog/2011/css-inline-styles-and-why-they-are-considered-harmful-accessibility
• http://www.nullskull.com/faq/99/css-disadvantages-of-using-inline-styles.aspx

About false positives, I think many of the good use cases are generating them with JavaScript, wouldn’t it be ok to have it in configuration-development as @sarvaje suggested? 🤔

@antross Should I add those links?

[bolah2009]

@antross Should I add the link to the readme?

[antross]

Should I add the link to the readme?

Yes, I think the first link (from nomensa.com) would be a helpful additional resource to reference. I think the second can be left out as it doesn't add much to the first.

About false positives, I think many of the good use cases are generating them with JavaScript, wouldn’t it be ok to have it in configuration-development as @sarvaje suggested?

As far as I know, reporting use of the style attribute should be okay to have on-by-default in all configurations.

My main concern is reporting use of the <style> element. Even scoped to configuration-development that would flag false-positives for HTML content as part of defining a component in some frameworks (e.g. Vue single-file components).

[antross]

@bolah2009 to be clear I think we can still keep both checks in this hint. If we want to get style attribute checks on-by-default I'd propose the following:

1. Add a hint-level option to enable checks for <style> tags and set it off-by-default
2. Switch from "traverse::end" which analyzes the live DOM to "parse::end::html".

The second change would enable analyzing only raw source (as opposed to the live DOM) which would avoid false-positives on style attributes dynamically modified by script.

With those two changes I think we should be able to turn this hint on by default in all configurations.

[bolah2009]

@antross

I have made the following changes you suggested:

1. Add external source discussing why this should be a recommendation to the Readme
2. Add a hint-level option to enable checks for <style> tags and set it off-by-default
3. Switch from "traverse::end" which analyzes the live DOM to "parse::end::html"
4. Update readme with how to configure the hint for <style> tags

Thanks for the suggestion. 🙂

[antross]

Thanks @bolah2009 for the updates! I particularly appreciate your thoroughness in getting the new option plumbed through, including updating the README and tests.

@sarvaje any further comments? This looks ready-to-merge to me. Then we can do a follow-up PR to update the default configurations.

[sarvaje]

LGTM!!

I just have one question :)

[antross]

@bolah2009 thanks for putting together this hint and for working with us to iron out the details! 🎉

If you'd like to take a stab at a follow-up PR to add this to the default configurations, see #4079 for a recent example.

[bolah2009]

Thanks @antross. I will open a PR to add it to the default configuration.

[bolah2009]

Hi @antross @sarvaje

I opened the PR to add the default configuration in #4257

I noticed the hint is not available (published) yet on npm, how long does it take to get published? Is there a way to use it before it been published? Thanks 😊

[antross]

@bolah2009 yeah, we don't have an auto-publish step set up currently (though we've talked about adding one).

We're planning to run another release within the next couple of weeks, but in the meantime you should be able to use this hint with the CLI or VS Code Extension by saving its built output locally and pointing to the local path in your .hintrc file. See https://webhint.io/docs/user-guide/configuring-webhint/using-relative-resources/ for an example .hintrc that does this (though your path will probably look more like ./hint-no-inline-styles/dist/src/hint.js).

[bolah2009]

Okay, thanks 😊