Problem/Motivation

Our current CSS is a mish-mash of styles that don't follow the SMACSS categorization standards that are now part of our CSS standards. See http://drupal.org/node/1887918

Proposed resolution

We need to reoganize our styles so that they are loaded in this order:

  1. "Base" HTML element styling
  2. "Layout" styling
  3. Component styling and their associated state and skin styles
  4. Un-associated state styles
  5. Un-associated skin styles

This is the second step in a proposed solution outlined at #1921610: [Meta] Architect our CSS. In short, we are moving all of core's "base" HTML element styling into a library so that we can state "Modules must never have base HTML element styling".

Remaining tasks

This patch is blocked by #1924368: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants.

After this issue's completion, we still have all the other steps outlined at #1921610: [Meta] Architect our CSS.

User interface changes

none

API changes

Adds a new drupal.base.css library that is loaded by default.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

Status: Active » Needs review
FileSize
6.55 KB

Here's the patch.

Status: Needs review » Needs work

The last submitted patch, 1924430-1-drupal.base_.css_.patch, failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
7.01 KB

Found some more base styling in the Overlay module. *sigh* So this is now in drupal.base-rtl.css:

html {
  direction: rtl;
}

Note, I have no idea why this new ruleset is needed BECAUSE THERE ARE NO COMMENTS FOR THIS RULESET. It was added in this patch with no discussion or comment: #766170-5: Overlay lacks rtl styling

Status: Needs review » Needs work

The last submitted patch, 1924430-3-drupal.base_.css_.patch, failed testing.

sun’s picture

Can we find a better file prefix than "drupal." ?

How about html.base.css?

Shyamala’s picture

Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

Adding tags

ry5n’s picture

Since this base CSS is now a core library, a Drupal `namespace` makes sense, doesn't it?

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
6.1 KB

I re-rolled this against head. There were a couple of conflicts in there that I'm not too sure about, so this will definitely need some review.

Kim

Status: Needs review » Needs work
Issue tags: -mobile, -d8mux, -d8mux-css-cleanup

The last submitted patch, 1924430-8-drupal.base_.css_.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

#8: 1924430-8-drupal.base_.css_.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1924430-8-drupal.base_.css_.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

#8: 1924430-8-drupal.base_.css_.patch queued for re-testing.

kim.pepper’s picture

Status: Needs review » Needs work

CSS_BASE is not being defined anywhere. Just re-read the summary and can see it is part of #1924368: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants

So much for a quick re-roll!

JohnAlbin’s picture

Status: Needs work » Needs review
JohnAlbin’s picture

> Since this base CSS is now a core library, a Drupal `namespace` makes sense, doesn't it?

It makes sense to me. And Morten was the one who actually suggested this name originally. It is how Drupal core thinks base HTML styling should be done.

kim.pepper’s picture

Issue tags: -mobile, -d8mux, -d8mux-css-cleanup

#8: 1924430-8-drupal.base_.css_.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

The last submitted patch, 1924430-8-drupal.base_.css_.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
6.12 KB

Re-roll.

JohnAlbin’s picture

The patch in #18 is missing the main element that was added to the block of HTML5 elements and the new input rule that was added to system.base.css.

Here's an updated patch.

rteijeiro’s picture

Status: Needs review » Needs work

The #19 patch applies well and it seems nothing is broken in the css styling.

Also the drupal.base.css is loaded.

Is there something more I should take into account?

rteijeiro’s picture

Fix status.

ry5n’s picture

Status: Needs work » Needs review

The CSS is all retained, it’s a straight consolidation, so that looks good.

I looked through system.module and found no straggling base styles except the new <details> and <summary> elements in system.theme.css. We can probably leave that for now, since <details> needs a js polyfill, has internal elements styled with classes, and may be better styled as a component anyway.

I can’t comment on the changes to theme.maintenance.inc or system.module, although everything seems to look correct with the patch applied.

izus’s picture

Issue tags: -mobile, -d8mux, -d8mux-css-cleanup

#19: 1924430-19-drupal.base_.css_.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

The last submitted patch, 1924430-19-drupal.base_.css_.patch, failed testing.

izus’s picture

Status: Needs work » Needs review
FileSize
6 KB

Hi,
rerolling #19
Thanks

rteijeiro’s picture

Status: Needs review » Needs work

The patch now applies well.

rteijeiro’s picture

Status: Needs work » Needs review

It seems right for me. Could anyone else confirm that?

echoz’s picture

Status: Needs review » Needs work

If the two patches are compared, we see that #24 is not removing a whole section of system.theme.css that #19 is removing, which is still currently in the file (plus the button element is now present). Assuming #19 was correct at that time, this needs work.

izus’s picture

Status: Needs work » Needs review
FileSize
4.8 KB

Oh i missed that, Thanks
Here it is !

echoz’s picture

Status: Needs review » Needs work

You left out drupal.base.css, what this issue is about. Don't forget there is the small ruleset for button, that was added since John's patch that has to go somewhere.

izus’s picture

Status: Needs work » Needs review
FileSize
6.66 KB

Hi,
taking into account the comments in #30.
Thanks

echoz’s picture

@izus, it's convenient to set our text editors to end files with a line break, as this is in our coding standards. #31 seems to have missed this, also mysteriously changed a comment, and failed to add that new button ruleset.

This patch should be an accurate rerole of the #19 patch.
I kept the new button ruleset with the button styles that it came with from #1719640: Use 'button' element instead of empty links in system.theme.css since I didn't think we want button {font-size: 1em;} in our drupal.base.css

Status: Needs review » Needs work
Issue tags: -mobile, -d8mux, -d8mux-css-cleanup

The last submitted patch, drupal-base-css-1924430-32.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

#32: drupal-base-css-1924430-32.patch queued for re-testing.

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
I reviewed #32 and it seems good for me.
Thanks

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/misc/drupal.base.cssundefined
@@ -0,0 +1,83 @@
+article,
+aside,
+details,
+figcaption,
+figure,
+footer,
+header,
+hgroup,
+main,
+menu,
+nav,
+section,
+summary {
+  display: block;
+}
...
+  border-collapse: collapse;
...
+img {
+  border: 0;
+}

Most of these declarations are in normalize.css and therefore I think might be unnecessary.

From normalize.css...

article,
aside,
details,
figcaption,
figure,
footer,
header,
hgroup,
nav,
section,
summary {
    display: block;
}
...
img {
    border: 0;
}
...
table {
    border-collapse: collapse;
    border-spacing: 0;
}

... so we only need main and menu in the bit that ensures display values for browsers that don't recognize the new elements.

echoz’s picture

Good catch, although currently only Seven uses core's normalize.css. #1839318: Replace drupal.base.css library with normalize.css for all themes is not committed yet but seems like it will be, although that issue has been stalled since mid March.

#32 could go in with a followup to remove the redundant parts when #1839318 gets committed (Seven currently uses both normalize.css and system.base.css with the overlap), or this issue can get postponed until then.

Notes on the html5 elements:

The version of normalize.css in core is out of date.
"main" is now in both v1 and v2 of normalize.

menu is no longer in normalize, we can get rid of it.
https://github.com/necolas/normalize.css/issues/24

Related, hgroup is being removed from html5, that could go too.

echoz’s picture

I opened #1986616: Update normalize.css library with a patch to update core's normalize.css library.

echoz’s picture

Status: Needs work » Needs review
FileSize
6.76 KB

If we want to go forward with committing this then following up with removing redundancies if and when #1839318: Replace drupal.base.css library with normalize.css for all themes gets committed, here is the patch with menu and hgroup removed.

Reference for hgroup removal, http://html5doctor.com/the-hgroup-element/ although this isn't gone from normalize.css.

LewisNyman’s picture

Here's a re-roll of the patch. This looks pretty good to me? RTBC?

echoz’s picture

Status: Needs review » Needs work

#40 leaves out drupal.base.css + drupal.base-rtl.css

echoz’s picture

Status: Needs work » Needs review
FileSize
6.76 KB

Re-roll of #39

echoz’s picture

Re-roll, system.module has been getting touched often lately.

Status: Needs review » Needs work

The last submitted patch, drupal-base-css-1924430-43.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
FileSize
6.76 KB

Yet another re-roll.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Checked. Patch applies well and drupal.base.css is loaded correctly. Go fo RTBC!

echoz’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.77 KB

system.module touched again. Testing that it applies should be all that's needed to rtbc again. Let's get this committed!

echoz’s picture

I mistakenly re-rolled before learning that's not needed when just line #s changed. It's the same patch as #45.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

This applies cleanly.

echoz’s picture

Status: Reviewed & tested by the community » Needs work
echoz’s picture

Status: Needs work » Needs review
FileSize
6.86 KB

re-rolled!

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies well and drupal.base.css file loads correctly.

Also tried with a RTL language and drupal.base-rtl.css loads correctly too.

Go for RTBC!!

JohnAlbin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
854 bytes
6.74 KB

Minor bugs discovered. In the latest patch, the button styling seems to have been inadvertently left in system.theme.css. Also, there's some HTML5 elements that are missing from the display: block rule.

Status: Needs review » Needs work

The last submitted patch, 1924430-53-drupal-base.patch, failed testing.

echoz’s picture

@JohnAlbin,
#37 explains the omission of menu and hgroup. Here's a resource link for hgroup too. http://html5doctor.com/the-hgroup-element/

#32 explains moving the button ruleset, but if you want that in drupal.base.css that's fine with me :-)

JohnAlbin’s picture

@JohnAlbin,
#37 explains the omission of menu and hgroup. Here's a resource link for hgroup too. http://html5doctor.com/the-hgroup-element/

#32 explains moving the button ruleset, but if you want that in drupal.base.css that's fine with me :-)

Sorry! Didn't realize you were trying to update the ruleset at the same time as moving them. Given #37, go ahead and roll a new patch with menu and hgroup removed from the base stylesheet.

However, the button ruleset is a base style and our new CSS guidelines for D8 say you CANNOT put a base style anywhere except in our base stylesheet. So either the ruleset goes in there as-is or the ruleset's selector needs to be rewritten to not be a HTML element. Looking at #1719640: Use 'button' element instead of empty links I see nod_ added it to make the button.link styling. IMO, the font-size: 1em; declaration should be moved to system.theme.css’ button.link ruleset and the button ruleset can just be removed.

echoz, can you roll a new patch? I can review. :-)

echoz’s picture

Status: Needs work » Needs review
FileSize
6.9 KB

Here ya go...

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Perfect! Thanks, echoz!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll

git ac https://drupal.org/files/drupal-base-css-1924430-57.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7061  100  7061    0     0   3173      0  0:00:02  0:00:02 --:--:--  3511
error: patch failed: core/includes/theme.maintenance.inc:92
error: core/includes/theme.maintenance.inc: patch does not apply
JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
6.93 KB

Re-rolled.

echoz’s picture

Status: Needs review » Reviewed & tested by the community

This was a comment change, and now applies cleanly.

JohnAlbin’s picture

Status: Reviewed & tested by the community » Needs work
JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
5.88 KB

This patch removes this now-obviously-redundant ruleset:

[dir=rtl] html {
  direction: rtl;
}

Here's documentation to back up my assertion. https://developer.mozilla.org/en-US/docs/Web/CSS/direction says:

This should normally be done as part of the document (e.g., using the dir attribute in HTML) rather than through direct use of CSS.

And, of course, the patch has been re-rolled so it no longer has a -rtl stylesheet. :-)

Status: Needs review » Needs work

The last submitted patch, 1924430-63-base-html.patch, failed testing.

pakmanlh’s picture

Status: Needs work » Reviewed & tested by the community

It applies perfect ;)

* Ops, I don't see the last update, sorry

pakmanlh’s picture

Status: Reviewed & tested by the community » Needs review
pakmanlh’s picture

Status: Needs review » Needs work
pakmanlh’s picture

Status: Needs work » Needs review

#63: 1924430-63-base-html.patch queued for re-testing.

pakmanlh’s picture

Status: Needs review » Reviewed & tested by the community

Finally, pass the test ;)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

git ac https://drupal.org/files/1924430-63-base-html.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  6018  100  6018    0     0   4781      0  0:00:01  0:00:01 --:--:--  5769
error: patch failed: core/modules/system/css/system.module.css:337
error: core/modules/system/css/system.module.css: patch does not apply
+++ b/core/misc/drupal.base.cssundefined
@@ -0,0 +1,89 @@
+[dir=rtl] caption {
...
+[dir=rtl] th {

This should be [dir="rtl"] see #2030925: quote rtl attribute selector

echoz’s picture

Status: Needs work » Needs review
FileSize
5.89 KB

reroll

izus’s picture

Status: Needs review » Reviewed & tested by the community

the patch in #71 seems good !
Thanks

JohnAlbin’s picture

This is RTBC from me as well.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

echoz’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.89 KB

needed re-roll to quote rtl attribute selector since #2030925: quote rtl attribute selector landed.

JohnAlbin’s picture

This new patch is RTBC from once the bot says its green.

echoz’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed be502b4 and pushed to 8.x. Thanks!

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