johnbarclay & I been discussing things in the Accessibility group to ensure that navigation menus are easier to navigate - http://groups.drupal.org/node/18461

Some of the changes fall out into just adding in some <h2> tags as suggested in WCAG 2.0:

Main additions for this (and a few other instances), include adding a hidden or offscreen CSS definition that can be used to ensure that the header is visible to screen readers and search engines but not to sighted visitors:

  .offscreen { position: absolute !important; left: -999em !important; }

and some header elements which are presently blank. In my recent blog post evaluating Drupal themes for accessibility, Garland wasn't even evaluated in this list as it didn't validate. However, it would be great for Drupal 7 if it did validate and that there were no warnings listed with it.

As I've tried to illustrate in another posting in the Accessibility Group, there are other reasons to include some sort of standardized hidden class which yanks the content off of the screen for visible users but leaves it in place for those who don't have the visual clues.

Files: 
CommentFileSizeAuthor
#92 364219-fix-theme-links_6.patch3.6 KBTheRec
Failed: Failed to apply patch.
[ View ]
#88 364219-fix-theme-links_5.patch3.3 KBTheRec
Failed: Failed to apply patch.
[ View ]
#75 364219-fix-theme-links_4.patch6.52 KBTheRec
Failed: Failed to apply patch.
[ View ]
#72 364219-fix-theme-links_3.patch7.56 KBbowersox
Failed: Failed to apply patch.
[ View ]
#71 364219-fix-theme-links_2.patch7.35 KBTheRec
Failed: Failed to apply patch.
[ View ]
#63 364219-fix-theme-links_1.patch1.59 KBTheRec
Failed: Failed to apply patch.
[ View ]
#56 nav_headers_364219_repair.patch2.26 KBbowersox
Failed: Failed to apply patch.
[ View ]
#47 nav_headers_364219_v3.patch4.87 KBEverett Zufelt
Failed: Failed to apply patch.
[ View ]
#44 nav_headers_364219_v2.patch4.87 KBbowersox
Failed: Failed to apply patch.
[ View ]
#39 nav_headers_364219.patch4.6 KBbowersox
Failed: Failed to apply patch.
[ View ]
#23 garland_headings_364219_v1.patch2.39 KBJeff Burnz
Failed: Failed to apply patch.
[ View ]
#16 garland_h2.3.patch1.6 KBmgifford
Failed: Failed to apply patch.
[ View ]
#11 garland_h2.2.patch1.6 KBmgifford
Failed: Failed to apply patch.
[ View ]
#2 garland_accessibility.patch2.16 KBmgifford
Failed: 10613 passes, 0 fails, 328 exceptions
[ View ]
garland_h2.patch1.96 KBmgifford
Failed: Failed to apply patch.
[ View ]

Comments

This should have a t() function around the text in the h2s.

StatusFileSize
new2.16 KB
Failed: 10613 passes, 0 fails, 328 exceptions
[ View ]

Right. Forgot about that. This one should be better, although it means bringing the text into the template.php file too.

Issue tags:+accessibility

How does the h2 fit in with content headings? Is offscreen a class on interest outside Garland? When users enter content in a node, do we recommend starting headings at h1 or h2?

The h2 issue is something I can understand and explain to theme developers. The offscreen issue is something I do not understand yet and would really like to see an Offscreen for Dummies explanation, something I can pass on to theme developers and tell them they will not be paid until their theme is compliant.

I demo themes at http://D-theme.com. There are an assortment of validator links for WCAG and US 508. Some of the validators allow multiple h1, which means the user could start content with a h1, but placing a h1 in a page after a lower level heading should break the nesting of headings. Has anyone found the definitive W3C statement on single or multiple h1?

The W3 recommendations for uses of headings do not state if the order of headings should be evaluated based on the order in the content or the order displayed. One of the examples suggests it should be order displayed because the example would not pass a test based on order of supply.

What happens with nodes that have the title presented as h2? Does the content start at h2 or h3? Should node or content titles use offscreen? Whatever we apply to themes, there should be a page to guide content creators so they can create content that does not break validation.

If the offscreen class is applicable to other themes, how would a theme developer know what to do?

I like publishing the tests applied to these type of changes. Tell people "Drupal pages are tested in browsers x, y, z, and validated using a, b, c". Module and theme creators can then run their own tests before publishing their projects on drupal.org. People developing private modules and themes for customers can run the same tests. We will see fewer complaints about "Drupal" not complying when people can read about the tests performed. Is there a test covering the offscreen issue?

Whoops, I probably ran off topic but I would like to know how this change can apply to other themes.

Content headings should always be H1 I believe.

I would like offscreen to be a standard class within modules/system/system.css

I don't know about the content within the body of the node. Probably H2 or H3. Definitely would like some more guidance here from people more experienced with screen-readers. Might also make sense to disable H1 from the tinymce or fckeditors so that it isn't a default option.

The offscreen class I'm proposing is used to hide text for sighted users, but display it to screen readers. There are a number of places where it is now a best practice recommended by WCAG 2.0 to move content offscreen using CSS. Using display:none; (as is presently being used by the collapsible fieldsets is a problem because that is generally hidden to screen readers and there isn't a way for those browsers to expand the content. In this case we're proposing adding in H2 tags so that menu items are easier to find, identify & navigate. So lists of related links, aren't just links, but are explicitly related links. It is recommended that every set of lists is preceded by a header describing what that list is there for. Often this is obvious for sighted users, so good to just push it offscreen by default.

Would be nice to see a W3C statement on multiple H1 headings for sure. I don't have one though.

Generally you want nodes & content titles to be displayed, so you wouldn't want to use the offscreen class I've proposed. Titles usually help everyone.

And agreed that this does need to be better explained. There are also some SEO concerns with hiding content offscreen. However, WCAG 2.0 was partly authored for Google, so I think they must be taking this into account.

This is the patch I've suggested:
http://drupal.org/files/issues/display_none_plus.patch

It adds the following lines to modules/system/system.css with some inline documentation

+/*
+** Accessible Hiding/Offscreen
+*/
+
+/* Hide content for all users through css */
+.hide {
+ display: none;
+ visibility: hidden;
+}
+
+/* Temporarily remove content, but make visible to screen readers */
+.offscreen {
+ position: absolute;
+ left: -999em;
+ width: 1em;
+ overflow: hidden;
+}

hide should hide for all.

Offscreen should be used for anything that you just don't want to have a sighted person see and that would interfere with the visual design. Full issue with this is here:
http://drupal.org/node/58941

This is mostly an issue of adding in information, H2 headers, that is presently just not outputted to the screen but would be helpful for visually disabled people. Not sure if there is a test to ensure that all dynamic lists have appropriate headers generated. I don't think there are any tests for offscreen. It requires some knowledge of what options are available & what information should legitimately be hidden to all.

Knowing how to apply this to others is definitely good. I was just applying it here as an example.

Mike

@mgifford regarding garland_accessibility.patch in comment #2:

Related terms and related links are not usually considered navigation menus. I would suggest applying this same change to the primary menu, secondary menu, and navigation menu. But I would suggest against applying this change to related terms and related links.

What constitutes a navigation menu? There is a grey line between what is a navigation menu versus just a list of links, but in general navigation menus appear on numerous pages within a site. Navigation menus should be consistently labeled and located in a consistent order relative to other areas of the page. Related terms and related links don't really fit this definition--they only appear when a certain node is displayed, often on only one page.

Here are a few guiding references:

P.S. I see this issue is for 6.x-dev whereas many of the other accessibility issues are for 7.x-dev. Was that intentional? Because the fix for this will change the markup output I would lean towards targeting 7 but not 6 because the markup change might disrupt existing themes and css. If it makes sense, I would be glad to roll a patch for 7. Besides my question above, I support the strategy of using h2 elements offscreen and I feel this is an important accessibility improvement that is very much needed in core.

Version:6.x-dev» 7.x-dev

I had read it as any list should be prefaced with a structural h2 element, so not just navigation elements:
http://www.w3.org/TR/2008/NOTE-WCAG20-TECHS-20081211/H50

I was proposing related terms and related links because they were relatively easy to implement. Primary/secondary menus require some more complex fiddling with the template.php page. Something like this could work, but I'm looking for a resource friendly way to yank out the titles from the array (and not having any luck). Probably just looking at this a bit too late.

$vars['primary_nav'] = isset($vars['main_menu']) ? '<h2 class="offscreen">' . $vars['main_menu'][0][title] . '</h2>' .     theme('links', $vars['main_menu'], array('class' => 'links main-menu')) : FALSE;
$vars['secondary_nav'] = isset($vars['secondary_menu']) ? '<h2 class="offscreen">' . $vars['secondary_menu'][0] . '</h    2>' . theme('links', $vars['secondary_menu'], array('class' => 'links secondary-menu')) : FALSE;

If it's just navigational elements then I'd agree with you, but as I read WCAG 2.0 requirements, they'd want this behavior for all lists of links. So block elements, menus, and maybe even user content if we can wrap a WYSIWYG around that.

I have advanced it for D7, thanks. Would be great for you to roll a new one for consideration. Probably the most important aspect is getting basic support for offscreen elements brought into core. That ties in tightly with these threads:
http://drupal.org/node/58941
http://groups.drupal.org/node/18563

Lots of outstanding controversy on how to best apply this with minimal impact on themes. This is especially complicated with expanding tabs... But I've let myself wonder off a bit.

Mike

Status:Needs review» Needs work

The last submitted patch failed testing.

Issue tags:+CSS

Need to have a logical way to organize the CSS to manage off screen content. Would love to have some CSS folks look over this.

@mgifford: Feedback from 2 accessibility gurus supports the suggestion that Related Links and Related Terms should have h2 headers preceding the UL or OL tags. These aren't strictly navigation, but headers would help accessibility for users with and without screenreaders.

Here's some of the feedback on our Web Accessibility Best Practices listserv:

"If it is a list of links that are related to each other and especially if they dynamically appear on more than one page I would recommend an h2 header with the content 'Related Links'. This provides orientation to people using screen readers of what the links are and keyboard only users who are using browsers that support header navigation can easily get to the related links."

"Header elements are not used only for navigation bars or menus. They are used also to structure a page logically. By providing header elements sections and sub-sections you help screen reader users to understand how the page is structured and what their relationships are. Related Terms or Related Links are a component of the page and without having the necessary header elements, keyboard users including screen reader users won't be able to navigate to that section."

So it makes sense to make this change, and also to put h2 headers on primary menu, secondary menu, and navigation menu. I think you're right that all these changes will greatly benefit by having support for offscreen elements in core, which I support.

StatusFileSize
new1.6 KB
Failed: Failed to apply patch.
[ View ]

Took out css as the css offscreen patch shouldn't be in the theme, but in the system.css file.

Issue tags:+style-rtl;arabic, +Farsi

Hi,

First, a big ++1 for this feature request!

However, there is a potential right-to-left-language issue, for languages like Farsi - in Internet Explorer 6 (and 7? - test).

In IE6 in RTL mode, text that is hidden off screen to the left will appear a long way to the right, with a long horizontal scroll bar.

I had to fix a similar bug with an 'offscreen' style rule for Moodle 1.6,

So, in my humble opinion it's better to use "top", not "left", eg.

.offscreen {
  position: absolute;
  top: -999em;
  height: 1em;
  overflow: hidden;
}

I'll do more testing with IE7, etc.
And I'll try to find time to review your big display_none_plus.patch (it may be better to split it into 3 separate patches - easier to get it into D7.)

I hope this helps. Yours,

Nick
http://twitter.com/nfreear

So great to see a Farsi thread in Drupal. Ages ago I was involved in getting Farsi collation into MySQL so have an affinity for the language. Glad you brought up the RTL stuff, not sure how that slipped my mind. So yes, makes sense to go up, way up. Also like seeing work done with other projects. Moodle's especially good since it works with Drupal. Documentation like this in the system.css is great too. Definitely this needs to be there to ensure that designers are reminded who the user is and what kind of content you would want to remove from the screen.

/*Accessibility: Skip block link, for keyboard-only users. */
a.skip-block, a.skip {
  position: absolute;
  top: -1000em;
  font-size: 0.85em;
}
/*Accessibility: text 'seen' by screen readers but not visual users. Fixed for RTL languages, example Farsi. */
.accesshide {
  position:absolute;
  top:-100000px;
  left:10px;
  font-weight:normal;
  font-size:1em;
}

Splitting my larger patch into smaller ones would be appreciated. I've been tossing up a few of them, but many of them are dependent on some core approval of better css for accessibility in the core system.css files.

Mike

Assigned:Unassigned» mgifford
Status:Needs work» Needs review

The most recent patch - garland_h2.2.patch - doesn't specify the css so I'm submitting it for review again.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.6 KB
Failed: Failed to apply patch.
[ View ]

Retrying patch

The -999em offset smells of hack.

Surely this is what media types are for: Just put a display:none; in a screen-specific stylesheet so it won't be displayed on that medium, but will still be present for screenreaders.

The patch doesn't actually change the CSS yet, so leaving at NR. This is just a suggestion for the patch that will add .offscreen to CSS.

This patch is related to this one about the CSS display;none; and this one about Skip Navigation. There are a few other related issues that have also been discussed in the Groups Pages on Accessibility.

Unfortunately the media types option you've outlined aren't viable. That's not how the screen reader technology works, even if perhaps it should be. There are a lot of things I think could be done better with screen readers, but we don't have any real influence there. There are lots of documentation links on this, even in this issue thread.

Having an off-screen or 0 height option seems to be the best options to resolve this.

If re-rolling this patch with a screen-reader accessible CSS will get the ball rolling one one of these issues, I'll certainly do that.

Arancaytar, will you give it a +1 if it's got a workable CSS solution?

Mike

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Component:Garland theme» theme system
Status:Needs review» Needs work

This is something on our to-do list at Palantir: accessibility headings for page sections. So +1 to the idea.

Where's the accompanying CSS supposed to go? Also, where's the latest version of it? Is it using a top offset or a left/right one?

Also, this issue isn't limited to just Garland, but to all the core styles/tpls.

@21, John, the proposal is to put the CSS into system.css as a set if generic hide/remove/reveal type classes - see http://drupal.org/node/473396

I'm still dubious about;

+  $vars['related_terms'] = t('related terms');
+  $vars['related_pages'] = t('related pages');

To my thinking "Related Pages" sounds like related content, and "Related Terms" would mean a term related to the current term, however we are viewing a node. Maybe I am being picky?

What about "Tags" and "Links"?

And should we do the breadcrumb?

I can write the patch and test with position absolute top, nothings coming into focus so no probs with scrolling issues.

Status:Needs work» Needs review
StatusFileSize
new2.39 KB
Failed: Failed to apply patch.
[ View ]

Patch for testing, adds headings to Garland node terms and links, and a generic "invisible" class to system.css, which is creeping into #473396: Defining System-Wide Approaches to Remove, Make Invisible & Push Content Off-screen with CSS so that needs discussion.

The invisible class uses position: absolute; top: -9999em; anything using left or right created horizontal scroll bars in RTL for not only IE but Firefox as well.

Status:Needs review» Needs work

Since only screen readers are going to see this, I'm thinking that adding a more descriptive title might be useful. Although, I'm not sure how well "Other items with the same tag" or "Links to related nodes" would work. I'm not stuck on the idea the phrasing though.

I'd like to see H2 tags for all list items including the primary/secondary menus. I think the navigation may be settled however as I believe the blocks give enough context.

Thanks again for putting up patches! Seems like there is a growing consensus on this (and other accessibility issues too).

Mike, a screen reader will "sound" like this:

"heading level 2, Tags, item list, link, not visited, Read More...".

If you use a screen reader its very obvious that this is list of tags. To my thinking tags is web vernacular used everywhere - thoughts from screen reader users welcome! Links are way more problematic, how to describe them has me horse whipped.

The major problem with applying h2 headings to primary/secondary in Garland is that we bork proper nesting, we'd have to get radical with the content source order and positioning of the primary/secondary links, which seems a bit of nightmare in Garland.

Maybe then the best approach is to break up this patch and add in the heading about the tags.

Assuming we get that into core and get the methodology worked out to hide/offscreen the text we can then look at more complicated issues like how to apply meaningful text to the other lists that are piped through the page.tpl.php file.

Also, in terms of nesting, you're talking here of semantic nesting of heading tags rather than CSS nesting, right?

Mike

Mike, whats your take on headings, if we place them on primary/secondary then h2 will be before h1 in the source order - problem or no problem?

I'd actually like the expand this patch and take in comment.tpl.php links as well.

Lets get these proof of concept patches out for wider testing, worry about the actual wording as we go.

Looking for feedback on an alternative idea.

This idea to code headings into the templates feels all a bit hacky to me and not very drualish. Tonight I messed around with theme_links and found it was very easy to modify it to work more like theme_item_list, ie:

/**
* Add two new parameters.
* $title The title of the list.
* $type The heading level to return (e.g. "h3", "h4")
*/
function theme_links($links, $title = NULL, $type = NULL ,$attributes = array('class' => 'links')) {
  global $language;
  $output = '';
  if (count($links) > 0) {
    // wrap the list in a div
    $output .= '<div class="link-list">';
    // conditionally output the title wrapped in heading tags
    if (!empty($title)) {
      $output .= "<$type>" . $title . "</$type>";
    }
    $output .= '<ul' . drupal_attributes($attributes) . '>';
  etc...

And for example in node.module...

function theme_node_links($element) {
  return theme('links', $element['#value'], $title = t('Links'), $type = 'h3', array('class' => 'links inline'));
}

Or in page.tpl.php, suppress the heading....

<?php print theme('links', $main_menu, NULL, NULL, array('id' => 'main-menu', 'class' => 'links clearfix')); ?>

If we got the CSS right we'd avoid breaking themes too badly, if at all.

Thoughts?

http://d-theme.com/garland_accessible uses the Garland from Drupal 6.12 plus garland_headings_364219_v1.patch. Go screen read. See if you can find the invisible text.

@pertex, this is Drupal 7. Apply the patch to head, you wont be disappointed again:) I can see in your patched D6 version the headings are empty, so likely the patch applied to node.tpl.php but failed for template.php. In any case this is proof of concpet stuff as far as I am concerned (though perfectly workable).

Lets talk about #28 more:)

Ok, this would be a cleaner way to do this for sure. Would allow us to more easily extend this type of logic to the many other lists that Drupal generates.

So for a reference to the API -> http://api.drupal.org/api/function/theme_links/7

The current list of parameters is:
function theme_links($links, $attributes = array('class' => 'links')) {

You've suggested adding title & type:
function theme_links($links, $title = NULL, $type = NULL ,$attributes = array('class' => 'links')) {

However this will mean we'll have to change all of the references to theme_links as the attributes will be out of order.

It's not good to mess with the drupal_attributes function. Not a good practice though to just add the title & type to the end of the array I suppose:
function theme_links($links,$attributes = array('class' => 'links'), $title = NULL, $type = 'h2' ) {

I'm also in favour of defaulting the type to h2 so that if a title is present it is never <>

I think this has a lot of potential!

I'll write a proper patch to get the attention of more reviewers.

@jmburnz

I love the solution that we've come to (#28). You asked in #27 whether it was acceptable for h2 headings on primary/secondary links to appear before the h1 in the source code. This is what I see most often, and the W3C WCAG 2.0 specifically includes an example of that:
http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H42.html#H42-ex3

@mgifford

31. I agree about keeping attributes in the same order and defaulting to h2.

Let me know how I can help get this in before code freeze.

This patch was held up by the invisible/hidden class patch, now we can move ahead with this, needs some thought because theme links gets called in different context for the same item, for example on node teasers and full node view, so the heading level needs to be contextually aware which needs some thought.

I like the patch in #28, but have a few comments.

1. The function definition definitely needs to reposition the two new params to the end of the param list.

2. $type needs to be more specific (type of what?)

3. It should be possible to have the heading either displayed, or invisible

5. I am opposed to providing a default heading level as this can break as opposed to improve accessibility)

6. I believe that it would be possible to determine context for heading level by having any function, like node_page_default() that outputs multiple nodes setting a variable that can be checked by theme_preprocess_page and converted into a theme variable that can be checked with logic

E.g. (pseudo code)

if $nodes-multiple
  $heading-level = 2
else
  $heading-level = 1

1. I'm not convinced of the reasons for this, consistency is better imo; with other theme functions attributes are always last.

2. $type could be $heading_level, or just $heading

3. theme/css task, will only need to account for Garland in this patch afaikt, Stark should let them show imo.

5. there is not default, $type = NULL, implementations can do what they will with $type, see next...

6. $page and $teaser, handle this in template_preprocess_node.

Quick resolutions and zero scope creep people, anyone want to roll a patch be my guest, my schedule is crazy!

I think this is very clean way to solve the issue imo, as opposed to hard coding headings into templates which is a bit ugly.

@jmburnz

I started rolling a patch and found there are roughly 8 calls in core to theme_links() or theme('links'). So we should include the new function plus the 8 updated calls all in the same patch, right?

I also realized that a corresponding change should probably be made to theme_item_list(), which is called from 17 different files in core, and theme_menu_tree() too. This is probably what you meant by 'scope creep'. So do you think we should just patch theme_links for now?

In the long run it would be beneficial to examine all of these lists and determine which are an 'important section of content' or a navigation bar that, according to the W3C WCAG 2.0, should have a heading.

All 8 calls need to be accounted for, and theming Garland, we have to decided on a call by call basis which attributes and heading level is used (or NULL if that makes sense).

I would roll a proof of concept patch quickly so we can start testing it and give feedback + get it past the bot.

theme_item_list could go this way also, to have the heading level as a param, that change could be easier if we let it default to h3? I think it should be a separate issue actually, while this is not a big change it might be easier to get this in if we focus on the link lists only?

We really need a patch asap so it goes on the radar for webchick et al.

Status:Needs work» Needs review
StatusFileSize
new4.6 KB
Failed: Failed to apply patch.
[ View ]

A patch is born. Please let me know how I can improve it and whether t() and check_plain() are good. Note:

  • in includes/theme.inc, theme_links() has one new parameter, $header = array()
  • the header must have a title and type (e.g., "h2" or "h3") to appear, and can have an optional class
  • Garland stays visually unchanged for sighted users, because themes/garland/template.php assigns class 'element-invisible'
  • on the contrary, as suggested above, in Stark there are now headers on the Main menu and Secondary menu
  • the rest of the theme_links() calls worked fine with no change because the $header param is optional
  • later this logic could be expanded to theme_item_list(), but for now this simple patch will provide a huge boost for screen-reader and keyboard only users

@brandonojc

Excellent work rolling this patch, a couple of comments.

1. I would recommend that the $header array be renamed to $heading, as this is a heading, not a header.

2. I would set the class option to classes (allowing for multiple classes, and possibly make this an array.

3. Where you are testing for isset on title and type, I would test for not empty() on the title and possibly test for h1-h6 on the type. If not testing for h1-h6 on the type I would test it for not empty as well.

@brandonojc

Was thinking about it in the night, yes I dream Drupal.

1. $heading['title'] could be renamed to 'text' as this is the text for the heading, and not really a title of anything.

2. $heading['type'] could be renamed to 'level' requiring to be set to 1 - 6, very easy to test for and more semantically correct, we are asking what level of heading to create, not what 'type' of heading.

Everett, the classes are in the attributes array, the class attribute taking a space separated list of classes. That's pretty much how other similar functions work and it works really well.

From my quick glance it looks good, and I cant see why we need the suggested !empty, the patch tests if the params isset, which seem appropriate.

+1 heading level and heading text, seems to make more sense.

Good effort, will test later today, need to refresh my aged dev environment....

@jmburnz

Good to know on classes, if this is the way that other similar functions deal with this then it's good.

As for !empty() vs. isset(), empty checks for everything that isset checks for, with the exception that empty also ensure that the variable isn't an empty string ''.

But yes, this was a good patch all in all.

StatusFileSize
new4.87 KB
Failed: Failed to apply patch.
[ View ]

Please review this improved patch and suggest any further improvements. Changes include:

  • Renaming parameters to heading, text, and level as suggested in 40 and 41.
  • Using !empty() as suggested. We do not output a heading with empty text or empty level.
  • Adding documentation that multiple classes can be space-separated.
  • Documenting references about proper use of headings and avoiding display:none.

Remember, this patch uses best practices by adding headings above navigation menus, but it does not visually impact Garland/Minnelli which adds class 'element-invisible'. In Stark the headings appear on screen as discussed above.

Works as desired and expected for me. Thanks.

@brandon

+1 looks good.

Only recommendation would be to modify:

isset($heading['classes']) to !empty($heading['classes']) so that an empty class attribute isn't output.

StatusFileSize
new4.87 KB
Failed: Failed to apply patch.
[ View ]

Rerolled patch from #44 with suggestion in #46. now checks to make sure that $heading['classes'] is not empty instead of isset.

Status:Needs review» Reviewed & tested by the community

Looks good then!

Looks good to me too. It's great working with you folks.

Status:Reviewed & tested by the community» Fixed

This looked good to me. Great work.

We'll want to update the upgrade instructions in the handbook. Please mark as 'fixed' once this API change was documented.

Thanks!

Status:Fixed» Needs work
Issue tags:+Needs Documentation

Tagging.

Status:Needs work» Fixed

Where can I contribute to this upgrading section of the handbook?

Here is draft documentation:
http://www.ojctech.com/blog/upgrading-drupal-7-themelinks

I would appreciate any feedback and suggestions.

Status:Fixed» Needs work

(accidently changed status)

hi, brandon!

All of the API changes between Drupal 6 and Drupal 7 are in a big list over at http://drupal.org/update/modules/6/7. Updating this page involves adding a new link to the bottom of the table of contents and the documentation to the bottom of the page.

However, you've kind of gone over and above what most API change documentation shows, and are making recommendations to module developers on how to create accessible output which is GREAT! I'd encourage you to coordinate with the documentation team on the best place to put that documentation in the developer guide, because I think it makes sense in both places.

It seems that the changes in theme_links() were not commited and it produces bad XHTML :

<ul text="Main menu" level="h2" class="element-invisible"><li class="menu-142 first last"><a href="/drupal-HEAD/admin/structure/menu-customize/main-menu/add">Add a main menu link</a></li>

I noticed it because "text" and "level" are not part of the used XHTML DTD ;) I don't know if it's due to an oversight when committing of if the changes were erased by the next commit (which also modified theme_links()... for other reasons). Anyways the patch might need a reroll due to this :S

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.26 KB
Failed: Failed to apply patch.
[ View ]

Please find a new 'repair' patch. This patch re-applies the changes to theme_links() which apparently were not committed. The patch keeps the changes made by the next commit intact. I have set this to RTBC, but please advise if there is a different protocol for this situation.

@TheRec

Thanks for letting us know that the commit somehow did not include the theme_links() changes.

No problem, I wish I would have had the time to re-roll but I should have read everything to be sure not to do anything not reviewed and I was somewhat busy with other patches :) Good luck with this one !

Status:Reviewed & tested by the community» Needs work

Thanks, committed!

Back to needs work for docs.

A minor detail, in function header :

  *    - class: (optional) space-separated classes for the heading

This should be "an array of classes for the heading", now that #326539: Convert 'class' attribute to use an array, not a string is in core.

Now that the modifications of theme_links() are commited, it seems that all the $attributes passed are ignored (was this even tested !?) in the current calls to this function which existed in the core before the function modification. This can be remarked because the CSS ID's of toolbar are not set anymore and thus the "Hello, " and the "Logout" link are badly positioned. This is due to the fact that you introduced a new parameter ($heading), in the middle of the others with your patch. You should either place this parameter at the end of the function (which is the "easy" solution), or modify every call to this function, direct or indirect... for the toolbar it is indirect as far as I can see... this would be the clean way, the logical order of this parameter is the one you chose, but it requires a little bit more work.

I think this whole patch was not tested enough before being marked RTBC :(

Priority:Normal» Critical

This broke invocations of theme_links() via Drupal's rendering API. There are at least two calls to this via '#theme' => 'links' in toolbar.module which break toolbar theming.

Assigned:mgifford» TheRec

I'm working on a fix.

Assigned:TheRec» mgifford

To be more precise/helpful, the problem is that toolbar module tries to set non-default attributes and the IDs do not end up in the arguments passed to theme_links().

<?php
  $build
['toolbar_menu'] = array(
   
'#theme' => 'links',
   
'#links' => $links,
   
'#attributes' => array('id' => 'toolbar-menu'),
  );
 
// Add logout & user account links
 
$build['toolbar_user'] = array(
   
'#theme' => 'links',
   
'#links' => array(
     
'account' => array(
       
'title' => t('Hello <strong>@username</strong>', array('@username' => $user->name)),
       
'href' => 'user',
       
'html' => TRUE,
      ),
     
'logout' => array(
       
'title' => t('Logout'),
       
'href' => 'user/logout',
      ),
    ),
   
'#attributes' => array('id' => 'toolbar-user'),
  );
?>

No id's given to the links containers after this patch was committed.

Priority:Critical» Normal
StatusFileSize
new1.59 KB
Failed: Failed to apply patch.
[ View ]

Ok, this should be it... I updated the theme registry declaration in includes/common.inc, and search every files in drupal for calls to theme('links', ...). There was one left that should have been modified too.
I did not update the calls made through the theme registry, like the one Gábor Hojtsy talks about in #62... they should be OK now that the theme registry is updated... but I am not sure that it is recommanded, not to specify an empty argument when it is in the middle of the list of parameters.

P.S.: This patch should be reviewed and tested correctly before it might get marked as RTBC... as far as I know this does not break any functionality in HEAD so we have the time to fix it... it only "breaks" the appearance... let's not rush this again ;)

P.S.S. : Do not forget to Clear your cache or resubmit the form under "Appearance" after applying the page, you have to rebuild the theme registry for it to work.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Those tests all pass locally... let's retry.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Test bot #34 is down after reporting the problem (Thanks Damien)... let's retry :D

It seems like the heading parameter will be used less than the attributes so I'm surprised it was placed ahead of it. Any chance we can change that as part of the follow up?

@TheRec

The patch in 63 works for me. I agree with you in 59 that the function documentation of the class param should say "an array of classes for the heading". When I was rolling these patches I didn't know #326539 was coming, so I appreciate all your quick help and I'm glad to do whatever is needed to make things right.

StatusFileSize
new7.35 KB
Failed: Failed to apply patch.
[ View ]

I also root for moving this parameter at the end of the list of parameters. So here's a patch... I also cleaned the way $vars['primary_nav'] and $vars['secondary_nav'] were constructed. Going from :

  $vars['primary_nav'] = isset($vars['main_menu']) ? theme('links', $vars['main_menu'], array(
    'text' => t('Main menu'), 'level' => 'h2', 'class' => 'element-invisible',
  ), array('class' => 'links main-menu')) : FALSE;

To
  if (isset($vars['main_menu'])) {
    $vars['primary_nav'] = theme('links', $vars['main_menu'],
      array(
        'class' => array('links', 'main-menu'),
      ),
      array(
        'text' => t('Main menu'),
        'level' => 'h2',
        'class' => array('element-invisible'),
      )
    );
  }
  else {
    $vars['primary_nav'] = FALSE;
  }

Which is think is way more readable.

Also I updated the theme_links() function header : moved the order of parameters and corrected the $heading['class'].

StatusFileSize
new7.56 KB
Failed: Failed to apply patch.
[ View ]

@TheRec

Here is a revised patch with only 1 small change, namely to make class an array in modules/system/page.tpl.php. The prior patch at 71 had:

+        <?php print theme('links', $main_menu, array('id' => 'main-menu', 'class' => array('links clearfix')), array('text' => t('Main menu'), 'level' => 'h2')); ?>

The revised patch has:
+        <?php print theme('links', $main_menu, array('id' => 'main-menu', 'class' => array('links', 'clearfix')), array('text' => t('Main menu'), 'level' => 'h2')); ?>

Otherwise this patch is identical to 71 (any other differences are only in timestamps and because I used the -p flag to cvs diff). Everything else tests out perfectly: the toolbar-user is fixed, the headings for accessibility work in Garland/Minnelli and Stark as planned. Thanks for your energy on this.

Status:Needs review» Reviewed & tested by the community

Patch applies nicely. Spits out a nice about the main menu -->

Main menu

This looks ready for me.

Status:Reviewed & tested by the community» Needs work

+++ modules/node/node.admin.inc 24 Aug 2009 16:39:06 -0000
@@ -373,7 +373,7 @@ function node_admin_content($form_state)
-    '#markup' => theme('links', array(array('title' => t('Add new content'), 'href' => 'node/add')), array('class' => array('action-links'))),
+    '#markup' => theme('links', array(array('title' => t('Add new content'), 'href' => 'node/add')), array(), array('class' => array('action-links'))),

I'm quite sure that this is wrong.

Guys, can you please go easy on the RTBC flag, and not actually mark patches RTBC until they are, in fact, R and T? If I can't trust the reviewers have done a thorough job on a patch, I need to do it myself, and that will drastically reduce the number of patches that can squeeze in before freeze.

This review is powered by Dreditor.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new6.52 KB
Failed: Failed to apply patch.
[ View ]

webchick spotted a mistake that was breaking the "Add new content" link on /admin/content, thanks to her. This was due to a leftover in the previous patch, before I moved the $heading parameter to the end. There is no change in functionality, we are just modifying one less theme('links',...) call. Here's the corrected version.

Status:Reviewed & tested by the community» Needs review

Cross-post.

Sorry for jumping the gun. on the RTBC. I'll review it more carefully in the future.

The patch in 75 works for me. I reviewed the code and all the class array changes look right. I tested these scenarios:

  • The headings for accessibility are still there (invisible in Garland and Minnelli, visible in Stark).
  • The toolbar-user Hello and Logout links are in the right position.
  • The add new content link on /admin/content now works too.

Let me know how else I can help.

Have you tested in Seven (the admin theme?).

Good question. I downloaded a fresh version of the CVS, applied the patch & the admin screen looked fine.

The main_menu & secondary_menu menu's don't seem to be used in Seven, so there should be no need to migrate the changes in Garland over to Seven.

I can't see how these changes would affect Seven, but it is good to verify.

Mike

I tested a handful of the /admin pages using Seven and everything functioned normally. In Seven this patch fixes the toolbar-user Hello and Logout links, putting them back in the correct position at the top right. Thanks again for the help, folks.

Status:Needs review» Reviewed & tested by the community

Ok, let's get this one in then.

Status:Reviewed & tested by the community» Needs review

Gábor Hojtsy> If you mean try the new theme_link() implementation, yes, it is used in Seven, but not for primary and secondary menu (since they are none), it is used to themes link lists as used in the toolbar or the "Add new content" list on /admin/content. Right now the $heading parameter is not used there for the moment, but this could be implemented in another issue (you could even suggest it in #540282: Accessibility improvements for the toolbar, where the main goal is to improve toolbar accessibility).

Status:Needs review» Reviewed & tested by the community

Cross post. My bad.

Status:Reviewed & tested by the community» Needs work

Ok, great. Thanks, folks!

Committed to HEAD. Back to needs work for docs.

Something else that occurred to me while reviewing this patch is it might make sense for the API to accept either a string for header or an array of values, and default to something sensible (like h2). Sort of how theme_table()'s data values work.

So in other words, both of these would be acceptable, and result in the same output:

<?php
print theme('links', $secondary_menu, array('id' => 'secondary-menu', 'class' => array('links', 'clearfix')), t('Secondary menu'));
print
theme('links', $secondary_menu, array('id' => 'secondary-menu', 'class' => array('links', 'clearfix')), array('text' => t('Secondary menu'), 'level' => 'h2'));
?>

That's great news! Nice to have this committed.

I do think that this extension would make sense. Having a system wide default for menu headings is probably a good idea, particularly if we can come to some consensus on how the community recommends dealing with headers.

We'll focus on getting someone to add the documentation in September if that's alright.

Mike

Thanks, everyone!

Regarding documentation, here is a draft that I plan to revise and contribute as suggested:
http://www.ojctech.com/blog/upgrading-drupal-7-themelinks

I would appreciate any feedback and ideas. Thanks!

Status:Needs work» Needs review
StatusFileSize
new3.3 KB
Failed: Failed to apply patch.
[ View ]

I implemented webchick's suggestions to improve theme_links(). It will simplify some calls of this function (namely 2 that are currently in core, which are updated with this patch too). I also updated the function header comments accordingly.

I also modified :

        $output .= ' ' . drupal_attributes(array('class' => $heading['class']));

To remove the prepended space which is useless since drupal_attributes adds it already (Yes yes... I'm the one creating issues for #555830: Clean up theme_image() to use drupal_attributes() for all attributes and revisit defaults for "alt" and "title" and I also sometimes create those "bugs" myself :P ).

"space-separated classes for the heading" - I'm sorry, but didn't D7 switch to putting multiple classes into an array rather than a string?

Arancaytar> Where do you see this ? It was changed in #74 as far as I know...

Only place I would agree it was not change is:

  *   consistently appears on multiple pages. To make the heading invisible
  *   use class => 'element-invisible'. Do not use 'display:none', which

It should be class => array('element-invisible');... but I would better prefer something like "use the 'element-invisible' CSS class." which would make it less dependant on the way classes are set by Drupal.

The patch in 88 looks good. I reviewed the code and tested to confirm the following:

  • The headings in Garland/Minnelli and Stark all work correctly. They are invisible in Garland/Minnelli and visible in Stark.
  • The theme_links in Seven still work (and don't have a heading, which is the correct behavior).
  • This patch makes no visual changes (but it sure simplifies the API).
  • A space is indeed provided by drupal_attributes(), so the h2 tag with class attributes works correctly.

Also, I confirmed that "space-separated classes for the heading" is no longer in core nor in this patch.

@TheRec, thanks for the good work on this. The documentation change you suggested in 90 is also good.

StatusFileSize
new3.6 KB
Failed: Failed to apply patch.
[ View ]

The function header is modified as per #90, the rest is unchanged.
Thank you for the testing and reviewing.

+1 this new patch is looking good to me.

I've applied this theme here - http://drupal7.dev.openconcept.ca/

Looks good to me. The site above is mostly a list of the remaining pre-code freeze critical accessibility issues.

Status:Needs review» Reviewed & tested by the community

Setting RTBC. The patch preserves all the functionality and the headings for accessibility. There is no visual change or HTML output change in Garland, Minnelli, Stark or Seven with this patch. The API just gets simpler and the code comment is more accurate. Thanks, everyone!

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

setting back to needs review to see if previous patch can pass testing.

HEAD is broken at the moment... ;) Wait before asking for re-test.

The tests pass again now that HEAD is fixed. I'm ready to put this back to RTBC.

Status:Needs review» Reviewed & tested by the community

Setting back to RTBC as in comment 96. Broken head broke the patch, it is passing testing now that head is fixed.

Status:Reviewed & tested by the community» Needs work

Committed to HEAD with a small whitespace cleanup! Thanks!

One last update to the docs and we can mark this sucker fixed. :)

Thanks! Yes, I have a draft of the docs written:
http://www.ojctech.com/blog/upgrading-drupal-7-themelinks

It will need a few updates with the simpler API and I'll submit it to the API update documentation area at http://drupal.org/update/modules/6/7. Thanks, everyone!

Category:feature» bug

Note that brandonojc has added suggested doc for the module 6/7 update page at http://drupal.org/node/583126#comment-2064746

I am closing that other issue as a duplicate of this one, since I think the discussion belongs here.

My comment on the suggested doc was that it seems excessively verbose compared to the other entries on the module update page, and I wasn't sure whether all of that explanation was really necessary. Someone needing to update their module from D6 to D7 is going to have a LOT to do, and probably needs a concise explanation? But if everyone else thinks that version is fine, someone (such as myself) with doc admin privs can add it to that page.

Everett Zufelt commented that the doc should be added to the theme update page as well.

Please continue discussion here...

I am also setting this now to a bug report, since the doc needs to be updated (it is a bug that the doc is not updated yet), and the feature has been committed.

The documentation page is a wiki, so we can just finalize it there. However, for the sake of putting up a first draft for someone else to improve.

31. List items should normally be preceded with a headings of the appropriate level.

To meet WCAG requirements headings should be used before all list items to ensure that there is a quick way for assistive technology users to browse through the content, using semantic headings to better group sets of lists. Visually most people can get a sense of these groups by how they are arranged visually on the page.

Now assigning an arbitrary heading level like <h2> might be alright for some sites, it does present a challenge if it breaks the heading hierarchy which can make it potentially less meaningful. Fortunately the new theme_links() function assesses if a heading level has been previously set and ensures that this is nested properly.

The end result is that putting the following into your page.tpl.php file:

<?php print theme('links', $secondary_menu, array('id' => 'secondary-menu', 'class' => array('links', 'clearfix')), t('Secondary menu')); ?>

will result in a semantically correct and accessible secondary menu because an invisible heading has been added:

<h2 class="element-invisible">Secondary menu</h2>

For more information on this topic and Drupal accessibility.

Can you edit your suggestion -- I think some of the tags you meant to be shown literally were interpreted as actual tags, and it is hard to figure out what you were trying to say?

that should be better.

Hmmm.

I like the doc in #104 pretty well...

It may have a problem though. Isn't the $header input to theme_links() supposed to be an array? It looks like you are just passing in the title of the proposed header, and not the whole array? I see that the theme_links() function will take care of it, but maybe an example showing the other array components would be more complete?

Also, looking at this, it looks good for the theme update guide. For the module update guide, maybe a simple page saying there's a new argument to theme_links() might be enough, with a link to the accessibility information? Thoughts?

Sounds good to me. Can you take those suggestions and add them to the upgrade pages?

Assigned:mgifford» jhodgdon

OK, I'll take care of it, but not today (teaching a beginner Drupal workshop). Early next week.

Status:Needs work» Needs review

I have documented this change... review would be good!

Module upgrade guide: http://drupal.org/node/224333#theme-links-param
Theme upgrade guide: http://drupal.org/update/theme/6/7#theme-links-param

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Of course, Mr. Search Bot! The last patch has been committed, so of course testing fails.... I was asking for a review of pages on drupal.org... :)

@jhodgdon

Thanks for posting this! I only have 2 small suggestions:

  1. Consider showing the full-length form of the $heading parameter. So instead of the 3rd argument being just t('Main menu') we could show array('text' => t('Main menu'), 'level' => 'h2', 'class' => array('element-invisible')).
  2. Add a reference link to the Accessibility section of the Theming Guide: http://drupal.org/node/561750

Thanks very much for taking the time to get this posted and supporting this accessibility work!

Thanks for the suggestions! I've made some updates to both pages -- please review again.

Both look good. I do like how you've added a reference to WCAG. I just added links to it.

Status:Needs review» Needs work

Hmmm...

So previously, I had just taken what mgifford and brandonojc had written for the doc and put it up there with a little bit of prose editing... But as I was giving this issue one final review before marking it fixed, I took a closer look at the current function code... http://api.drupal.org/api/function/theme_links/7

It looks like the function doesn't do anything to handle nesting (it just uses an H2, no matter what, if a string is passed in for the header), and it looks like the function doesn't add a default CSS class if a string is passed in for the header.

So I have revised the doc again.

Status:Needs work» Needs review

Sorry, didn't mean to change the status

@jhodgdon

Both docs look great to me. You're correct on both counts: the nesting level (h2, eg) is not automatically computed, so it depends on the coder to pass in the right level as well as the right CSS. The docs look good to go. Thanks again!

Status:Needs review» Fixed

OK, I'll (at least tentatively) mark this fixed then.

Status:Fixed» Closed (fixed)
Issue tags:-CSS, -Needs Documentation, -accessibility, -style-rtl;arabic, -Farsi

Automatically closed -- issue fixed for 2 weeks with no activity.