We should add a CSS coding standard:

Browser hacks and !important styles are only allowed in themes. Not in core/module CSS files.

That is, because they are more or less irreversible.

There are always proper ways to avoid them.

CommentFileSizeAuthor
#5 drupal.css_.5.patch3.73 KBsun

Comments

jacine’s picture

jacine
she/her
Location New York
commented

Yes, please.

jwolf’s picture

jwolf commented

Yes please, some more!

casey’s picture

casey commented

+1 This keeps css more maintainable.

Although we shoud make some exceptions for system module (like .clearfix)

xmacinfo’s picture

xmacinfo
he/him
Primary language French
Location Canada
commented

I love standards.

Is this a meta issue? Or will a single patch resolve all CSS standard issues?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Category: feature » task
Status: Active » Needs review
StatusFileSize
new drupal.css_.5.patch3.73 KB

Why not? Actually, that's *much less* than I imagined :)

robloach’s picture

robloach
he/him
commented

There must be more CSS hacks then just this...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: CSS coding standards: No browser hacks, no !important in core/module CSS, only themes » CSS coding standards: No browser hacks, no !important in core/modules, ONLY IN THEMES

Clarifying title. ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/system/system-behavior.css	3 Mar 2010 20:19:18 -0000
@@ -63,11 +63,6 @@ html.js fieldset.collapsible legend span
 html.js fieldset.collapsible {
   position: relative;
 }

Looking at this (again...), it's quite unusual to apply totally different (wrapping) styles to elements, just because JavaScript is enabled, or not? Totally prone to errors IMO... who actually tests core with JS disabled?!

Powered by Dreditor.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

All fieldset-specific stuff now lives in a patch over in #745380: CSS contains too many needless differences between JS and no-JS versions

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

Aforementioned issue has been committed.

+++ misc/vertical-tabs.css	3 Mar 2010 20:24:45 -0000
@@ -16,7 +16,7 @@ div.vertical-tabs ul.vertical-tabs-list 
 div.vertical-tabs .vertical-tabs-panes fieldset.vertical-tabs-pane {
-  margin: 0 !important;
+  margin: 0;

Can someone explain this !important?

+++ modules/contextual/contextual.css	3 Mar 2010 20:07:11 -0000
@@ -88,8 +88,7 @@ div.contextual-links-wrapper a {
 ul.contextual-links li a {
-  /* Color made important so not easily overriden with '#footer a' type selectors */
-  color: #333 !important;
+  color: #333;

Let's straight roll back that patch.

+++ modules/system/system.css	3 Mar 2010 20:24:58 -0000
@@ -125,10 +125,10 @@ tr.odd .form-item, tr.even .form-item {
 tr.merge-down, tr.merge-down td, tr.merge-down th {
-  border-bottom-width: 0 !important;
+  border-bottom-width: 0;
 }
 tr.merge-up, tr.merge-up td, tr.merge-up th {
-  border-top-width: 0 !important;
+  border-top-width: 0;
 }

Prefixing the selectors with 'table' should probably be sufficient.

132 critical left. Go review some!

aspilicious’s picture

aspilicious commented

crosslinking: #766444: Add !important to CSS attributes of .element-invisible in system-behavior.css

james.elliott’s picture

james.elliott commented

subscribe

casey’s picture

casey commented

What about "!important may only be used when the CSS declaration is ment not to be overridden in any circumstance"?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Would be cool to resolve the 2-3 remaining issues before beta. Any takers?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: sun » Unassigned
jessebeach’s picture

jessebeach commented

@sun, was drupal.css_.5.patch committed? If so, what are the 2 - 3 remaining issues you mentioned?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The remaining issues are those outlined in #10

nomonstersinme’s picture

nomonstersinme commented

I agree that !important should not be used in core and that styling elements based on JS being enabled is a bit annoying. I especially don't think modules should have any !important's...

+++ modules/contextual/contextual.css	3 Mar 2010 20:07:11 -0000
@@ -88,8 +88,7 @@ div.contextual-links-wrapper a {
ul.contextual-links li a {
-  /* Color made important so not easily overriden with '#footer a' type selectors */
-  color: #333 !important;
+  color: #333;

This is definitely there for the reason the comment states but I've got a problem with these contextual links. I don't like the way they are handled - I think that its css should always load LAST, personally. I hate when I style list items and then my contextual links are all messed up. It creates unnecessary work that I usually just don't even bother with and my cogs are a mess. ;) Obviously that is a whole other issue.

Maybe the way to 'solve' this is to add additional selectors? a bit clunky and not really a proper solution but maybe it would help in getting rid of the !important:

#header ul.contextual-links li a,
#content ul.contextual-links li a,
#footer ul.contextual-links li a,
ul.contextual-links li a {
  color: #333;
}
steinmb’s picture

steinmb commented

subscribe

jacine’s picture

jacine
she/her
Location New York
commented
Version: 7.x-dev » 8.x-dev
Component: markup » CSS
jessebeach’s picture

jessebeach commented

We can't be dogmatic about !important. It's a tool, not a curse. Take these declarations in system.base.css for example.

/**
 * Hide elements visually, but keep them available for screen-readers.
 *
 * Used for information required for screen-reader users to understand and use
 * the site where visual display is undesirable. Information provided in this
 * manner should be kept concise, to avoid unnecessary burden on the user.
 * "!important" is used to prevent unintentional overrides.
 */
.element-invisible {
  position: absolute !important;
  clip: rect(1px, 1px, 1px, 1px);
  overflow: hidden;
  height: 1px;
}

/**
 * The .element-focusable class extends the .element-invisible class to allow
 * the element to be focusable when navigated to via the keyboard.
 */
.element-invisible.element-focusable:active,
.element-invisible.element-focusable:focus {
  position: static !important;
  clip: auto;
  overflow: visible;
  height: auto;
}

The !importants are necessary to ensure proper functioning of these accessibility-enabling classes. When you override them, which we do in contextual.base.css

/* Override the position for contextual links. */
.contextual-region .element-invisible.element-focusable:active,
.contextual-region .element-invisible.element-focusable:focus,
.contextual-region:hover .element-invisible.element-focusable,
.contextual-region-active .element-invisible.element-focusable,
.touch .contextual-region .element-invisible.element-focusable  {
  position: relative !important;
}

You do it with intention. An absolute interdiction on the use of !important in our guidelines might cause some overzealous and green CSS purists to mess with these declarations.

I like what the proposed CSS Architecture guidelines have to say about !important

The !important flag should be used appropriately. Since it overrides all external stylesheet rules, it is useful for states that must supersede all others, no matter the module variant or other element state. For example, error or disabled states are appropriate places to use !important, since they must be consistent and always apply when present. Never use !important to resolve specificity problems for general CSS rules. Additionally, module developers should take extra care with the !important flag, since all module CSS should be easy to override.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Needs work » Fixed

I agree with Jesse. But I'll bet we have some !importants that could be removed somewhere.

The patch above is way out-of-date, so let's close this issue and re-open a new issue when we start implementing the new CSS Architecture guidelines in D8.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: CSS coding standards: No browser hacks, no !important in core/modules, ONLY IN THEMES » CSS coding standards: No !important in core/contrib modules, ONLY IN THEMES
Status: Fixed » Active

The point of this issue is to explicitly disallow usage of !important in CSS of core and contributed modules, unless there's a very very good reason for using it.

One of those very good reasons are the .element-hidden, .element-invisible, etc classes defined in system.base.css. Those do not need any further explanation/clarification.

I didn't have time to check whether the remaining instances in #10 still exist, but all of them are problematic. It's also possible that latest HEAD contains new instances.

Likewise, as @nomonstersinme mentioned, elements/widgets that receive special styling (like contextual links) are a "third" category, which we need to address. As a themer, I need to be able to (partially) adjust/override those styles and I certainly don't want to and should not have to use !important for everything I define. At the same time, if my other CSS targets more specific elements that also happen to be contained within those specially styled elements, then my custom styles should not negatively affect those elements.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Active » Fixed

We now provide guidance on !important in our CSS coding standards, hooray!

The !important flag should be used sparingly and appropriately, and in general should be restricted to themes. Since it overrides all external stylesheet rules, it is useful for states that must supersede all others, no matter the component variant. For example, error or disabled states are appropriate places to use !important, since they must be consistent and always apply when present. Never use !important to resolve specificity problems for general CSS rules. Additionally, Drupal core and contrib modules should avoid the !important flag, since all module CSS should be easy to override.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.