Garland: Use appropriate header-tags following the W3C

I've never seen a site which does exactly as you say, but it absolutely makes sense to me..
I'll look into it...

Updating title to be more descriptive.

We've been talking about header tags a lot in the accessibility group - http://groups.drupal.org/node/18461#comment-63800

Nice that there's some good overlap in the SEO teams.

Looking forward to having this brought into core.

How is this affected by the new, default page.tpl.php that has been comitted?

I don't think this makes it better. Present code is:

          <?php if ($logo || $site_title): ?>
            <h1><a href="<?php print $front_page ?>" title="<?php print $site_title ?>">
            <?php if ($logo): ?>
              <img src="<?php print $logo ?>" alt="<?php print $site_title ?>" id="logo" />
            <?php endif; ?>
            <?php print $site_html ?>
            </a></h1>
          <?php endif; ?>

Which has H1 spit out the title of the site not the title of the page.

WCAG outlines it here:
http://www.w3.org/TR/WCAG20-HTML-TECHS/H42.html

Drupal uses a "Site Name | Page Title" convention in the but in an H1 you'd only actually want the "Page Title" to be reflected.

REVISION NOTE: Ran into this post here which is related - http://drupal.org/node/44072 - Consensus seems to be opposed to what is "semantically correct".

Mike

Current Drupal 7 core uses h1 when the $title is empty, otherwise uses a div/strong combo. This seems like the most elegant approach as it ensures an h1 for each page.

If all agreed I will work on porting this approach to Garland and submit a patch.

Here is the code from modules/system/page.tpl.php

          <?php if ($site_name): ?>
            <?php if ($title): ?>
              <div id="site-name"><strong>
                <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
              </strong></div>
            <?php else: /* Use h1 when the content title is empty */ ?>
              <h1 id="site-name">
                <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
              </h1>
            <?php endif; ?>
          <?php endif; ?>

Beg to differ:

http://www.w3.org/QA/Tips/Use_h1_for_Title

H1 certainly may contain a link. 999/1000 the only time this is going the happen is on the homepage, and that would point to itself, which is perfectly OK.

From the QA linked to says "the top level heading should assume a certain amount of context", which for the hompeage is going to be the site name, which makes this approach correct, being the top level heading of the root document of a collection of documents, ie a website. This infers the context of the entire collection.

Your idea to stuff the site slogan is problematic, slogan is optional, site_name is not. Site name likely has more meaning than the slogan wrt to the overall content of the website.

ID's/class names etc is not the issue here, this is example code and whatever classes Garland currently uses would be leveraged. That said I think the inferred semantics of #site-name are useful until such time as html5 is a reality.

Note: I edited this a number of time, my apologies, I needed to make the points clear.

Garland prints the site_name and site_slogan (if present) into the h1, I assumed you meant use the site_slogan as opposed to the site_name. This will not change.

It's an old example, and there's another old example using links here - http://meyerweb.com/eric/thoughts/2004/07/21/pick-a-heading/

But more recently here from the RNIB - http://www.rnib.org.uk/wacblog/headings/quick-tips-for-accessible-headings/

Are there pages without $titles where this would be an option?

Every page should have an H1 on it.

lutegrass, you haven't mentioned what the accessibility issue is. And I'm assuming it's not just for H1's but for all header tags, right? Can you point us somewhere that contradicts what the W3C says. Is there somewhere in WCAG 2.0 about this? A link would be good.

jmburnz, you say "Current Drupal 7 core uses h1 when the $title is empty", but not sure where and what part of core. Garland is part of core after all.

Mike

Mike, its here - modules/system/page.tpl.php

I understand this came into being as part of the whole Stark initiative.

Frankly I would think that in a normal drupal site only the front page does not have a title, although there is nothing to stop someone generating pages without titles, this could be a possibility. We used to check $is_front for this approach but checking $title seems safer, we always get that h1 if no title is present.

Thanks for pointing me here - modules/system/page.tpl.php

This file is in D6, and am pretty sure it's legacy code. Think it inspired Stark to expose where HTML is being generated in Drupal.

Don't think this is a reference for other module development and hopefully it will be removed (from what little I know of it this seems like a good idea). Maybe not though, seems active here - http://drupal.org/node/387218

Agreed on the $title is the safe way to do this. If $title is missing then the site name is probably the best to have in it's place. No H1 isn't good.

Mike

Yeah, I think the D6 page.tpl.php comes from way back, if fact in D6 it had the H1 only approach, this new Stark focused page.tpl.php in D7 is a world apart and a lot better.

Lets go for a patch, I'll work on it this weekend.

First crack at a patch. This follows the Stark method and tests on $title, if title then a div/strong combo is used, else h1.

Added an ID (#branding) to make styling the div/strong combo easier.

Na, not happy with that first patch, v2 cleans up a couple of selector inconsistencies.

... and here's the patch :)

Thanks for rolling (and re-rolling) this patch.

I think that you should explain why you've consolidated the CSS. The themers would probably have some thoughts on this, but I just want to clarify that this has been changed:

-#wrapper #container #header h1, #wrapper #container #header h1 a:link, #wrapper #container #header h1 a:visited {
+#branding, #branding a:link, #branding a:visited {

I haven't tested this, but wanted to go over again the changes. This is what is there now::

          <h1><a href="<?php print $front_page ?>" title="<?php print $site_title ?>">
          <?php if ($logo): ?>
            <img src="<?php print $logo ?>" alt="<?php print $site_title ?>" id="logo" />
           <?php endif; ?>
          <?php print $site_html ?>
          </a></h1>

And you are proposing a change to:

          <?php if ($title): ?>
            <div id="branding"><strong><a href="<?php print $front_page ?>" title="<?php print $site_title ?>">
            <?php if ($logo): ?>
              <img src="<?php print $logo ?>" alt="<?php print $site_title ?>" id="logo" />
            <?php endif; ?>
            <?php print $site_html ?>
            </a></strong></div>           
          <?php else: /* Use h1 when the content title is empty */ ?>  
            <h1 id="branding"><a href="<?php print $front_page ?>" title="<?php print $site_title ?>">
            <?php if ($logo): ?>
              <img src="<?php print $logo ?>" alt="<?php print $site_title ?>" id="logo" />
            <?php endif; ?>
            <?php print $site_html ?>
            </a></h1>
           <?php endif; ?>

And this will ensure that there is always only one H1 tag on every page, but there is at least one.

I'm not sure we've achieved consensus on this though. Have lutegrass' points been addressed (positively or negatively).

I can apply this to my sandbox for testing, but there are more changes that we've discussed. Sticking to the page.tpl.php file would be best I think and open up a new ticket for the css changes.

Mike

You can't have one without the other. We have to modify the CSS to support the use of the div/strong combo, div does not automagically inherit the attributes and values of h1, we must explicitly declare them.

The selectors are shortened because we don't need those long, highly specific selectors because #branding is an unique ID, whereas h1 could be anywhere, thus original Garland required those long selectors to negate any chance of an h1 outside of #wrapper #container #header h1 falling pray to its styles.

Unless #branding was to used outside of the header (unlikely) this is perfectly fine, in any case if #branding was to used outside of #header, then that should have the long hairy selector.

You need the CSS, otherwise it looks like a dogs dinner and will break the visual style of the theme.

If you think modding the CSS constitutes issue creep, by all means open another issue, but its really such a simple change to do it now. As a themer I find it difficult to submit a patch that actually breaks the theme.

I understand lutegrass has concerns, but the question is, if not the site_name/site_slogan as h1, then what? We need properly nested headings,and this mimics what D7 page.tpl.php is doing and I think this is at least a very safe approach.

I'd like to hear what the maintainer has to say about this first also, perhaps if there is another good approach someone could post an alternative patch so we can have something else to bounce around.

@lutegrass, well you're not a pest! I'm happy you are raising these concerns to be frank. The problem is, if not this approach then what else is there on the table for us to cherry pick.

Ok, so I've applied this to Garland.

Front page produces:

            <h1 id="branding"><a href="/" title="OpenConcept Drupal 7 Test Site">
                          <img src="/sites/drupal7.dev.openconcept.ca/files/color/garland-5d958a8b/logo.png" alt="OpenConcept Drupal 7 Test Site" id="logo" />
                        <span>OpenConcept Drupal 7 Test Site</span>            </a></h1>

There is no h1 tag on /tracker page, closest thing is:

 <h2 class="with-tabs">Recent posts</h2>   

There also isn't one when I went to http://drupal7.dev.openconcept.ca/node/6

Closest think I had was

<h2 class="with-tabs">I like input:focus</h2> 

Every page needs an H1 tag and this patch doesn't seem to do it (at least for me).

Mind you this is a pretty highly patched install, so maybe I'm missing something.

Mike

I would think that is separate patch/issue? I thought we were just looking at the header, the 2nd part is to switch Garlands default use of h2 on the $title to h1, good spotting never the less ;)

Do you think we should just roll one big patch?

Lets break it down what needs to be done:

1) Conditional element on $site_html, h1 if no title, otherwise div/strong (proposed solution)
2) CSS to support the new html
3) Change Garlands default use of h2 on $title to h1
4) CSS to support this change

How about we break it into two patches :1) 1st patch to change all the HTML bits :2) 2nd patch for CSS changes.

Visual style will break after patch one but thats not a big deal.

Let's keep it all in this issue. Can have more patches if required.

I'd also really like to see testing conditions. Where do we see this as a problem in the existing code?

What kinds of environments should we be testing this to ensure it is doing what is required? What should testers be looking for?

There should only be one H1 tag, but every page should have one. There should be multiple H2 tags.

I don't have a clean version of D7 to even evaluate where the problems are now.

Mike

Info on setting up test environment can be found here - http://drupal.org/node/28245

Personally I use Eclipse and have D7 head set up as a project, I replace from HEAD when I start work on a new patch and then again after creating the patch, then apply the patch to the clean HEAD.

You should use PHP5.

I use Firefox Accessibility Extension, this tool can be used to display headings and other issues.

NOTE: this will highlight an issue NOT addressed by these patches, that the nesting order is incorrect when using the div/strong tags, this because Garland prints the left sidebar above the content (so block h2 can come first) and this is not something we can easily address here and now if ever.

Two main things to test for are 1) browser support and 2) RTL

This is a matrix of browsers that I know drupal.org supports - http://developer.yahoo.com/yui/articles/gbs/, maybe this serves as a guide although I would think that perhaps Firefox 1.5 and Safari2 are still on the table?

Heres is an all in one whopper patch - what this does:

1) Conditional element on $site_html, if $title uses div/strong, else uses h1, includes a new id (#branding)
2) Replaces the $title h2 with h1
3) Add id="branding" to the h1 in maintenance-page.tpl.php for visual consistency during install and maintenance mode
4) CSS to support id="branding", with RTL changes also
5) CSS to support h1.tabs, with RTL support also

The patch should ideally be applied to fresh D7 HEAD prior to install, otherwise put the site into maintenance mode to check the changes there.

We need reports back on each OS specific version of browsers.
We need reports for RTL.

The above patch cannot pass simpletest because the help.test is hard wired to expect $title wrapped in h2 heading, I assume this is done to specifically support Garlands original use of h2 heading for $title.

On my localhost if I hack help.test to use h1 I get all passes.

Here's the relevant code form help.test,

Line 63:
$this->assertRaw('<h2>' . t($name) . '</h2>', t('[' . $module . '] Heading was displayed'));

As stated at #454490: help.test expects $title in h2 heading the patch needs to make necessary changes to help.test. We cannot have failing tests in core.

Right on boombatower, I will get it done.

Patch v3 from #27, rerolled to include change to help.test

Love me bot, please :)

Seems to be a problem with the non front home pages. Usually the title of the sight is bright white.

        <div id="logo-floater">
                              <div id="branding"><strong><a href="/drupal-cvs/" title="test cvs">
                          <img src="/drupal-cvs/themes/garland/logo.png" alt="test cvs" id="logo" />
                        <span>test cvs</span>            </a></strong></div>           
                          </div>

Not sure if it's the switch to the branding id or not.

Wish I had time to offer more.

Have you checked the patch applied correctly (is the new css in style.css) and if you saved a color profile you need to resave the theme settings for new CSS to take effect.

You're right. The changes for themes/garland/style.css didn't catch.

All looks good now. Found the H1's I was looking for.

Looks like a good patch!

Agreed on this issue, tested and works as expected/should be.

Let Garland be even nicer to SEO, and commit this...

Stefan

Personally, I'm not sure the word "branding" makes a lot of sense. Can't we come up with something a bit more descriptive and helpful?

These changes are essentially changing the headings for the titles and ensuring that they are ordered correctly.

So we could call it heading or title, maybe even page-title. But agreed that branding isn't very descriptive in this context.

Mike

Looks like this still needs some work due to the ID name issue. Are there standards (or at least "best practices") we can draw from? I seem to remember Eric Meyer or someone trying to come up with a standard list of classes/IDs for all websites. Anything ever come of that?

I'd also love some before/after screenshots to show whether there is/is not a visual change as a result of this patch.

Alright, let's look for a better ID/name. I like how Mike is thinking and the suggestions he made. You want to drive the process of picking a better name, Mike?

@webchick - interesting that you bring up Eric Meyer, that is where the #branding id comes from.

http://meyerweb.com/eric/thoughts/2004/06/26/structural-naming/

If we don't like branding I might suggest following the current Drupal 7 core id which is #name-and-slogan, so we would have something like #logo-name-slogan.

There are no visual changes, I was very precise about maintaining the visual style.

I'm not very good at choosing names for things, so will decline on driving the process to pick a better name. I do think that #logo-name-slogan is pretty self-descriptive so might be the right approach.

I was a bit shocked that Eric's article was from 2004. Found some other ones that also kept the brand phrase that didn't mean much to me, but apparently means a lot to those looking at more semantic CSS identifiers.

What's in a Name and the followup conclusion

From 2008 there's the Structured Naming Convention in CSS article.

This older article on this topic also links to a table of naming conventions, but it doesn't look like it has been maintained.

These are good ideas, particularly for CMS themes. It might be useful to see if some of the Zen folks would be interested in doing a whole review of the Garland nomenclature to see if there is a way to make it more standardized.

That being said I think that this type of review should be done in another issue queue and that this patch should be brought into core.

I didn't see any problems with the visual style after applying the patch btw.

Mike

Ok lets drive this foward and make some proposals, lets here those +1's.

First some discussion.

#branding seems to be only real standard expressed by the likes of Eric Meyer and Andy Clarke. To me this makes sense, what is a brand? Logo, name, slogan? These comprise at least somewhat part of brand identity.

#banner - the proposed WAI ARIA role for these elements.

#logo-name-slogan - makes sense if we follow core page.tpl.php. To me fails semantically when any one of those elements are missing.

#heading - has potential imo, although is semantically weak.

#site-title - throwing this one out there, again I think this is semantically weak.

The #page-title suggestion - most other themes use this id for the actual page title, which makes sense, whereas this is not the page title, its the site name, logo and slogan.

In no particular order.

1. #branding
2. #banner
3. #logo-name-slogan
4. #heading
5. #site-title

+1

Hi Guys,

I've been manually applying this patch to a Drupal 6 install, hoping to achieve the same effect (it works wonderfully with just a little hacking, by the way).

While going through the code to understand what is going on, it struck me that the only difference in the garland/page.tpl.php file actually needs to be the type of container that we stick this content in (it's either "div" or "h1" - everything else is the same).

It therefore seems that in comment #20, that we could dramatically simplify (and clarify) the code by doing:
$container_type = ($title)?"div":"h1";
Then in place of <h1> and </h1> in the origonal code, we simply use <'.$container_type.'> and </'.$container_type.'>

Obviously if the current code is working to the satisfaction of all, there's no real need to change.

The D6 approach was abandoned in D7 Garland in favour of preprocessing $site_html, and simplifying the logic in page.tpl.php (prior to this patch), I would not like to go back to the D6 method. While the patch code looks pretty knarly I still think we should avoid the D6 style of logic in templates, so we could do the whole thing in garland_preprocess_page but then we force users to dig into template.php just to figure out where the HTML is coming from - I think that is fine for a contrib theme but I'm not so sure about a core theme.

From the RNIB article at http://www.rnib.org.uk/wacblog/headings/quick-tips-for-accessible-headings/

3. Pages should only have one main heading, H1, which is the main title of the page and should be positioned at the start of unique page content.

4. Headings are like a contents overview of a page. Sub headings of the H1 must be coded as H2 and subheadings of an H2 must be coded as H3.

5. Heading levels can’t be skipped i.e. you can’t jump from H1 to H3.

8. Heading structure should be consistent throughout the site.

If these recommendations from RNIB are worth following then a concern comes if the first heading on the site (DOM order) is not an H1, in this scenario it would be a violation of RNIB recommendations to use any other H2 headings for block content prior to the H1 heading that is recommended for unique content. Not sure if this was well thought through by the RNIB author.

My primary concern is that suggestion 8 be followed as much s it is possible, that heading structure stays the same across a site.

#site-banner is my recommendation

This is semantic, this is the banner for the site, regardless of what components the banner consists of, and conveys slightly more meaning than #banner.
+1

+1

#banner or #site-banner are fine names. Let's not let the name question hold up this important fix for D7.

I'll try to make time to pick up these patches again this week, been pretty flat out but this is an easy patch to roll if anyone wants to jump in ;)

Can reroll this patch this week. Just want to clarify that the only change to the patch in #32 is replacing banner with something different.

After reading comments #41 and #43 again I am actually quite happy with "branding", however, I am still happy with site-branding or site-banner.

Would be great to get some more thoughts on this naming so that we can get these changes into core.

If no one has objecting I am going to reroll this patch using branding as it seems to be the naming with the most support. It is accurate, the branding elements for a site (logo, title, slogan).

Setting back to needs review. I give a +1 to #branding as it appears to be an adopted standard as per #41.

If the patch in #32 passes then it would be great for someone to make some before / after screenshots so that we can set back to RTBC. Otherwise, I'll reroll if it fails testing.

Applied the patch in #32 to clean head. Looks like the chunck in help.test is failing to apply. Can someone familiar with simpletest files please take a look at this and see if you can figure out what changes need to be made to reroll this patch?

I think this will work for the testing as it should be using Seven for the help & not Garland.

I ran into some fails when I tested #32, but think this will work.

Think this is something that should be really, really close to RTBC as it's had lots of time to consider wording issues and this is essentially a re-roll of #32.

Subscribe. +1 for importance of applying semantic structure as endorsed by NFIB. There is a lot of bad practice on the Web, but the h1 should always be the title of the page. (For the home page, that's the same as the name of the site. Elsewhere, it isn't.)

.

There were some changes in the page.tpl.php file that broke the previous patch. Should be just the same though. Would be nice to get this into core so I don't have to keep rolling it though.

+1 for this patch. The code looks clean to me and it passes these tests:

• No changes to visual appearance in Garland--the logo and titles have exact same pixel position and size
• Tested with Mac Firefox 3.5, Windows Firefox 3.5, Mac Safari 4, Windows IE 6 and IE 7
• Tested with both Garland and Seven as the admin theme
• Tested as anonymous and adminstrator
• Verified that the site title/logo area is <h1 id="branding"> on the home page, but it correctly drops to <div id="branding"> on secondary pages (including 'Create new account', admin pages, both viewing and editing nodes, and actual content pages) when the page title becomes h1

Good work! This will be a boost for accessibility and set a good example for theme developers!

Can we get one more review of this final patch? I believe it's ready for RTBC and it is imporant for accessibility of D7.

Hi

I have used this patch and it works to

1. Give H1 to Website name when we are on homepage
2. Give H1 to Website content title when we are on content page

Thanks

After recent work on accessibility in my other themes I discovered that if the $site_name is OFF in theme settings then the alt tag is empty for the logo.

In Genesis and Adaptivetheme I solved this with the following:

$vars['logo_alt_text'] = check_plain(variable_get('site_name', ''));

The issues in template_preprocess_page

$variables['site_name'] = (theme_get_setting('toggle_name') ? filter_xss_admin(variable_get('site_name', 'Drupal')) : '');

This checking is too simplistic for many use cases as it simply leaves the variable empty when its toggled off - but this toggling should be more along the lines of a "visibility setting", not leave the variable empty, so we can use it for other things such as alt text.

We need to solve this for this to be RTBC.

Patch to solve the issue in #64 - please review the approach, that is the main thing, and the variable naming.

The patch creates a new variable ($site_html_attributes) and populates this using variable_get on both the site name and slogan in garland_preprocess_page, then prints this new variable for the site name title and logo alt text attributes. This ensures they are never empty - even when they are toggle off in the theme settings.

Thanks Jeff.

I had a problem in jumping from &lth1 id="branding"> to &ltdiv id="branding"> that the size of the site title jumped size substantially.

The image problem in the attachment is unrelated. I would have thought that font-size: 1.5em would be large enough but didn't seem to catch.

I'd have to assume that the patch did not apply cleanly for you - esp seeing the broken logo.

Can someone try to verify Mikes issues?

This is good on my system, I did some quick review testing in IE7, Firefox 3.5, Safari 4 and all no problems.

Sorry, my bad. Think I had applied too many patches to that install.

I started from a fresh CVS version & new db and it's all working well now.

Think this is RTBC.

Sorry, my bad. Think I had applied too many patches to that install.

I started from a fresh CVS version & new db and it's all working well now.

Think this is RTBC.

This looks good to me to. I tested the functionality and it works with no visual changes from the current Garland for pages both with and without a $title.

FYI, when I applied it I got the message "(Stripping trailing CRs from patch.)" But it applied cleanly and worked.

+++ themes/garland/template.php	4 Nov 2009 17:52:24 -0000
@@ -83,7 +83,11 @@
+  // Set a variable for the site name title and logo alt attributes text.
+  $slogan_text = filter_xss_admin(variable_get('site_slogan', ''));
+  $site_name_text = filter_xss_admin(variable_get('site_name', 'Drupal'));
+  $vars['site_html_attributes'] = $site_name_text .' '. $slogan_text ;

I would never in a million years have guessed that this contained the site name and slogan from the name of the variable. Is there something more semantically meaningful?

Also, why do we only make this change for Garland and not for other themes? Are the other themes getting this right? If so, can we simply remove special-casing from Garland?

This review is powered by Dreditor.

Agree on both points webchick - esp since the variable name is like a special case for a special case, meaning that Garland builds its own var $site_html with special logic and the variable name was a sort of name spaced var stemming from that.

No, most other themes do not get this right (those that try to use the site name as alt text anyway), however many themes simply use something like alt="<?php print t('Home'); ?>" rather than $site_name.

Need other themers feedback here; special case for Garland or something new for everyone?

Is it a good idea to use the site name as alt text on the logo & titles etc, or does home / home page in t() suffice?

Home appears in a lot of breadcrumbs. People know the term. I use alt="home" or alt="Back to the home page". alt="d-theme.com" makes the link sound like it goes to an external site and there are so many advertising images and links on some sites that people start to ignore anything going to a site name.

Re the discussion of changing a lot of themes. h1 works as the content and page headings in themes where the content column is first but fails when there is a left sidebar because the left sidebar contains h2. Various themes put content first and that solves several problems including the h1 problem. Placing content before the header or the top menu bar is difficult and unreliable because it depends on a fixed header height. Placing the content before the left sidebar is far easier and more reliable because it depends on a fixed width. Widths work in most browsers way back into old versions. Most themes with a left sidebar use a fixed width sidebar. Placing the content before the first sidebar seems like a killer solution easy to apply in most themes.

Christmas is based on Aardvark which uses the negative margin idea used in several themes including Basic and Zen. Research shows various Zen based themes working in a lot of sites in a lot of browsers. Basic removes some of the excess from Zen. Aardvark uses a simpler approach for faster loading and easier modification but it is still the same negative margin with the content before the left sidebar. That specific feature of all the themes does not break in any of the tests I tried. I could run up a D6 Garland placing the content before the left sidebar if that would help people test the idea. You would then have the content h1 before the left sidebar h2.

I believe we should continue with this patch for Garland and take up fixing other themes as a separate patch and separate issue.

Garland in Drupal 7 should be a model that is accessible, and then we can work to promote improvement of other themes with Garland as an example.

What should the ALT text be? The ALT text should contain the equivalent content/meaning as the image itself. The is a split in the screenreader community about whether it is better to use "ACME Corp. logo" or "ACME Corp. homepage". I would support either of those, but I recommend against just saying "Home" because that does not identify the site which the visual logo image certainly does. Here are survey results about what screenreader users prefer:
http://www.webaim.org/projects/screenreadersurvey/#images
http://www.webaim.org/projects/screenreadersurvey2/#images

Acme Corporation logo with link to homepage is the most popular in the relevant question. Using the Christmas theme on http://d-theme.com, the image would need alt="Drupal logo wearing a Christmas hat with link to homepage". I can understand that. I talked with a blind person about a page that had many images with alt="President Clinton and family". The blind person asked me why would you have five identical images on the same page? Which members of the family? Where are they? What are they doing?

I am favoring the option of alt="Site name logo" because the link is to the homepage (assuming the logo has a link, which in Garland it does), so the screen reader is going to indicate that.

The major issue Webchick has raised is the variable naming and the special casing - these are significant issues and worth addressing, if we can remove the special casing and make this a fix for everyone, then that's the best route.

The $site_html and $site_html_attributes variable names suck, these do need to be more semantic.

I'll put some thought and discussion at Drupal camp this week and see if we can come up with something better.

@peterx: You pointed out that the survey favored "Acme Corporation logo with link to homepage". I believe the survey question might have been confusing. Screen readers will automatically announce an image link as a link (e.g., announcing "visited link image [alt text here]"), so I believe it would be redundant to say "link" within the alt text. I know on its face the survey seems to suggest otherwise.

@brandonojc, you are right on target. Adding "link" to the actual "alt" text would make that word be announced twice, and that would be an annoyance. It could also be highly confusing, as one might think two links were present.

@webchick, I think the reason for using Garland as a special case is to ensure that all the themes included with core get this right. The ambition to fix all themes is there, but for D7 it is a significant goal to ensure that all themes included with core support the creation of accessible websites.

And is the problematic variable name "site_html_attributes"? If so, why not use "site_name_and_slogan"? Seems to me it would be a stretch to assume that a variable by that name would contain anything but the site name and site slogan. (If this is a dumb idea, my apologies. I know next to nothing about php.)

Yes, "site_name_and_slogan" is a good name for that variable. I suggest re-rolling with that to see if we have agreement and get back to RTBC.

FYI, the last line of code in the patch needs two tweaks for coding standards. As proposed in #65:

$vars['site_html_attributes'] = $site_name_text .' '. $slogan_text ;

Should be:

$vars['site_html_attributes'] = $site_name_text . ' ' . $slogan_text;

The two changes are: 1. adding spaces around the "." operator on both sides of the 1-space string (' '); and 2. taking the extra space out right before ";". (Plus the name of site_html_attributes should change, of course.)

@Cliff, I think that @webchick is suggesting that if this is a generalized problem with all themes (which it is), then it should be fixed not just in Garland's theme, but perhaps in phpTemplate so that we can ensure that this new template variable is available for all in a Drupal 7 version of this list - http://drupal.org/node/11812

@brandonojc, changes make sense. Do think we're ready for a re-roll. We're going to also need to work on the docs for when this gets in.

@mgifford, that makes sense. I didn't realize a global fix could be that easy.

As for the docs, is that also needed before December 1? Or is there a later date on that?

Re-rolled as per #80.

+1 for the patch in 83. Setting RTBC. I reviewed and tested and found the code looks good and works as desired.

I believe this patch responds to all the feedback since the last time it was RTBC:

• #71.1 @webchick: The variable name is more meaningful. ;-)
• #71.2 and #72-#79: Regarding fixing other themes, all the other core themes have better use of headings already. @Jeff Burnz is correct that they could have better alt text, but the basic issue with this patch is the H1 versus H2 for page titles issue. The other core themes already use H1 for the page title on content pages and for the site title on the front page. I tested to confirm that Stark and Seven do this properly already (with or without this patch). It was only Garland/Minnelli that were using H2 for page titles, which this patch fixes. If we want to improve the logo ALT text on other themes that seems like a good follow-up or a new issue. Please correct me if I misunderstand or if you have suggested code to include.

Tiny clean-up: it appears the patch adds 2 spaces on a blank line 86 right before the comment in themes/garland/template.php:

@@ -83,7 +83,11 @@ function garland_preprocess_page(&$vars)
-
+  
+  // Set a variable for the site name title and logo alt attributes text.

Much better, thanks!

Committed to HEAD.