Add description to the link items of the toolbar

Moving to proper component.

screenshot when mouse over Find content

Looks good, not sure if we need to push this in the RTBC queue before code freeze though

Looks good to me, too: I'd add that this sort of this is very useful as far as accessibility is concerned. A blind user would often rely on tooltip information to fill in the gaps in their perception of a website caused by a lack of context.

Awesome. looks good. I wish there were more titles used.

Committed to HEAD. Thanks!

@Jim

Using title attributes on anchors that have link text is usually not helpful at all for screen-reader users.

@Zolo

Why do you wish that more titles were used?

@Webchick

I wish I had a look at this before it was commited, I would have given it a -1 for accessibility. Link text should be made more descriptive. The title attribute was never intended to convey improtant information about the meaning and role of a link.

Oops. Fair enough, I should've thought of that. Sorry. Things are crazy at the ice cream sprint. :)

Rolled this back, and also made up a new tag. ;) Let's discuss this more.

H33: Supplementing link text with the title attribute | Techniques for WCAG 2.0
explains why using the title attribute is not a good idea for accessibility. This is particularly unhelpful for users of some assistive technology, users who cannot perceive the text in the limited time the tooltip appears, users who need the tooltip to be enlarged, but whose browsers do not support enlarging tooltip text, and keyboard only users who do not have access to the tooltip text.

Because of the extensive user agent limitations in supporting access to the title attribute, authors should use caution in applying this technique. For this reason, it is preferred that the author use technique C7: Using CSS to hide a portion of the link text (CSS) or H30: Providing link text that describes the purpose of a link for anchor elements (HTML).

@Everett Zufelt

JAWS doesn't read titles by default, true: but it can be asked to read them, and I think we should therefore make use of them when information is lacking in the link title. Yes, ideally link titles should be enough on their own, but this isn't always the case and nor can it be.

@Jim

JAWS users, the majority of which probably do not have their AT configured to read title attributes, are not the only users who lose out from accessing the title attribute, see my prior comment / resources referenced.

@PP

It is subjective whether this information is * important * or supplementary. However, it would appear to me through looking at the descriptions of the links that this information would be very helpful to a first time user, and therefore IMO is important.

@Everett Zufelt: I stand corrected. I had skimmed the WCAG 2.0 doc but couldn't find any reference to the "title" attribute. As WCAG 2.0 should be treated as as close to gospel as we're ever going to get, I'm now totally with having no "title" attributes.

So I guess the ideal is now that our links are sufficiently descriptive; failing that technique C7 comes into play, which would be interesting to get implemented :-P

@Jim

Implementing C7 is pretty simple, but it must be remembered that this only benefits screen-reader users, and not other users.

$link_text = 'Your original link text<span class="element-invisible">Your invisible link text</span>';

There are elements which requires explanations whereas other don't. And it's not a usability or accessibility issue, but rather a clarification issue, especially for new users.

In a form, we can display only a label next to an input tag or add a description text below the label. The same subjective rules should apply to menu items.

So, why would we permit the addition of a descriptive text below form elements if the form labels are explicit? Well, in some instances we do find that adding additional information is required.

If all the toolbar menu text are self explanatory, we don't need to add more information. But are all toolbar items self explanatory? No. You often need to click to discover the real content of their destination.

Adding form fields descriptions or menu items tool tips through the title attribute is subjective. But yes, first time users will benefit seeing this extra information.

So +1 to add this back.

@xmacinfo

I think that these descriptions would be valuable for new users as well. Let's add them in a way that ensures that as many users as possible will have access to them.

@pp

Ipologies for not answering your question before. I really don't think that a menu item description is particularly useful, perhaps more for the person administering a vast number of menu items than for the end user. If the descriptions are important, like in the current situation, then lets find a way to make the menu item description perceivable to as many users as possible.

Perhaps for the time being the best we can do for making descriptions of toolbar items perceivable is to use the title attribute, in combination with technique http://www.w3.org/TR/2008/NOTE-WCAG20-TECHS-20081211/C7"
rel="nofollow">C7: Using CSS to hide a portion of the link text from W3C / WCAG 2.0. An example of using C7 in Drupal is in comment #16.

I have also posted a discussion about this issue at http://groups.drupal.org/node/25937 to see if we can generate some ideas about making the description more perceivable to other users as well.

think this is a good solution, as certainly it will benefit both AT users and newbies. Still more should be thought about as far as other users.

I think this is as ready as it can be at this time from an accessibility perspective. It may (along with other menu links) require additional consideration in the future.

The patch in comment #22 looks good to me. My only question is if the link text and title attribute text need to be run through t()?

I tested the patch in #22 applied against clean head. It applied well and provided the desired effect for screen-readers.

One thing I notice is that "Configuration and Modules" and "Help" do not have menu item descriptions, this is a little inconsistent.

Also, I think that we should be testing to see if description is not empty before outputting the <span class="element-invisible">. I can reroll with this change if I can get some feedback on the translation question in #23, so that I can make both changes at the same time.

Fair point about not spitting out a description if one doesn't exist. I've cleaned up the code here.

I've also tested it with i18n, not that most of the descriptions are there, but this sandbox is trilingual (and bidi):
http://drupal7.dev.openconcept.ca/fr

Seems to work just fine.

Tested the patch in #25, working well.

I think if we can get some more input on this patch it is ready for RTBC.

@PP

I have commented on the new issue you opened and do not think that it is the desirable approach. Regardless if we can find a way to refine the suggestion you have made in that issue to something workable I think that it is important that the current issue be corrected. I believe that the patch in #25 is a good solution.

Want to point folks to the discussion about Read More links here -> http://groups.drupal.org/node/17627

System menus will all have descriptions for them (which is what is being inserted into the title & the invisible text in #25).

User generated menus (primary & secondary menu) will have the capacity to add descriptive text to it (but probably won't as most folks still don't bother).

It may be possible to give more context for other blocks or menu items, but it may not be. Your suggestion is to do it all with the l() function, and I know it's just not that simple.

Patch #25 accomplishes the goals of adding this tooltip and providing that additional context for more users.

@PP

The reason that a generalized approach to adding title attribute text as invisible text for anchors is undesirable is that not all of the title attribute text is important, and developers often use it incorrectly. To be honest, I wish the title attribute was * never * used for anchors and that a different method was developed for providing the tooltip on mouseover and focus.

However, for this toolbar in particular I believe that the descriptions of the menu items are 1. well written and 2. important and therefore should be made available to as many users as possible.

However, for this toolbar in particular I believe that the descriptions of the menu items are 1. well written and 2. important and therefore should be made available to as many users as possible.

I agree 100 % with Everett Zufelt. We should do this only for the toolbar links. Is that patch #5?

patch in comment #25 I think 510094-v3.patch

@xmacinfo

Patch #25 is the one that I think is ready for RTBC

@pp

As the accessibility community cannot control, now or in the future, how developers (core or contrib) may use the title attribute on anchor elements we really cannot generalize this solution with hopes that it will improve accessibility.

As per #35. ;-)

As the accessibility community cannot control, now or in the future, how developers (core or contrib) may use the title attribute on anchor elements we really cannot generalize this solution

why not? The accessibility team, is the one who should create the rule how menu description should be displayed. I am against any kind of local hacks in the toolbar module, or any part of Drupal core. Create a general solution, or don't change anything.

The patch #4 is already an improvement compared to what we have now, and that is the common Drupal solution for displaying descriptions in tooltips. The general solution for the accessibility problem should be discussed and solved in another issue.
The patch #25 is not a solution for any kind of problem, it is a hack..

@Pasqualle

I believe that it has been made clear that there is no generalizable solution to this problem. There will be some sets of menu items, like the toolbar, where it is desirable to have the title attribute made accessible to screen-reader users, and other menus where this is not desirable. IMO this is not a hack, but an implementation of functionality in a specific situation.

At least I will need a global kill-switch to disable hacks like this.. I really do not need extra hidden elements in my HTML, and there are many types of sites where functionality like this is not desired..
It is not clear why this can't be generalized to all links, or why there can't be an extra link parameter, which could be on (meaning: display [#attributes][title] in hidden tag) or off by default?

@Pasqualle

1. Please define hack.

2. Please explain why this solution (patch in #25) is a hack.

3. Please give a use case for a site where hidden link text is inappropriate.

4. I would love for you to role a patch to add functionality which solves this generally, until then, in absence of a good reason why this should not be commited, setting to RBTC.

1. hack is when you change small portion of the code, and it does not confront with the rest of the code, and it solves the problem only for you without thinking about other problems it may cause..

2. see 1

3. building a page which must be as small as possible. example twitter page (with the toolbar).. I must be in control of my site output, there is no clean way I can override your patch #25. I do not want to create another hack just to undo your solution.

4. because you made a hack. This is core, where only clean code and generally useful functionality is allowed.. Accessibility is a useful functionality but this is not a general and not a clean solution.

@Pasqualle

1. hack is when you change small portion of the code, and it does not confront with the rest of the code, and it solves the problem only for you without thinking about other problems it may cause..

Can you please explain what you mean by "it does not confront with the rest of the code". By your definition, anything that only happens once in core is a hack, that is, all functionality needs to be generalized.

3. building a page which must be as small as possible. example twitter page (with the toolbar).. I must be in control of my site output, there is no clean way I can override your patch #25. I do not want to create another hack just to undo your solution.

There is a lot of code in core modules that you cannot control the output of. You have the option of disabling those modules. I would imagine that if you wanted to make a page as small as possible that you would disable the toolbar.

4. because you made a hack. This is core, where only clean code and generally useful functionality is allowed.. Accessibility is a useful functionality but this is not a general and not a clean solution.

As it has been said before, unless you can provide a generalized solution which a. solves the problem, b. does not create more problems, and c. will actually be implemented by developers with little to no understanding of accessibility then this IMO is not a hack.

There is a lot of code in core modules that you cannot control the output of.

please show me one example and I will create an issue for it immediately.

generalized solution: #566774: Accessibility: link title attribute It's not ready but that is the generalized solution.

@Pasqualle

Having commented on the suggested generalized solution, I believe that you understand that it is clear that I believe that the solution will not work. In #526712: l() does not accept a parameter for supplemental text for screen-readers an attempt was made to modify l() to generalize the functionality to embed invisible text within links, it was deemed inappropriate, a decision which after much discussion I agree with.

Setting back to RBTC so that the maintainers can decide if this solution, which has received only one -1 is ready for core.

+++ modules/toolbar/toolbar.module	2 Sep 2009 13:09:24 -0000
@@ -147,6 +147,10 @@ function toolbar_menu_navigation_links($
+        $link['title'] .='<span class="element-invisible">' . $item['link']['description'] . '</span>';

Code style: should be .= ' (space between = and ')

Additionally, we should have a space between this and the actual title of the link. Right now it runs together as "AppearanceSelect and configure your site theme." when you have CSS disabled. I liked the active tab patch which put these descriptions in parentheses to help people who can see these descriptions (by accident or whatever) understand that they're just descriptions.

This patch exposes a really weird inconsistency in our labeling. For whatever reason, People, Dashboard, Configuration and Modules, and Help don't have descriptions, but Appearance, Structure, Content, and Reports do. I'm guessing this was a move by the UX team to not re-state the obvious, but with this patch this feels incredibly buggy. We probably shouldn't fix that here since we'd be killing kittens, but it definitely needs to get fixed soon.

I get what Pasqualle and pp are saying about this patch. Basically, that if Toolbar module's links print the description on hover, people are going to (very reasonably so) expect every link to do that. OTOH, I think adding the invisible spans for single possible link is way overkill, but fine for the toolbar module to do.

Is it possible to combine both the original patch and this one? For example, move:

+        $link['attributes']['title'] = $item['link']['description'];

...from toolbar_menu_navigation_links() to whatever a more generic function for building menu link output might be? There might not be such a function, and there might be a good reason not to do this, but I mention it just in case.

This review is powered by Dreditor.

Btw, I just filed #620616: Fix inconsistencies in menu description labeling to talk about those description inconsistencies. Depending on the outcome of that patch, this one might turn out to be "won't fix" if all of the descriptions go away.

I conferred with Bojhan and the descriptions should be put in. I'll be taking a crack at writing the descriptions in the meantime at #620616: Fix inconsistencies in menu description labeling. At present, I cann't find where the descriptions for Dashboard, Content, People and Help would go in the code.

@webchick, your review makes sense to me, thanks! I've rerolled my earlier patch with spaces and brackets as requested.

Not sure about moving this to a more generic function. This seems to work fine and be simple enough.

This patch now will clearly mark for both mouse & screen-reader users. It won't help keyboard only users, but not sure of a clean way around that.

applied the patch. Attached is a screenshot showing how it appears as text-only. Looks good to me!

Ok, then I'm going to set this back to RTBC as I think the issues have been addressed.

Sorry, this fell off my radar. Committed to HEAD. This is consistent with our use of invisble spans elsewhere.