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.

Files: 
CommentFileSizeAuthor
#75 1924430-75-base-html.patch5.89 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 56,869 pass(es).
[ View ]
#71 1924430-71-base-html.patch5.89 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 56,460 pass(es).
[ View ]
#63 1924430-63-base-html.patch5.88 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 57,975 pass(es).
[ View ]
#60 1924430-60-base-html.patch6.93 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]
#57 drupal-base-css-1924430-57.patch6.9 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 58,186 pass(es).
[ View ]
#53 1924430-53-drupal-base.patch6.74 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] 56,115 pass(es), 67 fail(s), and 47 exception(s).
[ View ]
#53 1924430-53-interdiff.txt854 bytesJohnAlbin
#51 drupal-base-css-1924430-51.patch6.86 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 56,079 pass(es).
[ View ]
#47 drupal-base-css-1924430-47.patch6.77 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 55,337 pass(es).
[ View ]
#45 drupal-base-css-1924430-45.patch6.76 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 57,550 pass(es).
[ View ]
#43 drupal-base-css-1924430-43.patch6.76 KBechoz
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#42 drupal-base-css-1924430-42.patch6.76 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 55,805 pass(es).
[ View ]
#40 drupal-base-css-1924430-40.patch4.97 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 56,726 pass(es).
[ View ]
#39 drupal-base-css-1924430-39.patch6.76 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 55,449 pass(es).
[ View ]
#32 drupal-base-css-1924430-32.patch7.29 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 55,491 pass(es).
[ View ]
#31 1924430-31-drupal.base_.css_.patch6.66 KBizus
PASSED: [[SimpleTest]]: [MySQL] 55,531 pass(es).
[ View ]
#29 1924430-29-drupal.base_.css_.patch4.8 KBizus
PASSED: [[SimpleTest]]: [MySQL] 55,378 pass(es).
[ View ]
#25 1924430-24-drupal.base_.css_.patch6 KBizus
PASSED: [[SimpleTest]]: [MySQL] 55,301 pass(es).
[ View ]
#19 1924430-19-drupal.base_.css_.patch7.49 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-19-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 1924430-3-drupal.base_.css-18.patch6.12 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 54,203 pass(es).
[ View ]
#8 1924430-8-drupal.base_.css_.patch6.1 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-8-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 1924430-3-drupal.base_.css_.patch7.01 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-3-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1924430-1-drupal.base_.css_.patch6.55 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-1-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new6.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-1-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's the patch.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-3-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

How about html.base.css?

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

Adding tags

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

Status:Needs work» Needs review
StatusFileSize
new6.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-8-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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

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

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!

Status:Needs work» Needs review

> 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.

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.

Status:Needs work» Needs review
StatusFileSize
new6.12 KB
PASSED: [[SimpleTest]]: [MySQL] 54,203 pass(es).
[ View ]

Re-roll.

StatusFileSize
new7.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1924430-19-drupal.base_.css_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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?

Fix status.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,301 pass(es).
[ View ]

Hi,
rerolling #19
Thanks

Status:Needs review» Needs work

The patch now applies well.

Status:Needs work» Needs review

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

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.

Status:Needs work» Needs review
StatusFileSize
new4.8 KB
PASSED: [[SimpleTest]]: [MySQL] 55,378 pass(es).
[ View ]

Oh i missed that, Thanks
Here it is !

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.

Status:Needs work» Needs review
StatusFileSize
new6.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,531 pass(es).
[ View ]

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

StatusFileSize
new7.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,491 pass(es).
[ View ]

@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.

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

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

Status:Needs review» Reviewed & tested by the community

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

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new6.76 KB
PASSED: [[SimpleTest]]: [MySQL] 55,449 pass(es).
[ View ]

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.

StatusFileSize
new4.97 KB
PASSED: [[SimpleTest]]: [MySQL] 56,726 pass(es).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.76 KB
PASSED: [[SimpleTest]]: [MySQL] 55,805 pass(es).
[ View ]

Re-roll of #39

StatusFileSize
new6.76 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.76 KB
PASSED: [[SimpleTest]]: [MySQL] 57,550 pass(es).
[ View ]

Yet another re-roll.

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new6.77 KB
PASSED: [[SimpleTest]]: [MySQL] 55,337 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

This applies cleanly.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new6.86 KB
PASSED: [[SimpleTest]]: [MySQL] 56,079 pass(es).
[ View ]

re-rolled!

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!!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new854 bytes
new6.74 KB
FAILED: [[SimpleTest]]: [MySQL] 56,115 pass(es), 67 fail(s), and 47 exception(s).
[ View ]

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.

@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,
#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. :-)

Status:Needs work» Needs review
StatusFileSize
new6.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,186 pass(es).
[ View ]

Here ya go...

Status:Needs review» Reviewed & tested by the community

Perfect! Thanks, echoz!

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

Status:Needs work» Needs review
StatusFileSize
new6.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]

Re-rolled.

Status:Needs review» Reviewed & tested by the community

This was a comment change, and now applies cleanly.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,975 pass(es).
[ View ]

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.

Status:Needs work» Reviewed & tested by the community

It applies perfect ;)

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

Status:Reviewed & tested by the community» Needs review

Status:Needs review» Needs work

Status:Needs work» Needs review

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

Status:Needs review» Reviewed & tested by the community

Finally, pass the test ;)

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

Status:Needs work» Needs review
StatusFileSize
new5.89 KB
PASSED: [[SimpleTest]]: [MySQL] 56,460 pass(es).
[ View ]

reroll

Status:Needs review» Reviewed & tested by the community

the patch in #71 seems good !
Thanks

This is RTBC from me as well.

Issue tags:+RTBC July 1

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new5.89 KB
PASSED: [[SimpleTest]]: [MySQL] 56,869 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

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.