Comments

theresaanna’s picture

Status: Active » Patch (to be ported)
StatusFileSize
new9.17 KB

Patch 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.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new9.09 KB

Sometimes 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.

theresaanna’s picture

Actually, 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?

tim.plunkett’s picture

At 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.

davyvdb’s picture

Status: Needs review » Needs work

I 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.

tim.plunkett’s picture

StatusFileSize
new9.25 KB

Re: #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:

jQuery.noConflict();
jQuery(document).ready(function() {
  var height = jQuery('#footer-wrapper').height();
  
  var toolbar = jQuery('#toolbar').height();

  jQuery('#footer-wrapper').css('height', height + 'px');
  jQuery('#footer-wrapper').css('margin-top', '-' + height + 'px');

  jQuery('#page').css('padding-bottom', height + 'px');
  jQuery('#page').css('padding-top', toolbar + 'px');
  jQuery('#page').css('margin-top', '-' + toolbar + 'px');
});
tim.plunkett’s picture

Title: Footer does not stick to the bottom of the page » Make footer stick to the bottom of the page
Category: bug » feature
Status: Needs work » Needs review

Forgot to set to needs review, and it technically isn't a bug, but is still visually important.

dcrocks’s picture

Can you show what it looks like after the patch?

kiphaas7’s picture

Status: Needs review » Needs work

Yuck @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.

// Instead of:
$('#test').addClass('foo');
$('#test').removeClass('foo2');

// Do this:
$('#test').addClass('foo').removeClass('foo2');

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):

(function ($) {

  Drupal.behaviors.bartikStickyFooter = {
    attach: function(context, settings) {
      var $footerWrapper = $('#footer-wrapper', context);

      var footerHeight = $footerWrapper.height();
      var toolbarHeight = jQuery('#toolbar', context).height();

      $footerWrapper.css({
        height: footerHeight + 'px',
        marginTop: - footerHeight + 'px'
      });

      $('#page', context).css({
        paddingBottom: footerHeight + 'px',
        paddingTop: toolbarHeight + 'px',
        marginTop: - toolbarHeight + 'px'
      });
    }
  };

})(jQuery);
Jeff Burnz’s picture

The 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

tim.plunkett’s picture

@Kiphaas7, mind rolling a patch?

kiphaas7’s picture

#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:

  1. Do nothing. Unacceptable because there is white space below the footer with short content height.
  2. Apply a sticky footer solution with a fixed height in css, which is on domready updated with jQuery to adapt to the actual, current height (progressive enhancement!). Any height changes later on for the footer due for example ajax content, or even simple show/hide functionality is not accounted for. (essentially the current attached patches)
  3. As 2, but with an event listener to update height of footer. Will cost system resources, and will never be as fluid/smooth as a div without a fixed height.
  4. Add a global site wrapper div that uses the same color as the footer, making the footer not sticky, but does fix the white block issue. A global site wrapper div because jensimmons mentioned that just setting the color on the body isn't a good idea for wysiwyg editors in #781572: Whitespace below the footer when there is not enough content on the page. Essentially the patches proposed in that same issue.
  5. Like 4, but with an addition of a carefully selected min-height on the content div. Carefully selected because it shouldn't become annoying with different resolutions and browser resizing.
  6. ... Something else?

I'd prefer 4 or 5.

Jeff Burnz’s picture

Agree 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.

Jeff Burnz’s picture

Status: Needs work » Needs review
StatusFileSize
new1.89 KB

OK, 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.

bleen’s picture

I'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:

  • Dynamic height of footer - this could change on a per page basis or even client side via javascript. In both scenarios, the footer would need to remain sticky (and in the correct position - e.g. not half way off the page, or 20px from the bottom for no reason.)
  • Dynamic height of other page elements - changes to other elements on the page cant break the stickiness or mis-position the footer
  • Footer still needs to work if JS is turned off
  • Footer needs to stay sticky if the browser window changes size
  • Sticky js cant conflict with other js on the page

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.

Jeff Burnz’s picture

Yes 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).

Jeff Burnz’s picture

StatusFileSize
new1.89 KB

Something 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.

Jeff Burnz’s picture

StatusFileSize
new1.74 KB

Oh dear, that patch was not right, this is the one...

bleen’s picture

Status: Needs review » Needs work

Doesnt 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()?

Jeff Burnz’s picture

Yeah, 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.

kiphaas7’s picture

Posted 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.

damien tournoud’s picture

The 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?

Jeff Burnz’s picture

@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.

jensimmons’s picture

timmillwood’s picture

damien tournoud’s picture

Status: Needs work » Closed (duplicate)

Now a duplicate of the other, fixed issue.

jensimmons’s picture

Status: Closed (duplicate) » Active

Tonight 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.

kiphaas7’s picture

My 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.

bleen’s picture

... 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...

Jeff Burnz’s picture

My 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.

Jeff Burnz’s picture

I'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 :)

Jeff Burnz’s picture

Status: Active » Needs review
StatusFileSize
new1.95 KB

Oh 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.

Jeff Burnz’s picture

StatusFileSize
new322.16 KB

Adding a screenshot to show what the patch in #32 does.

Status: Needs review » Needs work

The last submitted patch, bartik-fix-background-min-height-min-width.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review

ok ... 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

Jeff Burnz’s picture

Hmmm, 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).

Jeff Burnz’s picture

Couldnt 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.

Status: Needs review » Needs work

The last submitted patch, bartik-fix-background-min-height-min-width_2.patch, failed testing.

kiphaas7’s picture

I think the test failure is real, related to the color.module.

Jeff Burnz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, bartik-fix-background-min-height-min-width_2.patch, failed testing.

tim.plunkett’s picture

Assigned: theresaanna » Unassigned
Status: Needs work » Closed (won't fix)

This 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.

bleen’s picture

Status: Closed (won't fix) » Needs work

Jen 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?

tim.plunkett’s picture

Status: Needs work » Closed (won't fix)

Sorry, 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.

Jeff Burnz’s picture

Status: Closed (won't fix) » Needs work

WTF?

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.

tim.plunkett’s picture

2/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.

Jeff Burnz’s picture

Already 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.

Jeff Burnz’s picture

Status: Needs work » Closed (duplicate)

I 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

ZionEgg’s picture

Status: Closed (duplicate) » Needs review
damien tournoud’s picture

Status: Needs review » Closed (duplicate)

This should be closed.