Sub-issue of #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core

Inline with the CSS cleanup efforts of the HTML5 initiative, using CSSLint at http://csslint.net provides a quick way to code-sniff our css and tweak styles.

  1. Copy and paste the the stylesheet(s) below into the css lint tool at http://csslint.net and test.
  2. Fix any warnings or errors the tool finds.
  3. Patch Drupal 8 locally and make sure the css changes have not broken anything visually.
  4. Create patch and upload for the testbot.

Files: core/modules/user/css/user.module.css and user.admin.css

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it does not add any features or fixes any bugs
Issue priority Normal because it doesn't affect any major systems
Unfrozen changes Unfrozen because it only changes css
Prioritized changes The main goal of this issue is fixing css coding standards
Disruption Not disruptive
CommentFileSizeAuthor
#65 1663210-64-user-edit-after.png131.73 KBidebr
#65 1663210-64-permissions-after.png564.99 KBidebr
#65 1663210-64-account-settings-after.png189.48 KBidebr
#65 1663210-64-install-after.png595.23 KBidebr
#64 clean-up-user-css-1663210-64.patch6.89 KBmortendk
#64 cleanupusercss-interdiff.txt492 bytesmortendk
#62 csscleanup-interdiff.txt3.98 KBmortendk
#58 clean_up_css_in_the-1663210-58.patch7.18 KBmortendk
#58 password-generator-no-theme.png55.69 KBmortendk
#58 password-generator-with-theme.png52.44 KBmortendk
#56 clean_up_css_in_the-1663210-56.patch4.55 KBmortendk
#56 with-patch.png181.39 KBmortendk
#56 without-patch.png180.65 KBmortendk
#55 Account_settings___Site-Install.png40.36 KBjoelpittet
#54 clean_up_css_in_the-1663210-54.patch4.51 KBmortendk
#52 permission-class-added.png137.2 KBmortendk
#52 clean_up_css_in_the-1663210-52.patch4.37 KBmortendk
#51 clean_up_css_in_the-1663210-50.patch3.15 KBmortendk
#50 user-permission-patched.png137.48 KBmortendk
#50 user-permission-notpatched.png135.93 KBmortendk
#50 clean_up_css_in_the-1663210-47.patch.txt3.11 KBmortendk
#48 password-confirm-match-seven.png673.6 KBmortendk
#47 password-confirm-match.png202.57 KBmortendk
#47 clean_up_css_in_the-1663210-47.patch3.11 KBmortendk
#42 interdiff-1663210-35-42.txt702 bytesrootwork
#42 user-admin-fix-css-1663210-42.patch1.55 KBrootwork
#41 clean_up_user_css-1663210-40.patch755 bytesManjit.Singh
#39 Screen Shot 2015-02-07 at 19.18.43.jpg90.55 KBLewisNyman
#35 user-admin-fix-css-1663210-35.patch1.09 KBAnonymous (not verified)
#31 user-admin-fix-css-1663210-31.patch1.11 KBrootwork
#27 user-admin-fix-css-1663210-24.patch1.26 KBVikas.Kumar
#26 user-admin-fix-css-1663211.patch395 bytesprajaankit
#20 user-admin-fix-css-1663210.patch396 bytesVikas.Kumar
#15 clean_up_user_css-1663210-15.patch1.72 KBalansaviolobo
#13 drupal-user-fix-css-1663210-12.patch1.16 KBrootwork
#9 user-fix-css-1663210-9.patch1.55 KBishmael-sanchez
#8 border-user-css.png38.03 KBishmael-sanchez
#2 1663210-2-cleanup-user-css.patch1.23 KBManuel Garcia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Manuel Garcia’s picture

The #permissions id is for the table, which is ok in my book to keep the namespace there.
Similar reasons go for the rest of ID warnings

Then comes the question of the warning of qualifiying an h2, which is not a valid warning, since you can have several h2's in a single page afaik.

/* Generated by user.module but used by profile.module: */
....
.profile h2 {
  border-bottom: 1px solid #ccc;
}

Apparently that's used by profile module which is no longer in core. Which then leads to the question of wether or not we shuould have that blob of css in user module. I've left it untouched since I think it's outside of the scope of this issue.

The attached patch fixes the rest.

Manuel Garcia’s picture

Status: Active » Needs review
sdmunoz’s picture

I tested and reviewed the changes and all is good. But would it be better to change #permissions id to a class name generated like?

.permissions-processed .module

thanks

ZenDoodles’s picture

Status: Needs review » Needs work

Yes.

-#permissions td.permission {
+#permissions .permission {

...fails csslint

ishmael-sanchez’s picture

Personally, I agree with @sdmunoz I like defining class names, especially in modules, so that it can be overridden without excessive CSS specificity later. Also by doing so, it reduces CSS lint warnings down to 3 from 8.

atu’s picture

We have a real problem with this declaration, because csslint doesn't allow to write padding, and border with width:

div.form-item div.password-suggestions {
  padding: 0.2em 0.5em;
  margin: 0.7em 0;
  width: 38.5em;
  border: 1px solid #b4b4b4;
}

How we can resolve it ?

ishmael-sanchez’s picture

FileSize
38.03 KB

@atu I think we can actually just remove that border rule for the following reasons

  1. It's just presentational, just adds a border to highlight info about the password (See screenshot)
  2. Presentational CSS shouldn't be in core modules (IMHO, let themes decide how to display it)
  3. At this point (during initial Drupal install), the seven theme is already loaded, so if the border is important we can add it back there (style.css)
ishmael-sanchez’s picture

Status: Needs work » Needs review
FileSize
1.55 KB

Ok I took a shot at this, removed the border (possibly add to Seven instead) and fixed some of the CSS specificity issues. I did leave a couple overqualified selectors in the user-rtl.css so if that's used it would trump the other definitions, but I have never worked on a RTL website so I wasn't 100% if that's how it works.

atu’s picture

@ishmael-sanchez Did you test your change ? because we have 2 tag with the same class 'password-confirmation', that's why it's precise with a div.

ishmael-sanchez’s picture

@atu yes I tested it and it seemed to work. However, it could use more testing.

atu’s picture

@ishmael Ok cool ;)

rootwork’s picture

Issue summary: View changes
FileSize
1.16 KB

This needed to be re-rolled.

rootwork’s picture

Issue tags: +SprintWeekend2014

Tagging from the global sprint.

alansaviolobo’s picture

rerolled

Status: Needs review » Needs work

The last submitted patch, 15: clean_up_user_css-1663210-15.patch, failed testing.

LewisNyman’s picture

Issue tags: -html5, -SprintWeekend2014 +frontend, +CSS
nitishchopra’s picture

patch #13 has worked fine

LewisNyman’s picture

Issue tags: +Needs reroll

Needs a reroll but I need to review this patch because there are more changes to make to fix everything.

Vikas.Kumar’s picture

Title: Clean up user css » User admin css
Assigned: Unassigned » Vikas.Kumar
Status: Needs work » Needs review
FileSize
396 bytes
Vikas.Kumar’s picture

Assigned: Vikas.Kumar » Unassigned

Needs review

LewisNyman’s picture

Status: Needs review » Needs work

Looks like the latest patch is missing most of the changes in #15

Anonymous’s picture

the reroll on #15 seems to be wrong.. that's the current head close to the current head(different line numbers just..) just and #20 is probably based on that, so we should really be rerolling #13..? but then again #13 is missing alot of stuff from #9.. is it just me or are you confused as well?

Vikas.Kumar’s picture

Vikas.Kumar’s picture

prajaankit’s picture

Status: Needs work » Needs review
FileSize
395 bytes

#21

Vikas.Kumar’s picture

rootwork’s picture

This looks good. I had a minor question about whether the .even and .odd classes were still being used (because we had removed them from D8 in favor of CSS3 pseudo-classes) but I went back and looked at the change record and we are still using those classes on table rows.

I'll circle back and do some screenshots unless someone else gets to it first today.

Status: Needs review » Needs work

The last submitted patch, 27: user-admin-fix-css-1663210-24.patch, failed testing.

rootwork’s picture

Status: Needs work » Needs review
FileSize
1.11 KB

Whoops -- turns out the .odd and .even classes were removed, because the patch no longer applied. Here's a reroll without them.

Status: Needs review » Needs work

The last submitted patch, 31: user-admin-fix-css-1663210-31.patch, failed testing.

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/user/css/user.admin.css
    @@ -3,18 +3,18 @@
     
    -#permissions td.module {
    +.permissions .module {
    

    Above here can we add a comment that this is for the permission page?

  2. +++ b/core/modules/user/css/user.admin.css
    @@ -3,18 +3,18 @@
    -#user-admin-settings .details-description {
    +.user-admin-settings .details-description {
    

    I think we can remove this styling. It seems weird to have this styling only for this page. This also changes the font size to be below 10px which is way too small

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.09 KB

1. Added comment
2. Removed the whole styling like you asked.

mortendk’s picture

Status: Needs review » Reviewed & tested by the community

looks good

idebr’s picture

Title: User admin css » Clean up css in the User module
Issue summary: View changes

Updated the title to reflect the contents of the patch and included a beta evaluation.

idebr’s picture

Issue tags: +frontend, +CSS
LewisNyman’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
90.55 KB

Csslint shows no errors for user.admin.css but there are still some errors in user.module.css

rootwork’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
702 bytes

I'm not sure why csslint doesn't complain about the other element names (input.password-confirm, div.password-confirm), but it doesn't, so here's an updated patch with the errors corrected.

Manjit.Singh’s picture

@lewis: I have remove errors that you have mention in screenshot. Please review patch.

rootwork’s picture

@Manjit.Singh I think maybe you just posted your interdiff? That patch doesn't have the original changes in it.

I'm re-posting my patches from #40 to run them through the tests.

Manjit.Singh’s picture

@rootwork: Actually i have updated issue queue at the same time when you were updated.

Thanks for update. :)

crazyrohila’s picture

Status: Needs review » Needs work
Issue tags: +#DCM2015

@rootwork: Still there is an overqualified selector. div.password-suggestions

rootwork’s picture

Status: Needs work » Needs review

@crazyrohila if you look at my comment in #40 you'll see I mentioned that (there are actually two). Csslint doesn't seem bothered by them, so I was waiting to see what Lewis thought.

@Manjit.Singh yes I saw that our posts crossed each other. But apart from that, the patch you uploaded only included the changes _you_ made, not the changes from the earlier patches. (Your own changes are usually included in an interdiff.)

Manjit.Singh’s picture

@rootwork Actually i was thinking, lets remove overqualified selector from input.password-confirm, div.password-confirm. Is it make sense.

mortendk’s picture

we have a class with the .password-confirm classes that are used both on an input & the div.
This patch:
* renames the information .password-confirm to .password-confirm-match in the user.js file, else we cant remove the input & div.
* seven had to get that fix in as well, why that css file is also patch
* adds a @file in the top of the user.module.css

attached the followup issue, to make the file naming correct (following the MAT) that should be worked on after the csslint is checked, to not create to much creep.

Screenshots added as well for the 2 places that are effected for the classrenaming

change of classname:

seven install

mortendk’s picture

Issue summary: View changes
FileSize
673.6 KB

attached missing screenshot from seven install with the new .password-confirm-match class

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/css/user.admin.css
@@ -3,18 +3,14 @@
-#permissions td.module {
+/* Permissions page */
+.permissions .module {
...
-#permissions td.permission {
+.permissions .permission {
...
-[dir="rtl"] #permissions td.permission {
+[dir="rtl"] .permissions .permission {

There is no .permissions class on the page let alone the same table the ID is on.

mortendk’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +csslint
FileSize
3.11 KB
135.93 KB
137.48 KB

@joel good catch have fixed it to select on the form instead of the table id
I have added screenshots to show that theres no regression

Patched:

Not patched:

mortendk’s picture

heres the correct patch :/
change the selector on the permission page to .user-admin-permissions .module

mortendk’s picture

*removed a double bracket
* added in permission class for the system page, so we the selector there.

screenshot for the permission page:

joelpittet’s picture

Status: Needs review » Needs work

Thanks @mortendk this is looking better:

  1. +++ b/core/modules/user/css/user.admin.css
    @@ -3,18 +3,14 @@
    +[dir="rtl"] ..permissions .permission {
    

    double dot.

  2. +++ b/core/modules/user/css/user.admin.css
    @@ -3,18 +3,14 @@
    -#user-admin-settings .details-description {
    -  font-size: 0.85em;
    -  padding-bottom: .5em;
    -}
    

    Likely need a screenshot for this.

  3. +++ b/core/modules/user/css/user.module.css
    @@ -1,5 +1,6 @@
    - * Password strength indicator.
    + * @file
    + * Basic styling for user module.
    

    Should the section header exist too for the password strength indicator? I'm curious because I don't know.

  4. +++ b/core/themes/seven/css/theme/install-page.css
    @@ -45,7 +45,7 @@
    -  div.password-confirm {
    +  div.password-confirm-match {
    

    Does this need div? That should be caught by linter, I'd assume.

mortendk’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

1. ups ..
2. user-admin-settings dont exits anywhere, so i guess its a leftover from way back.
3. yes it should have cause its a @file afaik
4. yes cleaned up in this patch

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
40.36 KB
+++ b/core/modules/user/css/user.admin.css
@@ -3,18 +3,14 @@
-
-#user-admin-settings .details-description {
-  font-size: 0.85em;
-  padding-bottom: .5em;
-}

/admin/config/people/accounts

account settings

+++ b/core/modules/user/css/user.module.css
@@ -1,5 +1,6 @@
- * Password strength indicator.
+ * @file
+ * Basic styling for user module.

Just thinking they are both needed? @file block on top and the password strength below it?

mortendk’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
180.65 KB
181.39 KB
4.55 KB

Ok added it back in & fixed the selector to be a class instead of the ID
with patch

without patch

joelpittet’s picture

+++ b/core/modules/user/css/user.module.css
@@ -1,4 +1,6 @@
+ * @file
+ * Basic styling for user module.
  * Password strength indicator.

Not what I meant but close:P Was thinking separate comment blocks. Sorry should've been explicit.

I'll leave this as NR, for a second pair of eyes but other than that I think it's RTBC.

mortendk’s picture

im gonna scope creep this issue a bit and fix the MAT file naming [#1887922] as well now where at it, instead of having it as an seperate issue (as noted earlier)

This patch now separate the visual styling of the password strenght generator.
No module.theme.css

with module.theme.css

Default gray tones are set for the strength indicator, to remove any "colors" (yes gray is a color, but now its clear for the themer that theres "no styling")

idebr’s picture

Issue tags: +Seven

Updated the tags with 'Seven', since it also touches seven css.

joelpittet’s picture

@mortendk aww, and I was about to RTBC it... scope--

Can you please put up an interdiff on that so I don't have to review from scratch?

mortendk’s picture

i know but it seemed silly not to get it all done with the filenameing - i totally scope creeped my self.

heres the interdiff

mortendk’s picture

FileSize
3.98 KB
idebr’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/css/user.theme.css
@@ -46,15 +54,12 @@ div.password-suggestions ul {
+
+/* Toolbar tab icon. */
+.toolbar-bar .toolbar-icon-user:before {
+  background-image: url(../../../misc/icons/bebebe/person.svg);
+}
+.toolbar-bar .toolbar-icon-user:active:before,
+.toolbar-bar .toolbar-icon-user.active:before {
+  background-image: url(../../../misc/icons/ffffff/person.svg);

This styling has been copied over from user.icons.css, but the original file is still there. In #2421373: rename *.icons.css files to *.icons.theme.css the proposed resolution is to rename user.icons.css to user.icons.theme.css, so the styling here should not be copied over.

mortendk’s picture

Status: Needs work » Needs review
FileSize
492 bytes
6.89 KB

removed the left over icons

idebr’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
595.23 KB
189.48 KB
564.99 KB
131.73 KB

All User module CSS files now pass CSSLint, the css selectors have been split into admin/module/theme.css and fixes a CSS selector that previously no longer applied (#permissions td.module).

Screenshots annotated with relevant classes:

idebr’s picture

Assigned: mortendk » Unassigned
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work on the thorough testing here.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 883f0e3 on 8.0.x
    Issue #1663210 by mortendk, rootwork, idebr, vks7056, ishmael-sanchez,...

Status: Fixed » Closed (fixed)

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