Accessibility improvements for the toolbar

This patch should address the issues you described. I tested with an open source screen-reader (NVDA) and also with a Firefox extension (Fire Vox. Using navigation I could access the item and use it with the keyboard.
Sorry, I cannot test with JAWS at the moment, my usual test environement is not working right now :/

The screen readers which use the HTML source code before Javascript processing will not see the toggle, I do not see how this could be a problem because they would not be able to use its functionality, but if you have solutions to this issue, tell me... we cannot just put this toggling element in the toolbar.tpl.php as it was the case, otherwise users without Javascript would see it but not be able to use it, this way Javascript is unobtrusive.

Hopefully, the work that will be done here won't be destroyed by the new concept in the works in #555712: Toolbar collapsing is inefficient, but whichever of those issues makes it in the core first will have to be inspired by the other. I do not believe that usability is more important than accessibility (or the other way around), so I will work on both whenever I find the time, and at some point we'll have to "merge" them obviously.

The HTML element is placed above the main menu of the toolbar, so you place the cursor on the "Content" link...and use <Shift>+<Tab> to get the focus on that anchor... it's not easy, and might not show up in link list of the screen-readers because they usually build it with the HTML code when DOM is ready (as far as I know), before Javascript processing. As I said, I do not see how we could do it another way than adding this element with Javascript, having an empty element directly in the HTML code (as it is the case right now in HEAD), just for this purpose make that Javascript code obtrusive and the direct consequence is to show an element that is not usable by users who have Javascript disabled.

About using "title" attribute, I thought about it at first, it would also provide a tooltip for users, but the problem I had with this is I was not sure whether it is really advised to use only "title" and not the text of the anchor. What I am sure is that repeating the text of the anchor in "title" attribute is not a good thing for accessibility. If we use only "title", according to my test, in IE6 the link will not work (it cannot get a valid focus when using <Tab> navigation) unless there is a text for the anchor, so this won't be the solution unfortunately :(

The <span class="element-invisible"> could theorically work, so I tried to implement this solution, but IE6 has the same problem, if the text of the anchor (even inside a <span>) is not visible, the link cannot be used... so this isn't the solution either... we will have to stick to the text-indent in my opinion... this should not be a problem for screen-readers as far as I know.

I know the link I've just gave says the opposite, but after all, maybe this is one of the rare cases where we should repeat the text of the anchor in the "title" attribute, as it seems to be the only solution that is the most compatible. What do you think ? I've implemented this in the following patch, could you test again ? With this solution, with NVDA and FF 3.5 you should be able to test it also with your mouse (in addition of <Tab> navigation).

Patch applies nicely.

I'd like to have a solution that uses the element-invisible class, but perhaps this is not an option here. Not sure if the IE6 error you mentioned is a concern elsewhere.

Where is a good place to look for the "Show shortcuts" link. Is there a generic URL you can point us to to find the text and review it?

mgifford: I did not find any way to use element-invisible without degrading accessibility even more unfortunately.

Gábor Hojtsy suggested to make the toggle element available even for users without Javascript, this sounds like a good idea so I implemented this. The link should now appear in the link lists of some screenreaders, it is not added through Javascript anymore (as it was in my last two patch) so this improves accessibility and avoid leaving users without Javascript with a broken interface.

Ahh.. Labeling toggle in top right.. Makes more sense now..

Applied patch. Worked well.

Attached screenshot with firebug output.

So anything else need to be done or verified before it's ready to be committed?

mgifford: Did you test without Javascript... on various pages ?

Before setting this RTBC it would be better to get 2 other reviewers/tester ... and also get someone to review for coding standards :)
By the way, it adds few functions and thus this should happen before code freeze.

Just now I tested without Javascript. I also just tested it on Safari, Opera & Chrome for the Mac. All worked fine.

What other functions have you added to the code? The coder module doesn't hit the javascript as far as I'm aware.

I do not know what you are doing, the link is now back in the HTML and works as any other link in the page, it is not added through Javascript in #6... so unless it is a CSS issue, but as far as I know we are not using display:none;, but text-indent.

Did you clear the cache or resubmit the "Appearance" form after applying the patch ? It now uses a new theme function and the theme registry should be rebuild with either way.

I use the exact same solution that is used when Javascript is enabled (store the state of visibility in a cookie and set the CSS classes accordingly), but I handle it on server-side... the toggle link points to the path "toolbar/toggle" which calls a page callback (toolbar_toggle_page()) that does what's described in the comments ;) (toggle the cookie value and redirect the user from where is was coming when clicking on the toolbar). Then the CSS classes are set on various elements according to the state of visibility, while rendering the page.

When Javascript is enabled, this page is not called (we disable the link action with Javascript). Everything can be done on client-side with Javascript, so this "toolbar/toggle" page is just a fallback for users without Javascript.

If your question is how they are hidden with the appropriate CSS class ("collapsed"), see this code :

div#toolbar .collapsed {
  display: none;
}

And this is not a problem because we do want to hide them from the user (whether he is using a screen-reader or other assistive technologies) because it's the purpose of the toggle element. To show them we do not apply this CSS class and everyone can see them again.

I added the heading, I chose to load the menu and use its title as the heading text to be consistent with the administration interface. It is hidden with the "element-invisible" CSS class.

Oh... and this should be updated... seeing the extend of changes we are doing ;)

Ok, I applied the patch to the latest version of the CVS and it worked fine.

I'm running a copy of the site here - http://drupal7.dev.openconcept.ca - with critical accessibility fixes in it. It's mostly this list here - http://openconcept.ca/blog/everett/final_stretch_help_needed_for_drupal_...

think head's working now.

Well I think the patch is broken... Here's a re-roll :)

Ok. I tried that and it broke.

patch -p0 < 540282-toolbar-accessibility_5.patch
patching file modules/toolbar/toolbar.css
patching file modules/toolbar/toolbar.js
patching file modules/toolbar/toolbar.module
Hunk #1 FAILED at 21.
1 out of 3 hunks FAILED -- saving rejects to file modules/toolbar/toolbar.module.rej

Rerolled it again.

 drupal_page_footer();
+
+$this->debug('debug information');

Welll we don't need the debug code here ;) ...and there was a minor commit that might make this again out of sync... so here a re-roll.

Oops. Thanks for re-rolling.

Thanks for the feedback Everett.
I could only test the IE6 / NVDA configuration, I clicked and also used TAB navigation (+ Enter) to toggle it and it works (when navigating to the place where shortcuts are supposed to be, they appear... and when they are hidden, they are skipped and the focus is directly on the site title, if you're on the homepage), with or without Javascript enabled. The toggle is now a simple anchor, which points to an URL that toggles the visibility of the shortcuts in the cookie and redirects you where you've came from, so without Javascript I do not see how it could not work.
Maybe I am not testing the right way, could you describe what you did exactly to test it, what you expected and what happened instead ? What do you mean by "does nothing" ?
Also, did you make sure to clear your browser cache in addition of Drupal's cache ? If the script is not updated and you have Javascript enabled, it could be the cause of the problem too.

The following patch only changes the position of the toggle in the flow of the page, right now the toggle is the first element of the toolbar which might be good, but when you use it with Javascript enables it forces you to navigate with TAB through the while main menu. With this change, when you toggled with Javascript enabled, the next time you hit TAB you end up directly on the toolbar... it makes sense to me to keep those elements close, if you think it is not a good choice please tell me why. It has no impact on the visual display as this element has an absolute position.

I tested this patch and it seemed to have no negative visual effects.

The main difference between these patches is to move the toolbar_toggle down to the bottom of this list?

   <div class="toolbar-menu clearfix">
     <?php print render($toolbar['toolbar_menu']); ?>
     <?php print render($toolbar['toolbar_user']); ?>
    <?php print render($toolbar['toolbar_toggle']); ?>
   </div>
 

Think it needs an accessibility review to see if it is more visible for everyone.

Mike

Yes, to me it seems logical to keep the toggle and the shortcuts close... when using Javascript and TAB navigation, once you've reached the link right before the shortcuts, which is the toggle, if you use it to show the shortcuts then the following links are directly the shortcuts ... if you hide them, then the shortcuts are skipped when you press TAB to reach the next link on the page. Before this change, when you had just used the toggle, you had to go through the whole toolbar main menu links (Content, Structure, etc.) before reaching the shortcuts... and at that point you wouldn't have a single clue if the links are shortcuts or not. Also because the back then the shortcuts menu did not have a #heading...but that is fixed in this same patch (see comment #13)... anyways having them closer to each other improves the usability of it when usingTAB navigation.
It's less "visible" because it's not the first element of the toolbar anymore... but it's more visible when you use it.

Ok, about the heading, I agree that we could put one for the main menu of the toolbar. About changing the one for shortcuts, right now I coded it to use the menu name, so if the menu name changes, the heading changes, which I think is consistent. So I decided to change the toggle name instead, which will make it more consistent. This required to add a 'setting' variable with drupal_add_js() to let Javascript know the name of this menu (that is the cleanest way I think... I could also use the text of the anchor and replace "Hide" by "Show" and the opposite when toggling...but that sounded dirty). By the way, the code of Javascript toggling could be shortened a lot, but it is not the scope of this issue, I think I will fill another issue when this one is done to improve this a bit ;)

You find the place of "Hello <username>" wrong, where would you see that ? With the current code, without changing all the template of the toolbar, the only possible solution is to put it in first place, so I placed it this way now... It could be logical to have this first as we are greeting the user, but it might be annoying for users to have these links first on each page (usually I prefer to offer the navigation first... not Logout link). So the flow of the element is now : user menu -> main menu -> toggle -> shortcuts ... tell me if it's what you expected.

I could not reproduce the problems you encounter with IE6/NVDA 0.6p3.2. When I use the toggle with or without the Javascript the shortcuts show and hide as expected, the title attribute is updated and the text of the anchor too... I confirm that the change is not announced to NVDA in IE6, but if you navigate through links again the text has changed, so I'd blame IE6 for not pushing this change to NVDA (since it works fine in Firefox). I've done my best to inform the user and in modern browsers this information is pushed to the screen reader.

If you feel this is not the fault of IE6, and that my code is at fault, please, as I asked before, could you describe every action you executed to test the functionality (the way you use to reach the toggle link, key pressed, status of Javascript, etc.), so I can reproduce and eventually fix the problem, providing this is not due to this old browser (yes I know... it is pretty popular for users with disabilities, because of JAWS support, unfortunately).

About JAWS still seeing hidden shortcuts, that might be a feature of JAWS that still show anchors which are in a container hidden with "display: none;". I tried to hide them by adding "visibility: hidden" for the container now, it is supposed to work but I do not have JAWS, so I let you test it.

I tested this patch and it seemed to have no negative visual effects.

I see that re-roll will be needed now that #555712: Toolbar collapsing is inefficient was committed :) In that other issue, the accessibility was "quick" fixed by adding the toggle through Javascript (to avoid presenting an useless toggle when Javascript is not available), which should be removed now that I implemented a server-side toggle which does not rely on Javascript. This is why the toggle is added again in the toolbar.tpl.php and the Javascript to add the toggle has no reason to exist anymore (all this was explained in here, but I am just explaining the changes in this re-roll). There is no change in functionality, I leave this RTBC, if anyone notices a problem, please feel free to set it CNW.

+      'attributes' => array()

A "," was missing here... just a minor correction.

Latest patch nicely, doesn't seem to affect the visuals of the site.

Well... that doesn't sound right, at least not locally.

Seems ok... must have been a bot failure... restoring the status.

This will need a small re-roll now that #555712: Toolbar collapsing is inefficient was rolled back. Sorry for the inconvenience. :(

Indeed... I wish we could implement those changes before #555712: Toolbar collapsing is inefficient gets another patch, but I am not sure this will be useful, I do not know the extend of the changes that are "planned" for the toolbar (stuff like hover toggle, which are not accessible or other stuff like that).

If you compare the patches, the only difference with the one in #29 is the comma added in #34 ... so I don't think you need to re-re-(re?)-view this ;)

Silly bot.

Nonsense (I'm "so" surprised ... test bot #44 ... again), it works locally... only a bit of offset/fuzz when applying, but I'm tired to re-roll this... it has been sleeping for so long as RTBC... I won't waste my time on this unless it gets some attention.

**EDIT (after tests passed)** Surprise, surprise... :P

Looks good overall. Some minor documentation glitches and some questions.

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -27,10 +27,84 @@
+/**
+ * Formats an element used to toggle the visibility of the shortcut links.
+ * 
+ * @param $element
+ *   An associative array containing the properties of the element.
+ *   Property used:
+ *     - #collapsed
+ *       A boolean value indicating the current state of the visibility of the
+ *       shortcut links.
+ * @return
+ *   An HTML string representing the element for toggling.
+ *
+ * @ingroup themable
+ */
+function theme_toolbar_toggle($collapsed, $shortcuts_menu_name = '', $attributes = array()) {

The PHPDoc here does not match the function signature. Which is good, because $collapsed makes a heck of a lot more sense than an associative $element array containing one arbitrary property. ;P

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -27,10 +27,84 @@
+ * @return
+ *   A boolean value indicating if the 

If the...?

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -27,10 +27,84 @@
+  // PHP converts dots into underscores in cookie names to avoid problems with
+  // its parser, so we use a converted cookie name.
+  return isset($_COOKIE['Drupal_admin_toolbar_collapsed']) ? $_COOKIE['Drupal_admin_toolbar_collapsed'] : 0;

Just a question: why not use the converted name throughout then?

This review is powered by Dreditor.

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -27,10 +27,84 @@
+/**
+ * Formats an element used to toggle the visibility of the shortcut links.
+ * 
+ * @param $element
+ *   An associative array containing the properties of the element.
+ *   Property used:
+ *     - #collapsed
+ *       A boolean value indicating the current state of the visibility of the
+ *       shortcut links.
+ * @return
+ *   An HTML string representing the element for toggling.
+ *
+ * @ingroup themable
+ */
+function theme_toolbar_toggle($collapsed, $shortcuts_menu_name = '', $attributes = array()) {

The PHPDoc here does not match the function signature. Which is good, because $collapsed makes a heck of a lot more sense than an associative $element array containing one arbitrary property. ;P

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -27,10 +27,84 @@
+ * @return
+ *   A boolean value indicating if the 

If the...?

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -27,10 +27,84 @@
+  // PHP converts dots into underscores in cookie names to avoid problems with
+  // its parser, so we use a converted cookie name.
+  return isset($_COOKIE['Drupal_admin_toolbar_collapsed']) ? $_COOKIE['Drupal_admin_toolbar_collapsed'] : 0;

Just a question: why not use the converted name throughout then?

+++ modules/toolbar/toolbar.module	15 Sep 2009 06:42:47 -0000
@@ -98,12 +177,24 @@
+    '#heading' => array('text' => $shortcuts_menu['title'], 'level' => 'h2', 'class' => 'element-invisible'),
+    '#prefix' => '<div class="toolbar-shortcuts clearfix' . (_toolbar_is_collapsed() ? ' collapsed' : '') . '">',
+    '#suffix' => '</div>',
+  );
+++ modules/toolbar/toolbar.tpl.php	15 Sep 2009 06:45:39 -0000
@@ -16,14 +16,12 @@
-    <span class="toggle toggle-active"><?php print t('Show shortcuts'); ?></span>
-    <?php print render($toolbar['toolbar_menu']); ?>
     <?php print render($toolbar['toolbar_user']); ?>
+    <?php print render($toolbar['toolbar_menu']); ?>
+    <?php print render($toolbar['toolbar_toggle']); ?>
...
-  <div class="toolbar-shortcuts clearfix">
-    <?php print render($toolbar['toolbar_shortcuts']); ?>
-  </div>
+  <?php print render($toolbar['toolbar_shortcuts']); ?>

Why did you pull theming flexibility away from the template file here? Because of the extra logic involved in deciding what classes to show? If so, could we not just make that a variable that's printed like other (print $shorcut_classes or similar)?

As it is, a themer now needs to start monkeying in the renderable array to change any basic styling, such as the heading level, or adding additional classes to the surrounding code.

I'm on crack. Are you, too?

Thank you for the review.

1. theme_toolbar_toggle() PHPDoc will be fixed in the next patch.
2. _toolbar_is_collapsed PHPDoc will be fixed in the next patch.
3. About the cookie name, I used the name that already existed in the current code. I do not mean to break other parts of the code that might rely on it (even if it is not likely that anything uses this right now). The "dotted" way to name "variables" in Javascript code seemd pretty standard to me in Drupal (note, it is the only place we are using $.cookie, so there is surely not standard yet). If it has to change, please give me directions on the standards to use.
4. About the fact that theming experience is regressing, my goal is to prevent themers from breaking the Javascript functionality which relies heavily on that CSS class name. By leaving the <div class="toolbar-shortcuts clearfix"> element in the template file, we would risk the themers to remove or change this CSS class, which will break the collapse/expand functionality. If it is acceptable, please let me know, I'll use the solution you suggested to add the "collapsed" CSS class (or not, when it is expanded).

About 4., where should this variable for CSS classes for the shortcuts div would be placed ? Right now themeing seems pretty limited for toolbar, as there is no theme function (except the one my patch would add) and preprocess functions are not used directly. We are using function toolbar_page_build(&$page) { which is an implementation of hook_page_build() to add all the variables accessibles in the template which by the way are passed through "render" (so to modify anything, themers already need to to start monkeying in the renderable array to change any basic styling). So If I add a variable where would it have to be stored ? A preprocess function ?

Here's a possibility... if it fits, then we should have only the name of the cookie do discuss (if necessary).

Looks good. Applies nicely. Thoughts on a cookie name? Could just toss it in a session variable that might be easier to implement in core.

I've applied it here - http://drupal7.dev.openconcept.ca if folks want to set up an account and review it.

It was already done with a cookie before, I did not introduce this, so I won't change this behavior in this issue, which aims to improve accessibility. But yes it could be done with a session variable... but we'd loose the advantage of it being persistant accross sessions (which is introduced in #555712-72: Toolbar collapsing is inefficient. The current question is should we use another variable name, as the one with dots Drupal.admin.toolbar.collapsed set with Javascript (or with PHP) can be read only with $_COOKIE['Drupal_admin_toolbar_collapsed'] in PHP (it is due to a limitation of PHP variable names (it is documented in PHP manual, I don't see how it is a problem to follow this, specially when I add a comment to explain it).

I typed up a big reply the fact that $collapsed is coming in as an array in the theme function, causing the if clause to always return true and some concerns about absolute px values and other dodgy css, but d.o decided to log me out while typing it and now it seems I lost it all... I'll try to find the motivation to type all that once more -_-

I don't see how $collapsed could end up being an array with the current code... and I do not see any CSS modification (except that we changed the <span> for a <a>) so yeah I'd be interested in your "big reply" ;)

I couldn't figure out why it was popping in as an array either, and I never said this patch introduced CSS changes, I'm saying it should

although there are also a bunch of css cleanups necessary which aren't accessibility related, guess I should do that in a separate issue

As I thought (that's why I asked for a retest)... it needs a re-roll because of #511286: D7UX: Implement customizable shortcuts.

So here it is, but there are few things that changed :

1. No more menu title in the toggle text (and title attribute). Because as I understand, the new shortcuts system (including the shortcuts sets) do not use the menu system directly to manage the links, correct me if I'm wrong.
2. This is related to the first point, it doesn't make much sense to call it with the shortcut set name either because it seems that the "shortcuts" bar has turned into a "drawer of the toolbar" (see variable name changes initiated in #511286: D7UX: Implement customizable shortcuts), which is maybe a good thing. So I decided to rename the toogle text as "Open the drawer" / "Close the drawer".
3. The reroll is also needed because of, as seutje suggested (the $collapsed variable being an array) : #572618: All theme functions should take a single argument to make preprocess sane and meaningful. So the theme_toolbar_toggle() function now uses only one paramater... @seutje My bad if I sounded sceptic about it, but the change is not yet documented and pretty fresh :)

@seutje: Waiting for your input on some concerns about absolute px values and other dodgy css.

Cross posting I think ;) So we're clear on the $collapsed as array :)

Yeah CSS issues not accessibility related should go in another issue... and yes those ugly IE hacks must surely haunt the one who put them him (it's not me :P)...

While I'm at it, I set the status so the testbot can do its duty ;)

nice, so that's why it was suddenly coming in as an array, my apologies if I made it seem that this was introduced by this patch, I guess my timing was just horrible :P

opened #607682: Clean up the mess that is toolbar.css and will get on that today (will include removing the fixed height in there as we discussed on IRC last night)

Patch applied nicely. I can confirm that 'Close the drawer' & 'Open the drawer' was a visible link in the dom.

I'd like to see some more work to define in a bit more detail why changes were added to toolbar.module.

@mgifford: Well the "drawer" was introduced with new shortcuts... it was introduced in #511286-150: D7UX: Implement customizable shortcuts, because shortcuts can be used in other places than the toolbar, that is why the modules are separated and independant (or should be at least, I did not read all their code so I can't promise they already work completely independantly).

Ok, so @seutje & I have looked over the code and installed it. I think it might need to get a better review though before it can be marked RTBC.

Are there others who can pick through the code?

I have no problem with this patch locally. So I guess it was a glitch on test bot's behalf. Let's retest.

Really ? Same bot... and we're not touching anything in the update module as far as I know.

Seems like another change occurred in the hook_theme ... This should fix the problem with the current patch, I updated the function header comments.

Re-rolling, just for whitespace... thanks #606526: [Needs rollback] Remove trailing whitespaces and add newlines at end of files (@webchick: no, no... saying sorry before doesn't make people less "cranky" :P ).

I think that after 20+ additional comments we're ready to bring this back to RTBC.

This patch displays the show/hide link in a way that screen-readers can interact with and it will be great to see it in core!

Hm. Sadly, it appears that this one no longer applies. :( Hopefully a quick re-roll, then please bump back to RTBC.

Seemed to be some changes in the css tripped it up.

This works for me. Thanks mgifford for the re-roll.

Great! Committed to HEAD. Thanks! :D

   // Retrieve the admin menu from the database.
+  $main_menu = menu_load('management');

Seems like this introduced an undocumented dependency of the toolbar module on the menu module? (It leads to a whitescreen for me if I enable just the toolbar module alone.)

@David_Rothstein nice catch. Thanks for the heads up. I don't think this dependency is critical so I'm just checking for the module before loading it.

Actually, since this is a system-defined menu, I think we can make it work equally well regardless of whether the menu module is enabled or not.

+1 for patch #80.

That's a nice solution. Tested it with/without the menu.

Works like a charm and now it is green :) RTBC and sorry for introducing this dependency without knowing it. Thanks David_Rothstein for the fix.

Awesome. Thanks for the follow-up!

Committed to HEAD.

Can't apply this patch. For which version is it?

You might want to read comment #83. It was committed, that is surely why you cannot apply it anymore (as it is already applied). If you still encounter accessibility problems, you ought to create a new issue specifying exactly the problem you encounter as this issue was rather a general accessibility improvement.

Note that the non-JavaScript functionality seems to have regressed sometime after this issue was fixed, and now no longer works at all.

I have a mostly-working patch for that at #759524: Toolbar drawer collapsing no longer works without JavaScript.