Remove #block-system-main dependency, fix font sizes, remove crufty CSS

this was (probaly) introduced in #516150: Add fallback for main content block rendering (before, we coudn't disable block module, or it just didn't work properly.)
This issue can be considered a follow-up on that issue.

Quick visual review, patch misses ie6.css changes, also might want to use div.admin (similar to garland)

This has been fixed in Seven: #563390-84: Seven theme lacks formatting on common html elements such as lists, paragraphs & <code>.

However, it is a major problem in Bartik. IMO #block-system-main should not exist in any of the selectors, and ID's should be avoided as much as possible in general. Changing component.

Setting to major, we need to get this sorted out.

Agree with Jacine about not using ID's as selectors where possible - Bartik does a lot of over specifying with selectors like #header #logo and and so on.

Couple of ideas here:

1) use the #content ID and adjust font sizes etc
2) add a DIV wrapper with a class around <?php print render($page['content']); ?>

I think I prefer falling back to #content and doing the hard work of reworking the CSS.

Thoughts?

I'm cool with a #content div and agree the font sizing needs work. The font sizes don't really alternates so much, except for maybe the footer, so there's probably no point in adjusting the size for it to target #content over just #main-wrapper, but you and I are on the same page, so I'm sure whatever you decide is fine.

I'm sorry I haven't had time to take any of this on, but I've been wanting to get rid of all these ID's for a while. Pretty sure 99% of them can easily go.

OK, sure, if someone wants to roll something up here I'd be glad, I'm pretty slammed with a pile of other patches in the queue for Bartik, otherwise I'll give it a couple of weeks until some of the other patches go in to save me re-rolling a heap of code (well at least until the two criticals get cleared away).

In #8, suggestions 1 and 2 are separate ideas, right? And I'm not clear which one you're cool with, Jacine :)

I'm willing to spend time on this, I'd just rather not do something completely different.

@tim, thanks for helping out :D

I think the main goal here is to get rid of CSS like this:

#block-system-main .node-teaser {
  border-bottom: 1px solid #d3d7d9;
  margin-bottom: 30px;
  padding-bottom: 15px;
}
#block-system-main .node-teaser .content {
  font-size: 92%;
  line-height: 1.65em;
  color: #3b3b3b;
  clear: none;
}
#block-system-main .meta {
  font-size: 80%;
  color: #68696b;
  margin-bottom: -5px;
}
...
#comments .submitted p {
  margin: 4px 0;
  font-size: 110%;
  line-height: 1.2em;
}
#comments .submitted .comment-time {
  font-size: 70%;
  color: #68696b;
}
#comments .content {
  font-size: 90%;
}

etc...

Those ID's need to go in favor of just .node x, .node-teaser x, .comment x. There is a lot of font-sizing and line-height stuff going on that appears to be unnecessary and in general, it's not really efficient. As the class targeting is fixed, I have a feeling the font-sizing issues are going to become more apparent, and these 2 issues will end up overlapping. Though, if you think it makes sense to work on that part in a separate issue, that's fine with me.

They layout #ids are also pretty confusing, with some targeting ids and then others targeted id .section to apply width: 960px; Example:

#header div.section,
#navigation div.section,
#featured div.section,
#messages,
#main,
#triptych,
#footer-columns,
#footer {
  width: 960px;
  margin-left: auto;
  margin-right: auto;
}

Seems to me a nice .container { width: 960px; ... } and a little more consistency would be a lot cleaner.

Oh, regarding the layout stuff that should happen here if people are willing: #883408: Remove redundant section classes and wrapper divs from bartik, so don't worry about it in this issue.

The issue here is that #block-system-main and even the region wrappers do not print when block module is disabled. Therefor if someone disables block module and uses Panels things will go horribly wrong. The ideas in #8 are not actually seperate - they both set out to fix this issue.

The font sizing issues I mentioned would be if you used #1, because this would directly impact on things like the tabs (for example they would get the font size 115% and be too big).

I think the whole idea of using #block-system-main in the first place was that sidebar blocks that contained a node or comment would not inherit the same styles as those in the main content column. However, has anyone stuck a node in a block and looked at it? I think that would be wrong thinking (thus I agree with Jacine that we should just use .node, .comment etc) and if sidebar nodes-in-blocks needed cleaning up then that should be an override that needs to be accounted for.

On the earlier issue of font-sizes, well, hrmmm, Bartik is very confusing with regards to font sizing - there are already issue when you use text re-sizing (in some browsers some text re-sizes disproportionately) and really, it needs a complete revision.

The inconsistent width wrappers are a bit of mess, agreed, a nice .container class set on those would clean that up (for the other issue as stated...).

So, from my hacking on this

1 - its not quite as simple to just remove #block-system-main as there are styles set on it that control stuff we need to keep

2 - we need to get rid of all the #block-system-main ID's because the design borks when block module is off

3 - we should get rid of as many ID selectors as possible - theres way too much over specification in Bartik

4 - cant be afraid to change things drastically, it just has to be done and made to work because this is a major bug

OK, so I have given this a crack to get this started, since its a major issue we really need to fix this.

I've used the #comment #content ID instead of block-system-main. I also did the hard work of re-working all the font-sizes in Bartik so we have something sane to work with - the previous font sizing was pretty magical and very hard to understand and led to some very whacky resizing issues.

Lets test this and get some reviews, its probably going to need work because the font sizing is so odd in Bartik - I keep loosing track of what is overriding what and so on. I tested a lot of it but not all so we do need to run this through the ringer.

For example we have font sizes that go like this: .8em > 115% > .75em

As much as possible I've tried to reduce that down to 87.5% > 0.929em etc, but some areas may still need some work such as the footer and special regions.

I'm going to close all the other font sizing issues and drag them into this one and just get the whole thing fixed in one go.

In case anyone is wondering WHY we need to review and fix font-sizes its because Bartik sets a font size on #block-system-main, but the next logical wrapper is #content which takes in more elements and we needed to fix those anyway, so we may as well just fix them all and get this done now.

EDIT - just changed the #comment to #content re the next comment, man I am too tired... lol

Your patch is right, but you said #comment in your writeup. You meant #content. I was REALLY confused at first :)

Is the idea on unitless line heights to use them wherever they might get inherited by a block-level element lower down, but to go with em-based line heights on "final destination" elements? Makes sense...

The code changes and underlying math looks good. However, the above comment confuses me. I was under the impression that line-height: 1.5em; and line-height: 1.5; are equivalent.

If so, shouldn't the rest of the line-heights lose their em's?

Funny, a co-worker just pointed this issue out to me a couple days ago. If you want a relative line-height to inherit the "common sense" way, then it's got to be unitless. But if it's not going to cascade down anyway, then being unitless doesn't help anything -- except filesize, I guess.

Long explanation:

http://meyerweb.com/eric/thoughts/2006/02/08/unitless-line-heights/

Demo:

https://developer.mozilla.org/en/CSS/line-height

Unitless line-height is everything to do with inheritance - Bartik makes the cardinal sin of setting a value based line-height on the body (1.4em). I was trying to remove as many of the value based line-heights as possible, they should probably all be unitless (for consistency) but elements with no descendants can have a value, aka what markabur says in #17. Feel free to reroll, this is pretty big change and needs a fair amount of testing - I'd be real happy if we can do this incrementally - if you see a problem please fix and re-roll :) E.g. I'm pretty sure I forgot to account for nested table font-size inheritance...

I re-rolled this to remove all the line-height values - also cleaned up some other minor alignment issues.

I can't see that this breaks anything at the moment, if we can get some cross browser testing done it would be good, however I think this needs to go in regardless of that testing being robust (very hard to do) - I do not say that very often but this must be fixed and soon - we cannot let Drupal core theme fall apart because the user wants to turn off block module.

Time for a title change, this only applies to Bartik atm.

Round 3.

- Remove a lot of ID selector prefixes such as #content and #header.
- Fix nested table font sizes.
- Move the IE hacks to ie.css (.meta margin).
- Cleanup line-height issue with tabs, they were getting too tall.

Please bear with the extent of this patch, trying not to kill kittens but so many things are interrelated it will be madness to do this incrementally in 20 - 30 one line patches.

Ok, I haven't reviewed this in too much detail, but there are a few things that I noticed:

- Fonts in the sidebar are bigger
- Node content font is smaller (maybe it's p tags somewhere, I didn't look too deeply).
- Tab line height is too small. I know you mentioned this in the other patch, but that's a design change and not the focus of this patch.

Screenshots are attached.

Will re-roll today ;)

I rerolled but in the middle of the fixes a patch got submitted that borked comment titles, this might not apply cleanly... lets see (I want to see if this actually passes).

In Bartik the teaser and full node content font sizes are different, so I adjusted to account for that + the other issues Jacine pointed out.

Again this is iterative patch, I know it needs more work to try and remove more ID selectors and any small adjustments, and I have not fully tested the extra regions at all.

OK, the new patch for comment permalink landed quickly so I have re-rolled and passed this on my local machine - go bot go...

Sneaking another one in here as asked to by Jeff Burnz here #847118: Two tweaks needed for using Bartik as admin theme

- Firefox renders font sizes in input and textarea elements differently due to its internal stylesheet, so we need to set them explicitly in the theme (line 519-525 in the patch)

- IE (below 8) has its font-family for input elements set to sans-serif in ie.css, which also looks ugly when on a node or comment form. Removing this in lines 12-14 of the patch.

Otherwise no changes, except for a reroll against current HEAD.

Hm... both patches in #28 and #29 result in the vertical tabs being broken in IE6 and IE7 (screen attached). The last patch that didn't have this effect was #23.

back to needs work...

Iterative patch building on #29.

- removes more ID's
- changes many selectors to be less specific (e.g. changes div.class to just .class)
- removes some redundant legacy CSS

The next move I think is to take a close look at the extra regions and see what needs to be cleaned up there.

I'm not seeing that issue in IE6 or 7 -can you debug on your end to see what might be causing this?

Jeff: I can't see it anymore either - must have been a fluke, sorry.

By what standard are we to RTBC these patches? I can't find a clear description of what this issue is about anymore. It started out with some bug on the /admin page and has now (since #15 I think) morphed into something much wider concerning overall font-size handling.

Can you give an indication as to which areas in Bartik need special review after applying these patches? And maybe change the title of this issue, too?

OK, let me sum up what is going on here:

1) The main point of the patch is to remove all the #block-system-main dependencies in the CSS. We have to do this because Block module is optional in D7. This created an immediate new issue that many font sizes relied on a font-size reset Bartik was using on #block-system-main (was 115%) - so we have to fix the font-sizing.

2) Remove as much as possible Bartiks reliance on ID selectors. There is a lot of over specification in Bartiks CSS.

3) Font sizing issues - because of (1) we need to fix many font sizes - however if we simply fixed the bare minimum its a band-aid fix, what we need to do is fundamentally change the way font-sizes work in Bartik because there are resizing/scaling issues with font sizes and line heights all through Bartik. In short I munged two issues into one because to me it made sense to just nail it all in one hit.

The "test" is - does it look the same as before the patch - also you should switch OFF block module and check things.

I have changed the title, not sure how good it is, lol.

fixing typo

I have re-rolled to take account of the extra regions - Featured, Triptych and Footer columns.

I think this is ready, I've done a lot of testing and while there are very subtle line-height differences in some places its very similar. I can't pick out any glaring differences at the moment.

One point - you may think that some fonts are fractionally different in size - this is true because they are. This patch, as much as possible, has done away with the fractional computed font sizes - so we no longer have mad font sizes like 11.9833px, instead we have 12px. I have checked most of the computed values (although not all), and I have attempted as much as possible to use whole pixel values - the reason is cross browser consistency of font size.

In some places such as the sidebar block content and node/comment links I have used fractional values because I do not want to change the design in these places, where we can get away with a +/-0.2px in some places these font sizes must be the same (as the original), because in relation to the other elements they don't have the same "design effect" if we round them to an exact pixel value.

BTW - the way to really check this stuff is to look at the computed values for font sizes and line heights in Firebug (most browsers have some kind of inspector that shows you the computed font size).

I tested this too and found no problems in FF/WIN/Mac, Chrome/Mac, IE7, Opera/Win/Mac - only one:

- In FF/Mac the search button now wraps under the search input field. This is addressed by #923242: Search button has no hover state however, which applies together with this one.
- The patch in #876960: Comment form needs work for IE6 and 7 conflicts with this one, so I have set it to postponed until this here lands.

Big++ to get this in quickly so we can also commit the other issues which already are RTBC.

One cosmetic issue:

The font-size in contextual links scales with the base font-size of the region they are in. It becomes very large in the #featured region for example. Screen attached.

Good spotting - I have added an font-size insulator (font-size: small) and adjusted it down with a relative value. This is the exact same fix we used for the Toolbar module and should probably go into contextual.css rather than here, however we can remove it later if I can get the fix into the core module.

Well then... RTBC.

Let's get this in!!

+++ themes/bartik/css/style.css	28 Sep 2010 13:57:29 -0000
@@ -1075,17 +1089,22 @@
+.form-type-radio label,
+.form-type-checkbox label {
+  margin-left: 4px;
+}
+.form-type-radio .description,
+.form-type-checkbox .description {
+  margin-left: 2px;
+}

Would this need a corresponding RTL style? If not, then back to RTBC and sorry for the hold up.

Powered by Dreditor.

Let's not hold this up with very minor cosmetic issues, sorry. Let's file RTL issues separately. For an overview of RTL-issues: http://drupal.org/project/issues/search/drupal?text=RTL&assigned=&submit...

IMHO, this needs to go in quickly to give us an even remote chance of fixing all the minor, normal and major other bugs in Bartik. At the moment there are really a lot of open issues whose patches conflict with each other and we need some kind of order restored. Otherwise no patch will ever apply and Jeff et.al. spend all their days rerolling patches.

For what this issue is about, see #35:
- remove all the #block-system-main dependencies in the CSS
- Remove as much as possible Bartiks reliance on ID selectors
- Font sizing issues

This patch fixes these.

lol, for what its worth I already spend all day re-rolling patches for Drupal 7 ;) bring on GIT!!!

Ok, I just did another round of testing on this and I agree it's ready!

Even the #preview stuff looks good. Awesome job Jeff.

Just to say the patch worked for me as well, when tested against the criteria in #35.

The only tiny issue I found with font size was that text in a code tag nested in an element with a class of 'description' shrunk very small in Firefox 3.6 (not in IE8). The only place I found this was in Administer » Configuration » Search and metadata » Clean urls. It seems that Firefox is making it 0.85em of 0.85em. But it's not a deal-breaker and everything else seems to be fine.

@46 - good spotting, what is happening is that Firefox is automagicially applying the default monospace font to code tags. In Firefox this is Courier New 13px, if you change that default to 16px it will be right (not the fix, just saying).

This should be addressed in #827528: No styling for PRE, CODE tags in Bartik

I confess that I have no idea what most of this is doing, but it has sign-off from all the right people, and it looks like a PITA to re-roll. :P

Committed to HEAD.

OMG - than-you, thank-you, thank-you !!! Everyone group hug, now we can move on to the final push and attack those majors.