When there is not a full page of content, the footer will float up, leaving whitespace underneath the footer. See a screenshot: http://img.skitch.com/20100819-kuadnup6i3gr2bcfbi98acdq31.jpg
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | bartik-fix-background-min-height-min-width_2.patch | 1.95 KB | Jeff Burnz |
| #33 | bartik-min-height-page-wrapper.png | 322.16 KB | Jeff Burnz |
| #32 | bartik-fix-background-min-height-min-width.patch | 1.95 KB | Jeff Burnz |
| #18 | bartik-pseudo-sticky-footer_v2.patch | 1.74 KB | Jeff Burnz |
| #17 | bartik-pseudo-sticky-footer.patch | 1.89 KB | Jeff Burnz |
Comments
Comment #1
theresaanna commentedPatch adds the sticky footer method as outlined here: http://www.cssstickyfooter.com
The height of the footer is important in this method and since the footer is a fixed height, this patch includes jQuery to dynamically determine the footer height.
Comment #2
tim.plunkettSometimes those horrible visual bugs have easy solutions! Awesome.
There was some weird stuff going on with the jQuery. Also, if we want to find the height of #footer-wrapper, we have to move the padding to #footer-wrapper .section.
Comment #3
theresaanna commentedActually, in my testing it seemed as though the JS got the full height including padding as the final DOM element's height is used, not one applied in CSS.
What weird stuff?
Comment #4
tim.plunkettAt least in Firefox and Chrome (on a Mac), it's only counting the height, not padding. Firebug is showing "-moz-box-sizing: content-box;" (as opposed to padding-box), which backs that up.
With the jQuery, I literally have no idea what I was talking about, ignore that part.
Comment #5
davyvdb commentedI see two problems with the current implementation:
- If javascript is disabled, the footer is pushes more down. Not a huge issue, but still.
- If the toolbar is there, the sticky footer is too low. We should compensate for that (easily detectable by calulating the height of #toolbar).
To solve the first issue, it could be preferred to set the css in jquery. Based on another sticky footer method, I've written the following plugin once: http://www.drupalcoder.com/story/500-cross-browser-sticky-footer-with-fl.... The method used here is still better though since you don't the .scroll and .resize from my implementation.
Comment #6
tim.plunkettRe: #5
I agree with adding the stickyfooter-specific CSS in with jQuery, but I didn't implement that just yet, because I wanted to focus on the changes the jQuery.
The main issue is that with #page-wrapper {min-width: 100%;}, #page-wrapper is not aware of the height of #toolbar, since it's position: fixed. To get around this, I added a positive padding and negative margin to #page, which properly adjusts the height of #page-wrapper.
The big problem is over in #787940: Generic approach for position:fixed elements like Toolbar, tableHeader. This patch depends on the one being reviewed over there, which I'm working on next.
Relevant changes:
Comment #7
tim.plunkettForgot to set to needs review, and it technically isn't a bug, but is still visually important.
Comment #8
dcrocks commentedCan you show what it looks like after the patch?
Comment #9
kiphaas7 commentedYuck @javascript code, document.ready() and noConflict() is not necessary. Also, always provide context for your selectors. Handbook has a page for this:
http://drupal.org/node/756722
Also, cache your selectors if you use it more than once. This considerably speeds up javascript.
http://www.artzstudio.com/2009/04/jquery-performance-rules/
Also, chain your jQuery code.
But what you really should have done here is provide multiple arguments to the css() function.
Quick and dirty example (not tested, just as a code example):
Comment #10
Jeff Burnz commentedThe body is supposed to get the same background-color as the #footer-wrapper so no white shows - #781572: Whitespace below the footer when there is not enough content on the page
Comment #11
tim.plunkett@Kiphaas7, mind rolling a patch?
Comment #12
kiphaas7 commented#11, I never really took a good look at the issue itself, more at the code.
However, now that I'm thinking about it, this can't really be solved in an easy way. By fixating the height with jQuery, the patch effectively limits the capabilities of the footer. If you have dynamic content being loaded, the height of the footer needs to be adjusted as well, which the patch would prevent. (Either hiding the extra content or showing scrollbars on the footer: both ugly)
Off course, one can add an event listener for this (tricky in it's own way), but before you know it, a buttload of javascript is added for something that isn't crucial for the theme. Especially if, as #10 said, #781572: Whitespace below the footer when there is not enough content on the page lands. (Allthough that is marked duplicate right now, I would strongly recommend looking into that one again.)
So, as far as I know, there aren't any pure css solutions for a sticky footer with variable height. They all work with a fixed height. The proposed fix here tries to solve this by reading the height with jQuery (which incidentally should be done with outerHeight(false) to include padding and borders), but this also fixates the height, leaving dynamic (ajax) content to either set the footer height themselves or have an event listener that looks and/or responds for new content. That event listener is loaded with crossbrowser problems, as encountered in the overlay module, untill it was solved with a timeout checking the content height every 150ms. (Incidentally, a different solution for the height problem is used now in overlay, but totally not applicable for this problem.)
For a core theme, I wouldn't like such an over engineering solution for something like a sticky footer.
Different solutions:
I'd prefer 4 or 5.
Comment #13
Jeff Burnz commentedAgree with 12 here, over thinking and over engineering this would be a mistake - there are already treads open on this issue (white below the footer) so perhaps we should mark as a duplicate and discontinue work on a sticky footer approach (one with jQuery at least), as I very much doubt this will get broad support for our core default theme.
Comment #14
Jeff Burnz commentedOK, Jen has indicated she would prefer to go the sticky footer route rather than try to fix the problem the same way we did before (there are other issues etc).
I'm not exactly the worlds most foremost JavaScript geek but this is one I have used a few times - its principal weakness is that it doesn't work when the view port is re-sized, so if you change the size of the browser content can become obscured - this is the price to pay for something that is very simple. If the height of the page is taller than the window it does not kick in at all (in which case resizing, open ingcollapsed fieldsets etc is no problem), only if the page is shorter than the window does it kick in.
Comment #15
bleen commentedI'm very skeptical about getting this to work on a core theme. Some of the challenges I have seen with sticky footers in general (non of which would be acceptable in a core theme) include:
These are in no particular order and some of them are certainly easier than others, but I think they all need to be true before a sticky footer solution would be acceptable.
Comment #16
Jeff Burnz commentedYes bleen, totally bang on, its what ends up bloating the crap out scripts like this, the requirements to allow for browser re-sizing and things like collapsible fieldsets is hefty (in my experience anyway), which is why, in general, I would not really support this in core at all, even thogh I posted a simplistic solution, my true feeling is that this is all just a bad idea.
What if we just set a height on the #footer-wrapper so it always filled up the rest of the browser window (I mean with jQuery).
Comment #17
Jeff Burnz commentedSomething like this maybe?
What this does is set the height of the footer-wrapper so that it fills the rest of the window if the body is shorter than the window height.
Comment #18
Jeff Burnz commentedOh dear, that patch was not right, this is the one...
Comment #19
bleen commentedDoesnt work in this scenario:
Start with a small window (800x400)
Load page
maximize window (lets say to 1024x768)
Can we attach this behavior to $(window).resize()?
Comment #20
Jeff Burnz commentedYeah, I cant see why not bleen, but I'd really prefer to leave this one to the JS experts and not get to involved so I'm just trowing up ideas, that said I'll keep an eye on this as the last thing I would like to see is a monster bit of jQuery go in, trying to think kiss here.
Comment #21
kiphaas7 commentedPosted a patch implementing a solution like option 4 mentioned in #12 in #781572: Whitespace below the footer when there is not enough content on the page, since that feels like the more natural home for it.
Comment #22
damien tournoud commentedThe way we do that in other core themes is to set a sufficiently high min-height on the main content, and ignore both:
(1) ancient browsers
(2) people with really big screen sizes
Any reason not to do it the same way here?
Comment #23
Jeff Burnz commented@Damien - yes thats what we have already, the min-heigh is 500px, personally I have no problem with this. I am not a big fan of using JS for layout in our default core theme and would prefer we had sometime more akin what is going on in #781572 or a bigger min-height perhaps.
Comment #24
jensimmons commentedbtw, this issue is a dup of #781572: Whitespace below the footer when there is not enough content on the page
Comment #25
timmillwoodFiled a new comment on this subject on #781572: Whitespace below the footer when there is not enough content on the page
Comment #26
damien tournoud commentedNow a duplicate of the other, fixed issue.
Comment #27
jensimmons commentedTonight merlinofchaos was asking me in IRC why body has a dark grey color that's different than the white background of #page-wrapper. Apparently, that is causing problems in Panels.
Then later I was reading other issues:
http://drupal.org/node/937602 and
http://drupal.org/node/950986
where Jeff Burnz raises the possibility that maybe we really should do a stick footer, and help fix both of those issues.
Let's reopen this and see if solving the footer problem with a sticky footer would solve three other issues at once.
Comment #28
kiphaas7 commentedMy issues with a sticky footer, mentioned in comment #12, still stand. I don't see an easy way out of this, without adding loads of javascript.
Comment #29
bleen commented... and my comments in #15 still stand. I'm happy to review any solution someone comes up with here but my confidence level that we can come up with a cross-browser, accessible, persistant-when-browser-window-changes-size solution is very low...
Comment #30
Jeff Burnz commentedMy confidence is very low also, I am extremely doubthful it can be done with CSS only (because the height of the footer is dynamic), so it will require JS, and I have not used, seen or tried a JS solution that didn't have some issue.
FWIW I think we need to dig outside the square and come up with something rather ingenious and mind numbingly simple - something along the lines of http://drupal.org/node/950986#comment-3620764 gets us a long way there I think.
Comment #31
Jeff Burnz commentedI'm playing with this idea that sets the background color on #page-wrapper instead of the body, basically it sets 100% height on html, body and 100% min-height on #page-wrapper sort of thing, which works in all browsers except IE6 in which case the footer appears to float of the bottom (which it is anyway, the expanding footer is mere illusion).
This is building on the patch over in http://drupal.org/node/950986#comment-3620764
Needs some testing, although I think this is probably/maybe going to fix:
#950986: #main-wrapper min-height causes footer not to show without scrolling
#937602: Ugly black header background when horizontally scrolling
And this issue all in one go - fingers crossed... bar the IE6 issue, however the site/page is not broken in IE6, it just doesnt get so nice footer treatment, I can live with that :)
Comment #32
Jeff Burnz commentedOh yeah, need the patch...
Edit: the patch has a line of debug code in it, worry about that later, its testable as it is now.
Comment #33
Jeff Burnz commentedAdding a screenshot to show what the patch in #32 does.
Comment #35
bleen commentedok ... been playing with this for about 20 mins and so far so good. IE7 has an issue (although this might ba an ieTester issue)
http://skitch.com/bleen/d6h3x/windows-7-running
Tested on:
Chrome 9 OSX/Win7
Safari5 OSX
FF4 OSX
FF3.6 Win7
IE7/8/9 ieTester
IE8 Win7
As for IE6 ... I'm always ok with ignoring that old shrew of a browser
Comment #36
Jeff Burnz commentedHmmm, yes, IE7, I should have tested more (than 5 seconds), need to figure out something for that old dog also. I have more time tomorrow so will keep working on it, see if I can trigger the 100% height to kick in for IE7 (which will probably work for IE6 as well).
Comment #37
Jeff Burnz commentedCouldnt wait, this should fix the issue with IE7, but not IE6 (still don't think it matters for IE6, its not broken, just not as nice).
The small change is to add 100% height to #page as well which triggers it all into action.
Comment #39
kiphaas7 commentedI think the test failure is real, related to the color.module.
Comment #40
Jeff Burnz commented#37: bartik-fix-background-min-height-min-width_2.patch queued for re-testing.
Comment #42
tim.plunkettThis cannot be solved in this way. It is also too late to continue this discussion for D7. Jen asked that this issue be closed, and if someone has another approach it can be opened in a new issue for D8.
Comment #43
bleen commentedJen is the one who re-opened it ...
Also, as doubtful as I have been about this issue, I'm not sure why you are definitively saying that it cannot be fixed in this way. Can you please be more specific?
Comment #44
tim.plunkettSorry, I was closing issues rather rapidly and didn't expand much. jensimmons, webchick and I had a rather crazy sprint this weekend at BADCamp, and I was asked to close this by webchick, with Jen's assent.
We agreed that a sticky footer is desirable, but not possible in D7, and probably not possible in D8 with the work that was done here. If it is resolved, it will be a new technique that can handle something like a dragable/resizable textarea in the footer.
My instinct was to push to D8, but it does make some amount of sense to just start fresh if someone has an inspired approach.
Comment #45
Jeff Burnz commentedWTF?
You guys should actually spend 2 seconds to LOOK at the patch, this is NOT a sticky footer patch, this is very smart use of heights and widths to achieve a solution that has nothing to do with a sticky footer!
This is the "inspired approach" and is very smart indeed.
Comment #46
tim.plunkett2/3 of this patch is in core already. If the other 1/3 remaining isn't a sticky footer, than can you please either rename the issue or open a new one?
Perhaps a reroll, screenshots, or an explanation of what the patch accomplishes would shed some light on this.
Comment #47
Jeff Burnz commentedAlready well covered in 31 and 37, which raises a question - why are you closing issues having not even read through them?
I know half of this is in (I wrote the other patch), but not the other critical other half which switches around where the colors are added and the min height triggers.
Comment #48
Jeff Burnz commentedI am closing this, Tim is right for us to start another issue since this one is now too hard to follow for reviewers - see #988026: Move background color from body to #page-wrapper
Comment #49
ZionEgg commented#6: drupal-bartik-stickyfooter-888412-6.patch queued for re-testing.
Comment #50
damien tournoud commentedThis should be closed.