WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13636 closed defect (bug) (fixed)

twentyten top margin css fix

Reported by: Ornani Owned by: iammattthomas
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

While creating a new twentyten child theme for a client, I had to override the header.php file to add some stuff to the top of the page. Until now - everything is cool.

I encountered many CSS issues which all disappeared the moment I found out how the top 20px margin is applied.

The most intuitive thing is to think that the #wrapper div has a "margin-top:20px". This is not the case. The margin is applied on the #header div. It took me quite a while to figure this one out.

I don't know why this has been done, but I know it's going to confuse and cause problems for developers.

Also, IE6 and IE7 are not implementing the top margin property when assigned the current way, and using my solution, it works.

My suggestion is to move the margin-top property from the #header div to the #wrapper div.

Thank you for doing your awesome work with WordPress, Oren.

Attachments (1)

13636.diff (503 bytes) - added by markjaquith 8 years ago.

Download all attachments as: .zip

Change History (16)

@markjaquith
8 years ago

#1 follow-up: @markjaquith
8 years ago

Patch as described. Theme CSS isn't really my area. Can someone more CSS-y weigh in?

#2 @Ornani
8 years ago

Wow that was freaking fast, thanks!

#3 @ocean90
8 years ago

  • Keywords has-patch added; margin-top margin css twentyten ie ie6 ie7 removed
  • Owner set to iammattthomas
  • Status changed from new to reviewing

Patch looks good for me.

#4 @nacin
8 years ago

I agree that the patch results in better markup that is more clear. That said, does this affect existing child themes too much? Is it really worth it in that case? Anyone designing a child theme could find the 'offending' CSS quite easily, either in style.css or by using a tool such as Firebug.

I would opt for a commit, personally, but playing devil's advocate here.

#5 @Ornani
8 years ago

nacin - I agree with you. But... I used firebug and had a difficult time finding it, and it's not my first time using it. Also, this fixes it for IE6-7. The current theme doesn't have that top margin in IE6-7.

This does raise a small dilemma regarding the existing twentyten child themes, but I don't think this will break something.

#6 @mrgtb
8 years ago

I have a comment to make about this margin-top 20px used for both the Header and Footer. If you use the "Background Image" tool in WordPress 3 to add a background that scrolls vertically, and centred. Such as a 1px height by 980px wide image to add a shadow effect around your main content page. What happens is the margin-top 20px (Header) - margin-bottom: 20px (Footer). Also effects your background image.

Take a look at my Blog and note the background shadow image I used. But to make it scroll vertically correctly all the way to the (top and bottom). I had change those two margin-bottom: and margin-top 20px settings for Header and Footer to this: (margin; 0;)

Link: http://mrgtb.com

If I don't do that, the margin space effects the background image scrolling virtically using the WP background image tool.

#7 @mrgtb
8 years ago

Also read this article I wrote about it for more details: http://mrgtb.com/twenty-ten-wordpress-3-theme-tip-using-backgrounds-492/

#8 @Ornani
8 years ago

mrgtb: I tried using your background image on my fresh install and it looks okay: http://i49.tinypic.com/1i26gw.jpg

What problem did you encounter?

#9 @mrgtb
8 years ago

The problem I had, was the background image stopped scrolling where the margin-top:20px (Header) and margin-bottom: 20px (Footer) started. Have made made the patch changes here to you site?

Also, check your (404 Page) and see if you have problems there on that page with the background.

#10 @mrgtb
8 years ago

Sorry, it may have been the 404 Error Page WordPress 3 displays where I spotted the problem, and NOT on the Blog pages (where it might have looked OK). You need to see this problem happen by visiting a dead link to your site that will force WordPress 3 to show you it's 404 Error page.

#11 @Ornani
8 years ago

No, this installation is not patched. The 404 page looks the same.

Weird.

BTW: I also have a black cat :)

#12 @mrgtb
8 years ago

Hmmm, very odd. Because it was bugging the life out of me, no pun intended. And that's how I solved the problem by getting rid of that margin 20px space used for the Footer and Header. Maybe this might have something to so with you use RTL mode (unlike my site)? Not sure why it's not happening with you at all, that's all I can think off?

Thanks, that picture you see as my logo - is my Black Cat I took. ;)

#13 @Ornani
8 years ago

No, that's not the RTL.

Well this is too off-topic I think. Let's focus at the ticket itself.

#14 @automattor
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

(In [15092]) In Twenty Ten, move the top margin from the header to the wrapper div. Fixes #13636. Props markjaquith.

#15 in reply to: ↑ 1 @EMG
8 years ago

Replying to markjaquith:

Patch as described. Theme CSS isn't really my area. Can someone more CSS-y weigh in?

Heya Mark and everyone! :)

This is coming in 3 months late, but I thought I would chime in with an explanation.

The reason for the 'weird' 20px margin issue for both the top and bottom of the wrapper is because of a bug in CSS rendering related to the accidental 'doubling' and/or 'pushing out' of margins when two margins are placed together from two separate adjoining containers (0px included) without any padding or borders separating them.

For whatever odd reason, the top and bottom margins - applied to the header and footer respectively in this case - also got applied to the container that contained it which is the wrapper. The result (before the header/margin-top fix) is either a header and footer with 20px of marginal space and the wrapper having the same 20px marginal space or just the wrapper having the 20px marginal space and not the header and footer.

Currently on my version of Twenty Ten, I had the issue where my wrapper refused to touch the bottom of the screen and that's because of the 20px margin specification the footer has. Instead of visually margining a 20px space between the end of the wrapper and the footer div, it visually margined it after the wrapper. Depending on how you see it, the wrapper either got cut off before the footer's margin was applied or the margin 'pushed out' of the footer area and got applied to the wrapper.

A fix for that would be to add a 1px padding-bottom to the wrapper div and give the footer div a 19px margin.

Compare the before and after fix and you will see what the doubling or pushing out bug does.

This well-known CSS bug is also related to the equally weird collapsing margins bug where margins placed together with no borders or padding separating them actually collapse on each other.

Let's say you specified 20px margin-bottom with the block-top AND 20px margin-top with block-bottom and no padding or borders between them. The result would more than likely be a collapsed (in this case halved) margin of 20px instead.

Note: See TracTickets for help on using tickets.