theme_status_messages needs additional markup to be more apparent to assistive technology users

Here's a patch for recommendation #1:

     $output .= "<div class=\"messages $type\">\n";
+    $output .= "<h2 class=\"element-invisible\">" . t("$type messages") . "</h2>\n";
     if (count($messages) > 1) {

We make an invisible header that simply says "error messages" or "status messages".

I'm eager for feedback.

@Everett

3.1. Yes, the patch only outputs a header if there are messages. The patched line is within an if() block for that.

3.2. You are correct that some themes might put the status messages with this h2 before the h1. Garland puts the messages just after the page title, so that's best. Most themes will probably order it like Garland. But if some theme doesn't, I believe it's okay to have an h2 before the h1 since the W3C includes that in an example for WCAG 2.0:
http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H42.html#H42-ex3

Let me know if you have any concerns or alternative suggestions. Thanks for the feedback!

Is using an invisible H2 tag the recommended approach? If so, I'd add a PHPdoc comment with some explanation and a reference to the recommendation. Thanks!

Cool. I've updated the patch and included an explanation in a PHPdoc comment. I'll work on providing some references. Thanks!

I have reviewed this patch and everything seems to work and look fine. It has passed all tests on my local environment.

Like Dries, I am curious as to the precedence to this method. I like the idea, but it would be nice to know why an H2 tag was chosen and not something else.

--
zzolo

@Dries and @zzolo

Thanks for reviewing this. I've added comments into a new patch and I'd appreciate any further input and direction.

• Regarding appropriate use of element-invisible, "content should only be hidden from sighted users and made available to screen reader users when content is apparent visually, but not apparent to screen reader users," according to WebAIM. See http://www.webaim.org/techniques/css/invisiblecontent/ for the complete guide.
• Regarding the choice of h2 versus h3, the W3C recommends that heading levels not be skipped (such as skipping from h1 directly to h3). Since the status_messages often follow the page title, which is usually level h1, it is best to use level h2. See http://www.w3.org/TR/WCAG-TECHS/H69.html and http://www.w3.org/TR/WCAG-TECHS/H42.html for examples and explanation.

@Everett

Regarding making the heading invisible in the theme layer, that would be possible but more complex. It could be done like 364219 by adding a $heading parameter to theme_status_messages(). That would allow flexibility choosing a heading level and classes. We would add a call to theme_status_messages() within preprocess_page() in Seven and Garland to make it an invisible h2. But that is a more complex solution. It seems like a safe assumption that most themes will style status_messages in a visually apparent way for sighted users, so it is safe to make it invisible by default. That said, I would gladly code this if it is a preferred alternative that will get committed to make Drupal 7 more accessible.

I think making the heading invisible is a good default and brandonojc makes good arguments as to why that is.

Also, I think brandonojc makes a good point as to why h2 is chosen. I guess I was thinking that there was some arbitrary reason, but it makes sense. I would say that this is good to go.

Thanks for the clarification, as well as updating the PHPdoc. Very helpful.

Alternatively, we could consider displaying a heading to all users but that would require design work to make it look good. We can consider that in another patch.

Anyway, t("$type messages") does not properly use the t() API. That breaks our ability to automatically extract translation templates. Have a look at how other t() function deal with dynamic input parameters.

We're close. :)

The problem here is that the $type is a string identifier and not translated: http://api.drupal.org/api/function/drupal_set_message

What might be best is creating a simple array. This works because there are really only 3 different values for type:

$types = array(
  'status' => ('Status').
  ...
);

Then using the type to call the correct translated text

t('!status message', array('!status' => $types[$type]));

Or, actually, you could just put the whole message into the array and call as needed. The above method seems a little unnecessarily complicated.

Please review the updated patch. Thanks to everyone for all the feedback. As Dries said, "we're close" and of course I want to take the time to get it done correctly so I appreciate all the input.

@brandonojc

Actually, I think you may be thinking about this wrong. First we must think about what $type is. It is a string identifier for 'error', 'status', 'warning'. These are not friendly to put on the page, but more importantly these are never translated since they are just identifiers.

So, what would be best, I think:

function theme_status_messages($display 
  $output = '';
  $status_header = array(
    'status' => t('Status Message'),
    'error' => t('Error Message'),
    'warning' => t('Warning Message'),
  );

   foreach (drupal_get_messages($display) as $type => $messages) {
     $output .= "<div class=\"messages $type\">\n";
     $output .= '<h2 class="element-invisible">' . $status_header[$type] . "</h2>\n";

  .....

@zzolo

Thanks for the input. Please take a look at this new patch that implements your suggestion. Your technique is better for maintenance and clarity. I was trying to save a few CPU cycles by using the internal $type variable to create the text since they happened to match, but your suggestion is better and cleaner. Let me know if you have any other suggestions on this one.

Everett makes a good point. It would be ideal to do an empty() check before displaying a title. In fact, the h2 should not be displayed if there is no title to display. Otherwise, looks good.

Thanks, zzolo and Everett. Here's an updated patch for review.

Looks great. Good job.

One last quick observation: I *think* that $status_heading[$type] needs to be check_plain()ed. I'm not sure where this data is coming from exactly but it seems like a good idea.

@webchick

It looks safe to me because we define $status_heading[] here in this method, so even a malicious $type variable wouldn't give us a bad $status_heading[$type] value. But if you or others decide a check_plain() is appropriate then let's add it.

All righty then, committed to HEAD. :) Great work, folks!!

It seems like this approach needs to be documented somewhere so that contributed module authors and core developers known when/when not to use such code in their own theme functions, so marking needs work for documentation.

What we probably need to do is start a new "Accessibility" section in the handbook under the Developer's Guide (http://drupal.org/contributors-guide). Please coordinate with the folks in #drupal-docs about where/how best to do this.

@webchick: The significant section on accessibility in http://drupal.org/node/464472 seems like it's the place. There is also an ongoing issue about accessilbility docs for theming: #462532: Add accessibility information to Theming Guide

@Everett, @brandonojc, most of us know very little about accessiblity.

Would either of you be willing to take on the the item requested by webchick in #28 - probably in http://drupal.org/node/464472, add info about this issue and best practices regarding it?

Yes, I'd love to contribute a few sections:

• Recommended Header Structure
• Proper use of "element-invisible" and "display:none"

Thanks, everyone!

@Everett: It can, but I wouldn't push it too long. The idea is that starting day 1 of code freeze, module and theme authors can start porting their stuff. I anticipate there being a huge cabal of people on the code sprint following DC Paris working on porting as many modules as possible in one day. If we don't have documentation present to help them get this right, we'll likely spend the next year+ chasing them down with follow-up patches.

Not that documentation doesn't mean that people won't continue to get this wrong, but it's much easier to point to canonical reference than to make the same follow-up 50 times in 50 projects.

Please review this draft contribution to the Theming Guide:

http://ojctech.com/blog/headings-and-visibility-drupal-theming-guide

It covers the 2 topics: "Recommended Header Structure" and "Proper use of element-invisible and display:none". I'd appreciate any suggestions or input. Unless I hear any large changes or objections soon I'll submit it into the Theming Guide and the work can continue there.

This looks great, but one basic thing I am confused about is what is the CSS definition for class "element-invisible"?

The CSS for element-invisible is defined in system.css:

.element-invisible {
  height: 0;
  overflow: hidden;
  position: absolute;
}

This particular technique for hiding content was chosen after lengthy debate. I'll add this CSS to the documentation since others have been curious as well.

Documentation is now available in the Accessibility section of the Theme Guide:

Headings: http://drupal.org/node/561750

Accessible Techniques for Hiding Content: http://drupal.org/node/472572