Posted by ZenDoodles on June 27, 2012 at 4:23pm
17 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | contextual.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
| Issue tags: | html5, Needs manual testing, Needs screenshot, Novice |
Issue Summary
Sub-issue of #1190252: Meta: 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.
- Copy and paste the the stylesheet(s) below into the css lint tool at http://csslint.net and test.
- Fix any warnings or errors the tool finds.
- Patch Drupal 8 locally and make sure the css changes have not broken anything visually.
- Create patch and upload for the testbot.
Files: modules/contextual/contextual.css and contextual-rtl.css
Comments
#1
https://gist.github.com/3006391#L5799
#2
I added some webkit border radiuses to fix some warnings. Here's the remaining css warnings.
csslint: No errors in contextual.base-rtl.css.
csslint: No errors in contextual.base.css.
csslint: No errors in contextual.theme-rtl.css.
csslint: There are 4 problems in contextual.theme.css.
contextual.theme.css
1: warning at line 22, col 1
Outlines should only be modified using :focus.
.contextual .trigger {
contextual.theme.css
2: warning at line 24, col 3
Using height with border can sometimes make elements larger than you expect.
border: 1px solid transparent;
contextual.theme.css
3: warning at line 24, col 3
Using width with border can sometimes make elements larger than you expect.
border: 1px solid transparent;
contextual.theme.css
4: warning at line 29, col 3
Using width with padding can sometimes make elements larger than you expect.
padding: 0 2px;
#3
#4
#1290506: Remove webkit-specific border radius from CSS
#6
#2: drupal-1662968-fix_contextual_css-2.patch queued for re-testing.
#7
I applied the patch in #2 to confirm that it was working and removed the warnings at csslint.com for all files except contextual.theme.css. I also tried resolving the remaining four warnings but only ended up introducing new issues. I would suggest this is RTBC since the remaining four warnings are not critical errors we're trying to address per the parent ticket (#1190252: Meta: Use csslint as a weapon to beat the crappy CSS out of Drupal core). The specific region these issues are referring to (.contextual .trigger) is limited enough that the box warnings shouldn't be a problem.
#8
see #4. we don't need it for CORE.
#9
There were actually more issues. Most were related to using width/height and border/padding. Using box-sizing actually makes csslint aware that you know what you're doing, so I've ported some of the changes.
There's an issue with ouline: none, though. It is awful for accessbility (see https://github.com/stubbornella/csslint/wiki/Disallow-outline%3Anone), but it is ugly when applied to stuff that already have a border. I've left the outline enabled for the focused unactive trigger, but once it is active I still get rid of it. Couldn't think of a better solution, so this is what we've got right now. Original implementation didn't show any visual cues of focus to the trigger widget.
#10
Here are some screenshots.
#11
oh wrong patch
#12
#11: csslint-contextual-1662968-9.patch queued for re-testing.
#13
The last submitted patch, csslint-contextual-1662968-9.patch, failed testing.
#14
CSS patch meets style guidelines, applied patch, patch failed, no longer applies. reroll to head.
#15
rerolled to head
#16
I can't add (or don't want to add) anything else to the current (as of patch #9) visual cues of the contextual widget. I'd like to RTBC this, but it is a reroll of my own patch :/
#17
Wish we could get rid of the vendor prefix for FF, but it looks like there is no unprefixed version yet (http://caniuse.com/css3-boxsizing) so for now the patch applies and works correctly.
#18
csslint, on my machine, warns that webkit also needs the vendor prefix. caniuse.com tells that chrome no longer needs, but iphone/android does. Thus I'm fixing that.
re-running csslint also tells that box-sizing doesn't work on ie6/7 (we no longer care) and that "Outlines shouldn't be hidden unless other visual changes are made." which is already addressed, although csslint is unaware of that.
So I hope no one minds if I add the -webkit prefix and keep this as RTBC since I don't break anything. Really.
#19
CSS changes need manual testing in all browsers and all affected core themes:
Post before-and-after screenshots for one browser/theme.
#20
#21
Hey everyone, reworked this patch slightly to reflect the new structure, ran it through Lint and the only errors were the IE ones mentioned above and it complained about the use of !important, but now knowing enough about this, maybe there's a reason for that.
It all looks fine visually to.
#22
Yay, thanks for picking this up! :D However, you shouldn't mark your own patches RTBC, so you can give someone else the chance to just look it over first and make sure it's cool.
Looks like this could use some more eyeballs from some front-end folks. I was curious about the vendor prefixes on those properties, but John/Morten seemed to think they were ok.
#23
+++ b/core/modules/contextual/contextual.theme.cssundefined@@ -23,18 +23,23 @@
+ border-radius: 4px 4px 0 0; ¶
...
+ -webkit-box-sizing: border-box; ¶
traling spaces
+++ b/core/modules/contextual/contextual.theme.cssundefined@@ -23,18 +23,23 @@
+ box-sizing: border-box;
why add box-sizing ?
+++ b/core/modules/contextual/contextual.theme.cssundefined@@ -23,18 +23,23 @@
+ border-bottom: none;
why add this one.
#24
I think most of this work was addressed in #849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers.