Posted by dfserra on January 29, 2007 at 3:17pm
13 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | Garland theme |
| Category: | task |
| Priority: | normal |
| Assigned: | Stefan Nagtegaal |
| Status: | closed (won't fix) |
| Issue tags: | accessibility, CSS problems, screenreader |
Issue Summary
The blinds use screen readers for read Internet pages.
The screen reader show the information in html order.
Garland theme has un problem with html order.
The footer is after the main content and before the right sidebar.
The footer should be the last html code.
The appearance no problem, only the html order.
Thank you.
Comments
#1
I'm a bit surprised how easy this was. I feared simply moving the footer div would break the page design, but it's done nothing apart from widening the footer area. Before, it was between the sidebars, now it fills the width of the page and lies below the side bars.
The only problem I can see is if the sidebars are much longer than the content, in which case the footer is far deeper down (you have to scroll down the whole page to see it, not just to the end of the page). But I'm not sure if this is enough of an issue to merit keeping the HTML order as it is now.
#2
Sorry, my wording was unclear there. I meant the end of the content of course.
#3
I think this is an okay change if well-tested. The change should happen first on the current development version, now 7.x, and then be backported as necessary. I am not 100% sure a backport to 5.x would be good since the page layout does change slightly. It will be easier to make a backport decision once the current version is sorted out and committed.
#4
I'll look into this soon...
#5
#6
Should I reroll for D7?
#7
@Arancayar: you may do so, but I'm afraid it gives problems with the layout.. (Remember we can haveno ly the left sidebar, the right sidebar, both, or none) when testing..
And, please verify that it works with: IE6, IE7, Safari 2, Safari 3, Firefox, Opera..
#8
Arancaytar, keith.smith, drumm, fdserra: Any news on this yet?
Did any of you guys tested this?
#9
I've re-rolled the patch and tested it with my D7 install. Seems to work fine for me.
It is logical that the footer would come last. Even just for those who are first trying to figure out how Drupal works.
#10
We shouln't be changing the visual style of Garland. This is new patch that adds dynamic margins to the footer, padding and background color to maintain the visual style.
Tested in IE6/7/8, Firefox3, Opera 9.62, & Chrome - in all a sidebar configurations.
The left and right padding is the same as for .left-corner, but the bottom I set to 5px i.e. padding: 0 25px 5px 35px;
Background set to #fff
Margins - before there were negative margins and no margins set for body.two-sidebars, I reversed this to give positive margins for body.sidebar-left & body.sidebar-right, and set margins for body.two-sidebars, i.e.:
body.two-sidebars #footer {
margin: 0 210px;
}
Most certainly needs more eyes on it, this needs to be pixel perfect. The only possible visual difference is the 5px padding bottom on the footer.
#11
I heard some encouraging things about the state of Drupal in accessibility. We seem to be the ones caring and the accessibility community is watching that in hopes of putting Drupal to use. Thanks guys for taking this step.
#12
@jmburnz: Use double spaces in your patch instead of tabs please..
We need this tested in *all* major browsers.
When we designed the theme it was cross-browser, and we should keep it that way.
I'll investigate this a bit further when time permits, probably in the upcoming week or so..
#13
Is there a browser matrix that Drupal core follows, such as Yahoos.
#14
Not sure how those tabs got in there, pretty sure all nuked now.
#15
Seemed to work. Had a bit of an issue applying it:
mike@dev:~/dm7$ patch themes/garland/style.css < garland_footer4.patch(Stripping trailing CRs from patch.)
patching file themes/garland/style.css
(Stripping trailing CRs from patch.)
can't find file to patch at input line 49
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: themes/garland/page.tpl.php
|===================================================================
|RCS file: /cvs/drupal/drupal/themes/garland/page.tpl.php,v
|retrieving revision 1.24
|diff -u -r1.24 page.tpl.php
|--- themes/garland/page.tpl.php 18 Feb 2009 14:28:25 -0000 1.24
|+++ themes/garland/page.tpl.php 30 Apr 2009 00:03:56 -0000
--------------------------
File to patch: themes/garland/page.tpl.php
patching file themes/garland/page.tpl.php
patch unexpectedly ends in middle of line
Hunk #2 succeeded at 65 with fuzz 2.
Would be great to see this change in core.
Mike
#16
Wider testing is needed, I don't have access to OS X machine right now, I have Win XP, Vista and Ubuntu to test in etc, but no mac.
#17
I'll test this on my OS X box in Safari, Firefox, Opera and Flock.
When testing, make screenshots in IE 5.5, 6, 7 and 8, Firefox, Opera, Mozilla, Safari for Windows and attach those here.
It would be nice if there are some people, who could make some screenshots for Linux/Ubuntu too.
Remember to test any kind of block configuration, ie:
- no blocks at all;
- only left sidebar blocks;
- only right sidebar blocks;
- left and right sidebar blocks;
- no footer, no blocks;
- no footer, only left sidebar blocks;
- no footer, only right sidebar blocks;
- no footer, left and right sidebar blocks;
- footer, no blocks;
once we are really sure this is passing in all browsers, I'm going to set this RTBC.
#18
These screenshots are for IE 5.5, 6, 7, 8, Firefox 3, Opera 9 and Chrome (all Win XP).
The show the configs as follows:
2 sidebars with footer
sidebar left with footer
no sidebars with footer
2 sidebars without footer
28 in all.
Before I go on let me know if you see any issues.
#19
The last submitted patch failed testing.
#20
I tested on any browser in OS X. this is rock solid and ready for committing.
This get's my vote..
@Dries, go for it! :-)
#21
This patch needs to be re-rolled because the footer message is no more.
#22
Ok, I will re-roll, thanks Dries.
#23
New patch with no footer message, otherwise identical.
#24
No idea why I had trouble applying this patch.
#25
The last submitted patch failed testing.
#26
Because you are not running it against head, Garland page tpl no longer has a footer-message, the #24 patch above will fail the test. The patch in #23 is correct.
#27
I'll have to check my environment again, but I deleted those files and ran CVS update on them to pull down fresh versions of the page.tpl.php & style.css pages. Maybe the cat walked across the keyboard and pulled down a branch or something highly random like that. Sorry.
But yeah +1 on patch #23!
#28
The last submitted patch failed testing.
#29
Add tag.
#30
Reposting patch from 23 to see if we can get it to test properly, as status has been "retesting" for some time.
This patch is the same as the patch in 14 set to RTBC in 20, but without footer message.
#31
+1 for the patch in 30 if it is still having no negative effect on visual appearance.
#32
+1 for the patch in 30. I installed it locally and I wasn't able to observer any problems with the visual display in FF3.
#33
I did a tonne of testing with this earlier, so unless theres a nay sayer... RTBC
#34
The last submitted patch failed testing.
#35
Does anyone have any idea why this patch keeps passing, and then subsequently failing the testbot?
#36
@Everett: It depends on the status. "Failed to apply patch" means that HEAD changed because other patches were committed and it made this one no longer able to find the lines it's supposed to change. Needs to be re-rolled.
#37
Rerolled patch in #30. Only changes were replacing body.sidebar-left and body.sidebar-right with body.sidebar-first and body.sidebar-last, as this has changed in style.css since the patch was last rolled.
#38
I think the patch in #37 is ready for RTBC as it is the same as the patch in #30 with minor changes to class names to accommodate changes that were made in themes/garland/style.css
#39
I compared a patched version and an unpatched version. In the pathed version the DrupalIcon is lower than the others.
#40
Problem seems to be with the placement of the min-height:400px; in the .left-corner or possible .content class.
Removing it brings it back up in Firefox where it is elsewhere. Unfortunately it changes the whiter space below it too.
Not sure how to resolve the css issues here.
#41
The last submitted patch failed testing.
#42
Ok, just swapped this:
<div id="footer"><?php print $footer ?></div>For this:
<div id="footer"><?php print render($page['footer']) ?></div>And it should work again.
Still has positioning issues. Not better/worse as far as I can tell but different.
#43
The last submitted patch failed testing.
#44
Think this issue is closed now as I believe footers are managed in blocks now.
#45
Mike, do you think this is still relevant now? Footer region is still going to be the place where users put footer information, do you think it really matters where it comes in the source order?
I'm leaning towards that it does - users placing content in the footer would expect that its at the bottom (would appear at the bottom) and be less important (maybe?), but to appear at the bottom to screen readers it must be at the bottom of the source order, not above sidebar-second.
#46
There is a challenge with mediating how the world does work and how it should work. Most AT users are at this point used to navigating a site so that it is presented in the order that it appears visually rather than how it should appear logically. I don't think that most AT users will assume that the footer is going to come at the bottom of the page's DOM.
That being said, I do think it makes more sense to have the footer container at the bottom of the HTML. I don't think this is nested properly, but to get this moving ahead again.
It's listed here http://drupal7.dev.openconcept.ca/
#47
Looking at this again with fresh eyes I see a problem that I have no idea how we missed earlier on - if the sidebar is taller than the center, the footer is pushed down and theres gap between the center and the footer. In current garland the backgound of the footer is white and it sits hard against the bottom of center.
The only real way to emulate this is to employ an equal height columns technique , which could be hard to make work in Garland. I'll give it a go.
Update, arrghh, well thats going to be a really tough ask....
#48
I'm not sure it's that large an accessibility issue. It would be a nicer structure, but not sure that it will impede use.
#49
#46: garland_footer-45.patch queued for re-testing.
#50
I'm bumping this to D8, unless Garland gets retired #911054: Remove Garland from Core in which case it can be closed.
#51
Closing since #911054: Remove Garland from Core went in.