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:
- http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H42.html#H42-ex1
- http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H69.html#H69-ex2
- http://www.dhs.state.il.us/IITAA/IITAAWebImplementationGuidelines.html#w...
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.
Comment | File | Size | Author |
---|---|---|---|
#92 | 364219-fix-theme-links_6.patch | 3.6 KB | TheRec |
#88 | 364219-fix-theme-links_5.patch | 3.3 KB | TheRec |
#75 | 364219-fix-theme-links_4.patch | 6.52 KB | TheRec |
#72 | 364219-fix-theme-links_3.patch | 7.56 KB | bowersox |
#71 | 364219-fix-theme-links_2.patch | 7.35 KB | TheRec |
Comments
Comment #1
johnbarclay CreditAttribution: johnbarclay commentedThis should have a t() function around the text in the h2s.
Comment #2
mgiffordRight. Forgot about that. This one should be better, although it means bringing the text into the template.php file too.
Comment #3
mgiffordComment #4
peterx CreditAttribution: peterx commentedHow 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.
Comment #5
mgiffordContent 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
Comment #6
bowersox CreditAttribution: bowersox commented@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.
Comment #7
mgiffordI 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.
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
Comment #9
mgiffordNeed to have a logical way to organize the CSS to manage off screen content. Would love to have some CSS folks look over this.
Comment #10
bowersox CreditAttribution: bowersox commented@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:
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.
Comment #11
mgiffordTook out css as the css offscreen patch shouldn't be in the theme, but in the system.css file.
Comment #12
nfreear CreditAttribution: nfreear commentedHi,
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.
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
Comment #13
mgiffordSo 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.
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
Comment #14
mgiffordThe most recent patch - garland_h2.2.patch - doesn't specify the css so I'm submitting it for review again.
Comment #16
mgiffordRetrying patch
Comment #17
cburschkaThe -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.
Comment #18
mgiffordThis 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
Comment #20
webchickComment #21
JohnAlbinThis 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.
Comment #22
Jeff Burnz CreditAttribution: Jeff Burnz commented@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;
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.
Comment #23
Jeff Burnz CreditAttribution: Jeff Burnz commentedPatch 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.
Comment #24
mgiffordSince 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).
Comment #25
Jeff Burnz CreditAttribution: Jeff Burnz commentedMike, 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.
Comment #26
mgiffordMaybe 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
Comment #27
Jeff Burnz CreditAttribution: Jeff Burnz commentedMike, 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.
Comment #28
Jeff Burnz CreditAttribution: Jeff Burnz commentedLooking 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:
And for example in node.module...
Or in page.tpl.php, suppress the heading....
If we got the CSS right we'd avoid breaking themes too badly, if at all.
Thoughts?
Comment #29
peterx CreditAttribution: peterx commentedhttp://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.
Comment #30
Jeff Burnz CreditAttribution: Jeff Burnz commented@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:)
Comment #31
mgiffordOk, 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!
Comment #32
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'll write a proper patch to get the attention of more reviewers.
Comment #33
bowersox CreditAttribution: bowersox commented@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.
Comment #34
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis 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.
Comment #35
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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)
Comment #36
Jeff Burnz CreditAttribution: Jeff Burnz commented1. 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.
Comment #37
bowersox CreditAttribution: bowersox commented@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.
Comment #38
Jeff Burnz CreditAttribution: Jeff Burnz commentedAll 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.
Comment #39
bowersox CreditAttribution: bowersox commentedA patch is born. Please let me know how I can improve it and whether t() and check_plain() are good. Note:
Comment #40
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #41
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #42
Jeff Burnz CreditAttribution: Jeff Burnz commentedEverett, 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....
Comment #43
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #44
bowersox CreditAttribution: bowersox commentedPlease review this improved patch and suggest any further improvements. Changes include:
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.
Comment #45
johnbarclay CreditAttribution: johnbarclay commentedWorks as desired and expected for me. Thanks.
Comment #46
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #47
Everett Zufelt CreditAttribution: Everett Zufelt commentedRerolled patch from #44 with suggestion in #46. now checks to make sure that $heading['classes'] is not empty instead of isset.
Comment #48
mgiffordLooks good then!
Comment #49
bowersox CreditAttribution: bowersox commentedLooks good to me too. It's great working with you folks.
Comment #50
Dries CreditAttribution: Dries commentedThis 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!
Comment #51
webchickTagging.
Comment #52
bowersox CreditAttribution: bowersox commentedWhere 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.
Comment #53
bowersox CreditAttribution: bowersox commented(accidently changed status)
Comment #54
webchickhi, 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.
Comment #55
TheRec CreditAttribution: TheRec commentedIt seems that the changes in theme_links() were not commited and it produces bad XHTML :
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
Comment #56
bowersox CreditAttribution: bowersox commentedPlease 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.
Comment #57
TheRec CreditAttribution: TheRec commentedNo 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 !
Comment #58
webchickThanks, committed!
Back to needs work for docs.
Comment #59
TheRec CreditAttribution: TheRec commentedA minor detail, in function header :
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 :(
Comment #60
Gábor HojtsyThis 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.Comment #61
TheRec CreditAttribution: TheRec commentedI'm working on a fix.
Comment #62
Gábor HojtsyTo 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().
No id's given to the links containers after this patch was committed.
Comment #63
TheRec CreditAttribution: TheRec commentedOk, 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.
Comment #64
TheRec CreditAttribution: TheRec commentedComment #66
TheRec CreditAttribution: TheRec commentedThose tests all pass locally... let's retry.
Comment #68
TheRec CreditAttribution: TheRec commentedTest bot #34 is down after reporting the problem (Thanks Damien)... let's retry :D
Comment #69
drewish CreditAttribution: drewish commentedIt 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?
Comment #70
bowersox CreditAttribution: bowersox commented@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.
Comment #71
TheRec CreditAttribution: TheRec commentedI 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 :
To
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'].
Comment #72
bowersox CreditAttribution: bowersox commented@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:
The revised patch has:
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.
Comment #73
mgiffordPatch applies nicely. Spits out a nice about the main menu -->
Main menu
This looks ready for me.
Comment #74
webchickI'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.
Comment #75
TheRec CreditAttribution: TheRec commentedwebchick 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.
Comment #76
TheRec CreditAttribution: TheRec commentedCross-post.
Comment #77
mgiffordSorry for jumping the gun. on the RTBC. I'll review it more carefully in the future.
Comment #78
bowersox CreditAttribution: bowersox commentedThe patch in 75 works for me. I reviewed the code and all the class array changes look right. I tested these scenarios:
Let me know how else I can help.
Comment #79
Gábor HojtsyHave you tested in Seven (the admin theme?).
Comment #80
mgiffordGood 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
Comment #81
bowersox CreditAttribution: bowersox commentedI 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.
Comment #82
Gábor HojtsyOk, let's get this one in then.
Comment #83
TheRec CreditAttribution: TheRec commentedGá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).
Comment #84
TheRec CreditAttribution: TheRec commentedCross post. My bad.
Comment #85
webchickOk, 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:
Comment #86
mgiffordThat'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
Comment #87
bowersox CreditAttribution: bowersox commentedThanks, 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!
Comment #88
TheRec CreditAttribution: TheRec commentedI 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 :
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 ).
Comment #89
cburschka"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?
Comment #90
TheRec CreditAttribution: TheRec commentedArancaytar> 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:
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.
Comment #91
bowersox CreditAttribution: bowersox commentedThe patch in 88 looks good. I reviewed the code and tested to confirm the following:
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.
Comment #92
TheRec CreditAttribution: TheRec commentedThe function header is modified as per #90, the rest is unchanged.
Thank you for the testing and reviewing.
Comment #93
Everett Zufelt CreditAttribution: Everett Zufelt commented+1 this new patch is looking good to me.
Comment #94
mgiffordI'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.
Comment #95
bowersox CreditAttribution: bowersox commentedSetting 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!
Comment #97
Everett Zufelt CreditAttribution: Everett Zufelt commentedsetting back to needs review to see if previous patch can pass testing.
Comment #98
TheRec CreditAttribution: TheRec commentedHEAD is broken at the moment... ;) Wait before asking for re-test.
Comment #99
bowersox CreditAttribution: bowersox commentedThe tests pass again now that HEAD is fixed. I'm ready to put this back to RTBC.
Comment #100
Everett Zufelt CreditAttribution: Everett Zufelt commentedSetting back to RTBC as in comment 96. Broken head broke the patch, it is passing testing now that head is fixed.
Comment #101
webchickCommitted to HEAD with a small whitespace cleanup! Thanks!
One last update to the docs and we can mark this sucker fixed. :)
Comment #102
bowersox CreditAttribution: bowersox commentedThanks! 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!
Comment #103
jhodgdonNote 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.
Comment #104
mgiffordThe 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:
will result in a semantically correct and accessible secondary menu because an invisible heading has been added:
For more information on this topic and Drupal accessibility.
Comment #105
jhodgdonCan 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?
Comment #106
mgiffordthat should be better.
Comment #107
jhodgdonHmmm.
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?
Comment #108
mgiffordSounds good to me. Can you take those suggestions and add them to the upgrade pages?
Comment #109
jhodgdonOK, I'll take care of it, but not today (teaching a beginner Drupal workshop). Early next week.
Comment #110
jhodgdonI 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
Comment #112
jhodgdonOf 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... :)
Comment #113
bowersox CreditAttribution: bowersox commented@jhodgdon
Thanks for posting this! I only have 2 small suggestions:
t('Main menu')
we could showarray('text' => t('Main menu'), 'level' => 'h2', 'class' => array('element-invisible'))
.Thanks very much for taking the time to get this posted and supporting this accessibility work!
Comment #114
jhodgdonThanks for the suggestions! I've made some updates to both pages -- please review again.
Comment #115
mgiffordBoth look good. I do like how you've added a reference to WCAG. I just added links to it.
Comment #116
jhodgdonHmmm...
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.
Comment #117
jhodgdonSorry, didn't mean to change the status
Comment #118
bowersox CreditAttribution: bowersox commented@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!
Comment #119
jhodgdonOK, I'll (at least tentatively) mark this fixed then.