You are here

Tame seven's reset.css

resetting margin/padding/border to 0 on 023 strength selectors is really painful, we don't need to override system.css twice, we can just alter it out now with hook_css_alter() and maybe copy over the few bits we're actually keeping

I've had a crack at this. I did not look at the previous patches at all and simply started by removing the entire reset to see what happened - upshot, not a lot.

The way I see this is that the reset is doing a lot of what I might consider "front end" resetting - blockquote, ins, del etc etc - this has no place in Seven, its an admin theme, using this for a front end theme is not our consideration, and some of those will affect perhaps one browser, like Safari, now we're in real edge case territory.

I'd ask this to be taken quite seriously, it removes a tonne of what I think is just pure cruft from the reset, and after a fair few hours I can't see any major regressions. yes I had to add back in some stuff, but IMO those styles are going where they should be, applied to the specific selectors and elements.

I have only tested in FF and IE8, however I will continue testing in IE*, Opera and so on, but would be great to get some feedback, would really like the smoke this out of Seven once and forever.

If you think I'm nuts tell me so, I tend to go off the rails sometimes...

While this patch looks nice, this seems like polish that's going to require extensive testing to ensure we didn't break anything, and likely a flurry of follow-up patches afterwards when we inevitably do.

I think this is D8 at this point, unless you disagree with this assessment.

@webchick, I agree - I was throwing this out there to show that it can be done, but I would rather play it safe given the stage we're in - its a very difficult patch to review, too hard for right now imo.

Lets bump it to D8 and come back to it really early in the cycle so we have a long time to account for any regressions.

Mike - this will be very time consuming to review, in essence its one huge before/after comparison, both visual and calculated values. Of course that's unlikely to ever happen (to big task) so we'd go with a "good enough" at some point and clean up regressions.

however those seem more like feature requests/additions, rather than a strait out fix for this issue.

I'm totally fine with going forward with the approach in the current patch in this issue. I just wanted to gauge interest in Normalize. I don't want to bikeshed this issue; we can move Normalize to another issue.

I can't evaluate normalize from a technical POV, but it sounds like it would let me as a non-CSS guru start with something sensible and work from there, rather than start from _nothing_ and have to build up from there. +100 to that.

That's exactly what it tries to do, instead of trying to reset everything to 0, it normalizes things across the various browsers.
You can easily try it out by visiting http://necolas.github.com/normalize.css/demo.html in different browsers and comparing the results.
The only discrepancies you might see are things the browser simply doesn't support (like range inputs, inline SVG, etc.).

I'm about to start modifying Seven's style.css to support http://drupal.org/node/1510532 and want to work on top of a reset that I know is stable. I'd like to resolve this issue either beforehand, or in parallel. I'm fine with either the patch re-rolled in #16 or with normalize.css (which I have been using in production for a while). I'm seeking some consensus, judged pretty much by how far we can get before feature freeze.

Attaching updated patch which brings in some changes from http://drupal.org/node/1747868 (especially loading the reset first, before all other stylesheets). Also updates normalize (which this basically is) to the current 2.0.1 version. This no longer supports IE 6/7 which is consistent with release support for D8.

Images attached of screenshots in Chrome (latest stable, Mac). A few representative screens are attached directly, the rest zipped. I can provide screens from other browsers if necessary, just not right this moment.

Screens show obvious regressions only with the secondary tabs. Not sure what to do with regressions – should they be fixed in this patch?

Is it intentional to have extra borders around the secondary tab buttons? Currently in d7 and d8, Seven doesn't have those. Also, the active button in the screenshots above looks incorrect, with that fat bottom border.

Let me also amend that we still have ~1 year to fix any possible glitches and flaws in themes.

Contrary to that, this change here can be considered to be bound to feature freeze, since it essentially is code thaw material, as it revamps the fundamental styling (reset) of Seven theme.

Let's make sure to get these kind of changes into core before feature freeze (Dec 1st). After that, there's little hope/chances for getting such changes in, and our work will have to focus on correcting (NOT: improving) the front-end experience where needed.

That's an important differentiation to make, and we apparently do not have clearly documented guidelines for theme changes/improvements with regard to feature freeze and code freeze. Thus, from my perspective, this is the most reasonable and sensible interpretation that could be applied; i.e., changes/improvements before feature freeze, only fixes are allowed after.

So let's bear in mind that we have almost an entire year for polishing of theme stuff.

So, being relatively new to Drupal core development, I kept thinking: "normalize should be available to all themes", but I did not know how. The latest patch does it. Thanks @sun!

The details of the changes in #45 are:
- Add the complete, unaltered normalize.css library to /core/misc/normalize/ just like any other library. The CSS itself is the same as in all patches since #30.
- Delete core/themes/seven/reset.css
- Remove mention of reset.css from Seven's theme.info file.
- Register the normalize library in system.module (system_library_info()) with a weight of -10.
- Actually add the library's css to the Seven theme (Seven's template.php)

Normalize itself has been carefully written, tested and validated by the front-end community at large. I've been using it as my reset with Drupal for a year now.

As far as the supporting code for Drupal, everything works correctly in my testing with Normalize added first, before all other css including system.css, and only in Seven. The Seven theme continues to show no visual regressions. The patch as a whole solves the heavy-handedness of Seven's original reset and creates a great asset for other themes at the same time.

Disclaimer: I was not familiar with the process of adding external libraries in module code, but after consulting the docs for the few lines of code I needed to check, this looks good. Leaving as RTBC.

I would much prefer to add normalize.css before every other CSS file at a system module level. (Using CSS_SYSTEM with a follow-up to change CSS_SYSTEM to CSS_MODULE with a negative weight.) In other words, normalize.css is added by the system and used (by default) by all themes, not just Seven.

What do people think of that idea? And do we want to do that now or as a follow-up?

@JohnAlbin: Definitely a separate follow-up issue. (And I will object to that. ;))

Also, to clarify on CSS_SYSTEM, I explicitly tested that normalize.css is in fact output as the very first library on the page. It is output before System module's base CSS files, but in the same group. If aggregation is enabled, those files are bundled into a single aggregate.

Committed and pushed to 8.x. This was ry5n's first core patch. :) Let's all give him high fives at BADCamp! :D

We should get a change notice that normalize.css is now available to theme authors. I believe I also land on this being "opt-in" since there could be other similar libraries that pop up over the next 3-5 years, and that would allow themers to choose their solution without undoing core assumptions.

@tim.plunkett Oh, crap. I clearly missed some important screens in testing. I did test (Chrome and IE9, screenshots above), so I am not sure how I missed global form regressions… But that's beside the point. I apologize for the mess.

I will post a patch that puts back the old reset, leaving Normalize in. I'll then start work on fixing the CSS in the UIs above so we can switch back to using Normalize. Does that sound good?

@ry5n, yes that sounds perfect.
Sorry for my terse post, it was from my phone.
By "until this is actually tested" I didn't mean to imply that this wasn't tested at all, but rather more fully on some more complex screens.

The main thing I noticed was that .form-item, especially radio buttons, went from having 0px padding to 9px or more. The other thing was weird borders on pages with tabledrag (Menu, Block).

This patch restores Seven's previous reset.css and switches back to using it instead of Normalize, leaving the library itself in Core. I will start work right away on fixing visual regressions so that we can use Normalize in Seven as soon as possible.

@tim.plunkett I checked the before/after on the core commit before I posted #56, and the Blocks UI does get a bit worse. I missed it in testing. I didn't immediately see any other regressions (in forms, Views or Menu UI), but I figured your suggestion was a wise idea. What do you think? Should we still apply #59, or just do a thorough re-test and fix stuff in follow-up?