Bug 513250 - [css][lint] Update CSSLint messages and severities to recognize that many are style issues
Summary: [css][lint] Update CSSLint messages and severities to recognize that many are...
Status: RESOLVED FIXED
Alias: None
Product: Orion (Archived)
Classification: ECD
Component: JS Tools (show other bugs)
Version: 13.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 15.0   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 508847
Blocks:
  Show dependency tree
 
Reported: 2017-03-07 10:56 EST by Curtis Windatt CLA
Modified: 2017-03-27 13:37 EDT (History)
2 users (show)

See Also:


Attachments
Before (55.63 KB, image/png)
2017-03-27 13:26 EDT, Curtis Windatt CLA
no flags Details
After (48.83 KB, image/png)
2017-03-27 13:26 EDT, Curtis Windatt CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Curtis Windatt CLA 2017-03-07 10:56:28 EST
CSSLint running results in many of our existing CSS files being littered with problems.  The problems have no explanation on why they are bad style (or wrong) and most do not explain how to fix them or offer quickfixes.

For example, several of the rules deal with IE issues that have not been a problem for multiple versions of IE.
See: adjoining classes, too many selectors, import limit

Updating to a new version of CSSLint provides a good opportunity to improve this.
Comment 1 Curtis Windatt CLA 2017-03-07 12:18:39 EST
Too many font-size properties should have its message updated to indicate how to fix it.  Right now it talks about abstraction, but instead you should be using a relative size like 1em or a percentage, rather than a specific px.
http://www.webdeveloper.com/forum/showthread.php?262347-RESOLVED-How-to-abstract-font-sizes

Unknown value for property could be warning instead of info.
Duplicate property should be warning.

!important should have a message explaining why important is bad 

Heading should not be qualified should have a message explaining why combining a class with a H1 element is poor style. Similar problem for heading element should have consistent appearance, heading styles should only be defined once.

Too many web fonts
Too many floats
Too many font-size
Too many !important
These add warnings based on the number that are used in a file. Not helpful since each use is already marked as being bad style.

Don't use ids in selectors
don't qualify headings
disallow hiding outline
outlines without focus
Are style rules (some for accessibility). Discouraging their use might be helpful, but nagging the user who does use them is not helpful. The fix for these is to allow the rule to be turned off for the file or for that one line.
Comment 2 Curtis Windatt CLA 2017-03-07 12:25:39 EST
unknown-property should be warning
Comment 3 Curtis Windatt CLA 2017-03-07 12:32:36 EST
validate-box-model should have messages that indicate use of padding, border, etc. will increase the final computed box size of the element beyond what is set in the width property.
Comment 4 Curtis Windatt CLA 2017-03-07 12:37:31 EST
All setttings values will have to be reviewed as many still default to warning, but they show up as info instead.
Comment 5 Michael Rennie CLA 2017-03-07 13:00:46 EST
(In reply to Curtis Windatt from comment #1)
> Too many font-size properties should have its message updated to indicate
> how to fix it.  Right now it talks about abstraction, but instead you should
> be using a relative size like 1em or a percentage, rather than a specific px.
> http://www.webdeveloper.com/forum/showthread.php?262347-RESOLVED-How-to-
> abstract-font-sizes
> 
> Unknown value for property could be warning instead of info.
> Duplicate property should be warning.
> 
> !important should have a message explaining why important is bad 
> 
> Heading should not be qualified should have a message explaining why
> combining a class with a H1 element is poor style. Similar problem for
> heading element should have consistent appearance, heading styles should
> only be defined once.
> 
> Too many web fonts
> Too many floats
> Too many font-size
> Too many !important
> These add warnings based on the number that are used in a file. Not helpful
> since each use is already marked as being bad style.
> 
> Don't use ids in selectors
> don't qualify headings
> disallow hiding outline
> outlines without focus
> Are style rules (some for accessibility). Discouraging their use might be
> helpful, but nagging the user who does use them is not helpful. The fix for
> these is to allow the rule to be turned off for the file or for that one
> line.

All of that makes sense to me.

It would also be helpful to link to the MDN doc (to try and avoid bias, emphasis on try).

For example for !important: https://developer.mozilla.org/en/docs/Web/CSS/Specificity#The_!important_exception
Comment 6 Curtis Windatt CLA 2017-03-07 14:16:31 EST
Too many !important declarations rule does not have a configurable severity and always shows as warning.

We should remove the 'approaching' limit rules from the config entirely. No need to clutter up the settings page.
Comment 7 Curtis Windatt CLA 2017-03-07 16:56:38 EST
After talking with Mike and Steve, I think we can take this bug one step further and turn off style guideline problems by default. Assuming we wanted to keep the style rules around, we could simplify the CSS Validation settings page to have 'enforce style guidlines' 'parse errors' and 'potential problems' as three options rather than 20 individual rules.

For a CSS newbie, having some indication that what they are writing is bad form might be useful. So maybe on an empty CSS file we put an info annotation that tells the user that they can turn on style guidelines.
Comment 8 Curtis Windatt CLA 2017-03-23 14:56:55 EDT
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=4525718865db16cbf0ce2b42c8716c76eafda774
First cut of fix, organizes the settings into different types, fixes missing alphabetical rule, updates some settings wording and reduces the severity of pretty much everything.
Comment 9 Curtis Windatt CLA 2017-03-23 16:21:53 EDT
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=c4bf3186b839827ebce5bccbc1a48de4907c976f
Updates problem messages and reduces severity of box-sizing.

I'm closing this as FIXED.  The new text helps the user understand what each setting does or how to fix the problem.  Default severities are ignore for all the rules relating to style/performance.
Comment 10 Steve Northover CLA 2017-03-27 12:25:52 EDT
I'd love to have a before and after demo.  What I find usually is that 99% of the changes are good and sometimes there is "one that is on" that "should be off".  Of course, this is all opinion.

The general rule:  

Error for things that stop the code from running (ie syntax error)
Warning for serious problems (undeclared variables)
Info for things that are not serious
Ignore for "non-issues"
Comment 11 Curtis Windatt CLA 2017-03-27 13:26:36 EDT
Created attachment 267483 [details]
Before
Comment 12 Curtis Windatt CLA 2017-03-27 13:26:50 EDT
Created attachment 267484 [details]
After
Comment 13 Curtis Windatt CLA 2017-03-27 13:37:29 EDT
(In reply to Steve Northover from comment #10)
> I'd love to have a before and after demo.  What I find usually is that 99%
> of the changes are good and sometimes there is "one that is on" that "should
> be off".  Of course, this is all opinion.

I attached a screenshot with an example.  Most of the remaining blue in the after picture is from empty rules. The yellow and red are from unexpected property values.

> 
> The general rule:  
> 
> Error for things that stop the code from running (ie syntax error)
> Warning for serious problems (undeclared variables)
> Info for things that are not serious
> Ignore for "non-issues"

For CSS:
Error for syntax problems
Warning for potentially broken CSS (bad property name or bad property value)
Info for mistakes (empty rules, potentially wrong property combos, common browser compatibility)
Ignore for style, performance and old-school IE compatibility issues. If the user cares about shrinking their file by a few characters or work on IE6 they should be running a separate build tool.