Local tasks lack semantic markup to indicate an active task

We also need to know the depth of the menu item. (see #371453: theme_menu_local_task does not differentiate between primary and secondary local tasks) Or is that what you mean by "context"?

subscribing

Could someone offer a sample of what a "semantic" active secondary task might look like??

It would be so easy to add $level to that array. Why not do it?

I think the same array of information should be passed to theme('menu_local_task', ...) But I see that's a bigger change because that function would take new parameters.

I believe what you really want is this.

Hrm. I see the difference now - you added the " (active)" string to the link text, not to the title attribute like in my patch.

However, that would look a bit strange for users that are used to get a visual highlighting of the currently active link(s).

Isn't there some other, reliable way to pass on textual information/attributes to screen-readers?

This is a great solution. It looks good and it will provide such an accessibility improvement for the whole interface!

Thanks for rolling the patch!

Mike

The latest patch applies nicely & produces the expected results!

+1

I question the approach this issue is taking to solve this issue (which is really one of context):

$head_title and Page $title should provide context to screen readers, last time I looked we don't use AJAX to refresh local tasks so to my mind the context is either already there, and if not, that is the place to address it.

1. The user just clicked a task or menu link - so they know where they are going.
2. The page title and heading level 1 are read out, that gives the context for the page you are on.

Currently this is not the case, since when secondary local tasks are clicked the target page when loaded does not change the title. To my mind that is the issue, I think adding invisible text to a link is a work-around - and non-standard one at that. Will screen reader users really tab through all those links (and wait to hear "active" read out) or jump headings?

Context should be provided by the semantic markup that user expects, in this case at the very least by <title> and <h1>.

My 2 cents.

Please review the updated patch. The functionality of the prior patch worked as advertised and I've added a few things:

• Updated the corresponding test in modules/node/node.test. We had to use assertRaw() because assertLink() does not handle HTML in the link text.
• The only part we need to translate through t() is the '(active tab)' text. Let me know if you disagree with this approach.
• Minor code and indent tweaks.

Based on my testing, the active tab hidden text works in the following places:

• the Seven /admin screens
• works in the Garland screens, eg /node/1/edit
• works with 2 levels of tabs, eg /admin/appearance/settings/garland, in both themes

Further review and feedback would be great.

@jmburnz, I see your point but I wonder if we could rally the community to change the page title or h1 in all these cases. One tricky case is the node View tab. I'm not sure we could add the word View into the h1 or title tag. That would need to appear on every page for users logged in with edit permissions. If we can't put the word View into the h1 or title, this patch seems like a way to queue users that View is the current operation. I'm curious what you and others think.

I'm all for wrapping 'active tab' in a t(). It is an interface level that should be translated.

I don't think we should be piping the brackets through it though. @brandon you are using:
t('(active tab)')

But I think this is more standard:
'(' . t('active tab') . ')'

Mike

hello.

I do agree about the necessity of having a way to recognize the active task, so thanks for opening this issue.

if it isn't much work, I would rather have the active task change be reflected in the title of the page, as this is more user friendly and is more expected. this would also benifit screen readers's users and sighted people alike from a usability point of view.

however, since I can't provide a patch, i would rather have usable solution roled out before the code freeze in 6 days time. the current patch sounds to be better than nothing and is surely usable.

thanks,
Mohammed al-shara.

Glad to have you back involved Mohamed76. Would be especially great to have testing in RTL if you have time.

It is reflected in the title already. And it's nice that D7 even tosses in the content type that is being edited:
Edit Article Example | test sandbox install

This allows individuals to focus over the element and determine what that specific link is the active task. It's not an either or but both.

Mike

Hey, thanks for pointing that our Mike, is it possible to have this on one the test sites spoken about yesterday, I know there a few who cant have a D7 setup and would like to see the patches as applied.

Hey, thanks for pointing that our Mike, is it possible to have this on one the test sites spoken about yesterday, I know there a few who cant have a D7 setup and would like to see the patches as applied.

Ok, I've rerolled this patch with the i18n changes, applied it to one of our test servers --> http://drupal7.dev.openconcept.ca/

Create a new account here - http://drupal7.dev.openconcept.ca/user/register

And then send me a message to let me know so I can approve your account. I'll then give you admin access to add/remove content.

Not sure there's any better way to do this.

Mike

I've re-rolled the patch in 29 with only one change: a fix to the Node edit test in modules/node/node.test. There was a mistake in the assertRaw() line that is fixed now: $this->assertRaw($edit_link, t('Edit tab is active.'));. The test passes on my environment so we'll see about the bot.

Testing the patch gives an error on node #34 but passes on my environment. Can anyone advise how best to troubleshoot the test?

I'm assuming this test is the failing test:

$edit_link = '<a href="/node/' . $node->nid . '/edit" class="active">' . t('Edit') . '<span class="element-invisible"> (' . t('active tab') . ')</span></a>';
$this->assertRaw($edit_link, t('Edit tab is active.'));

Yeah, looks like that's the test that fails and I think I see why.

<a class="active" href="/drupal-head-cvs/node/1/edit">Edit<span class="element-invisible"> (active tab)</span></a>

The order of the class and href attributes are reversed. If the order is reliable then changing it in the patch for node.test will fix it. Looking at the available Assertions for SimpleTest I don't see any that work on the dom instead of just text though. I guess just switch the text around?

@Brigadier

Thanks for the help. A reliable option may be to go back to matching on text like the original test did. Rather than assertRaw() we could do assertText('Edit (active tab)').

Please review this updated patch. The only change is the assertText() test as discussed. Thanks, everyone!

+1

Applied this here with no problems - http://drupal7.dev.openconcept.ca

This is a critical issue to get in core before sept 1st as per - http://openconcept.ca/blog/everett/final_stretch_help_needed_for_drupal_...

This is also will likely influence the following issue http://drupal.org/node/467296

We want the phrase 'active tab' to be translated so that when a blind screen reader is viewing the site in Thai that they get the description all in Thai:

แก้ไข (แท็บที่ใช้งาน)

Rather than in Thai & English:
แก้ไข (active tab)

This is based on a machine translation http://translate.google.ca of what appears for an English screen reader:
Edit (active tab)

Hello mike,

thank you for giving me access to a test site. I can see how this issue is implimented on the site. the active tap has text telling us so: "view [active tab]". however switching to any other tab just changes the text of the active tab, like when clicking contact it says "contact [active tab]" however the title of the page still says "mohammed| open concept]" and the heading of the title also is still labled "mohammed". I was hoping ti become like "contact mohammed | open concept. or something like "mohammed | contact"

anyway, it may be too late to do so now. what should be done so the current fix goes to core? it's certainly very helpful and usable.

thanks,
Mohammed.

+++ includes/menu.inc	27 Aug 2009 01:10:01 -0000
@@ -1262,7 +1262,14 @@ function theme_menu_item_link($link) {
+  if (!empty($link['active'])) {
+    $link_text .= '<span class="element-invisible"> (' . t('active tab') . ')</span>';
+    $link['localized_options']['html'] = TRUE;
+  }

This is not going to work out. By setting 'html' = TRUE, you potentially expose unsafe HTML from the original menu title.

The original $link_text must be check_plain()'ed then.

+++ includes/menu.inc	27 Aug 2009 01:10:01 -0000
@@ -1531,7 +1538,7 @@ function menu_local_tasks($level = 0) {
-            $link = theme('menu_item_link', array('href' => $tasks[$p]['href']) + $item);
+            $link = theme('menu_item_link', array('href' => $tasks[$p]['href'], 'active' => TRUE) + $item);
             $tabs_current .= theme('menu_local_task', $link, TRUE);
@@ -1571,21 +1578,24 @@ function menu_local_tasks($level = 0) {
-            $link = theme('menu_item_link', array('href' => $tasks[$p]['href']) + $item);
+            $link = theme('menu_item_link', array('href' => $tasks[$p]['href'], 'active' => $active_task) + $item);
...
           else {
-            $link = theme('menu_item_link', $item);
+            $link = theme('menu_item_link', array('active' => $active_task) + $item);
           }
-          // We check for the active tab.
-          if ($item['path'] == $path) {
+          if ($active_task) {
             $tabs_current .= theme('menu_local_task', $link, TRUE);

'active' actually is the purpose of the second argument (TRUE) to theme_menu_local_task().

This might need some re-consideration.

+++ modules/node/node.test	27 Aug 2009 01:10:02 -0000
@@ -203,9 +203,9 @@ class PageEditTestCase extends DrupalWeb
+    $this->assertText(t('Edit') . ' (' . t('active tab') . ')', t('Edit tab is active.'));

Since the proper escaping is mentioned in this review, we should also verify this in a test.

4 days to code freeze. Better review yourself.

Thanks for this sun. We do need a little bit more information however.

The first point is excellent and we can easily roll a patch to resolve that.

We're not exactly sure how you want us to address your 2nd & 3rd points however.

We've discussed passing along strings for translation like this:

$link_text = t('%menu_item_link_text<span class="element-invisible"> (active tab)</span>', array('%menu_item_link_text' => $link['title']))

to give context rather than splitting up the task & the active tab text. I think it's going to be easier for everyone if we keep it like it is.

Are you asking us to write a simpletest to verify this? What escaping is mentioned where?

As a developer and a translator, I would imagine it is simpler to leave 'active tab' translated separately from the actual tab names. If my site has 15 different tab names, I would be frustrated to find out that I needed to do 30 translations -- one for each tab name in both active and inactive state. I would rather just translate my 15 tabs like usual and then also add one translation for 'active tab' that would apply everywhere.

I support this important patch either way. Thanks for your work on this.

Glad to see that this issue is being worked on. Getting this information across to people who can't perceive it visually will be a real boost.

@Everett

That makes sense. Here's what I suggest:

$active = '<span class="element-invisible">(' . t('active tab') . ')</span>';
$link_text .= t('!title !active', array('!title' => $link['title'], '!active' => $active));

This allows translators to translate '!title !active' to customize the order. And they can just translate 'active tab' once, while translating all the titles as usual.

1) Please note that check_plain() must only happen when $link['localized_options']['html'] is not already TRUE. If it is already TRUE, then the link is supposed to output HTML instead of plain-text, so you can just append your SPAN.

2) I now realize that theme_menu_local_task() applies an 'active' class only to the surrounding LI and you want to alter the link's attributes. So this should be fine... Although I'm not entirely sure whether this is the right place to do it. The 'localized_options' for the menu link should already contain a class 'active-trail' for all menu links that are in the trail to the current page. If that is not the case for local tasks, then that would be a separate bug.

At the very least, you should be using the regular $link['in_active_trail'] property (instead of the custom 'active') like elsewhere in menu.inc.

3) A test should ensure what I mentioned in 1) -- menu links containing already HTML get the additional HTML appended without being check_plain()'ed, and menu links not being HTML already need to be properly escaped.

4) Should (hopefully) be covered by 2) already.

On those last follow-ups:

#49 looks good. Yes, the SPAN should not be part of the translatable string. However, the parenthesis should, and there should be no space between the placeholders, and you need to use unique placeholder names, so translators have some clue about the string at all:

$active = '<span class="element-invisible">' . t('(active tab)') . '</span>';
if (empty($link['localized_options']['html'])) {
  $link['title'] = check_plain($link['title']);
}
$link['localized_options']['html'] = TRUE;
$link_text = t('!local-task-title!active', array('!title' => $link['title'], '!active' => $active));

thanks, @sun! that all makes sense.

I applied this and it is working well. Think this is now RTBC!

This patch looks great. I tested to confirm it has no visual change in Garland/Minnelli or Seven. It correctly adds the "(active tab)" invisible text to both primary and secondary tabs. I've included one Garland screenshot where the visual appearance is unchanged while the invisible text is added to both the "Configure" primary tab and "Global settings" secondary tab.

Regarding @sun's suggestion about using $link['in_active_trail'], unfortunately the in_active_trail parameter is not available to theme_menu_item_link(). In the future if that becomes available we can upgrade to use it.

Added an inline comment to indicate the purpose of the "(active)":

// Add text to indicate active tab for non-visual users.

Otherwise unchanged, so leaving RTBC.

Yes, this new patch still applies cleanly and I tested to re-confirm that it works just like the last one. I support leaving it RTBC. Thanks, @Owen!

+++ includes/menu.inc	31 Aug 2009 21:17:19 -0000
@@ -1262,7 +1262,19 @@ function theme_menu_item_link($link) {
+    $link_text = t('!local-task-title!active', array('!local-task-title' => $link['title'], '!active' => $active));

That t() function looks a bit unnecessary?

While I can see how this can help screen readers, I'm actually surprised that this is the way to do it. I'd expect that screen readers would automatically know that this would be active based on the URL rather than some extra markup. Shouldn't we provide markup at the ul-level to communicate that this are tabs, and then let the screen reader inidicate what is the active tab?

why can't we just simply use something like

$link['attributes']['accessibility'] = t('hidden text for accessibility purposes');

but the accessibility problem of the active link can be easily fixed inside l() same as the active class is added..

you may add the required functionality to this issue: #566774: Accessibility: link title attribute

Tagging.

Please re-roll.

The last patch submitted relies on functions that no longer exist - http://drupal.org/files/issues/issue-521852-active-task-8.patch

@sun

Since this function no longer exists in D7, can you point me to the patch that describes how we're supposed to do this now?
http://api.drupal.org/api/search/6/menu_item_link

Found it rather frustrating scraping around to determine how to best pass along this information.

It appears that #283723: Make menu_tree_output() return renderable output eliminated theme_menu_item(). Anyone who already has their head around the new code know how the best approach to pass along $active?

This should hopefully get you "somewhere".

sub

Thanks sun, that helped out a great deal.

I had to modify it a bit, but seems to be working fine now.

@Everett which patch were you testing?

Setting this to needs review as I think #68 is working fine.

@Everett

Usually the tabs come right after the node title, which is an h1. So what about <span class="element-invisible">Tabs</span>? It could be an h2 or a span. That might be better than repeating the node title twice in close proximity, especially on long pages? Do you agree?

testing of bot was bust

Patch at #122 of #467296: Accessibility improvements for vertical tabs should address the problem.

@Frank

Can you explain more about how that patch addresses this? This specific issue is to make the active tab (aka local task) have some invisible text in it for screenreaders saying "Active task" or a similar phrase.

-Brandon

@Brandon

The active vertical tab will be rendered as follows:

<li class="vertical-tab-button selected" tabindex="-1">
  <a href="#">
    <strong>URL path settings</strong>
    <span class="summary">No alias</span>
    <span id="active-vertical-tab" class="element-invisible">(active tab)</span>
  </a>
</li>

See Accessibility and Drupal > Drupal 7 accessibility notes.

Please note:
The vertical tabs are not "local tasks" as usual tabs but are derived from collapsible fieldsets by using jQuery .

hth
Frank

Please review. This has the same functionality as the prior patch by @mgifford. FYI, this was RTBC before the Paris/September code freeze.

Tests passed:

• No visual change -- verified in Garland and Seven
• Correct tabs are marked as "active tab" including admin pages with 2 levels of local tasks (eg /admin/appearance/settings/garland)
• Tested in Firefox 3.5 and Safari 4 on Mac

@Frank, Yes this patch essentially provides the same functionality in the way of an "(active tab)" indicator to screenreader users for the traditional tabs (View, Edit, etc) which often appear after the node title.

Looks good to me Brandon. I tested it here http://drupal7.dev.openconcept.ca

Setting it back to RTBC.

This was first RTBC now over two months ago. - Bump!

@mgifford and others: Pursuant to the Drupal 7 development schedule, patches around API clean-ups and feature exceptions were given priority during the period from Drupalcon to Oct. 15 (later extended to Oct. 17). Over the next month (from now until Dec. 1) is where we focus on accessibility, usability, and performance. Hence why some of these have been sitting in the queue for awhile.

Just a couple of things:

+++ includes/menu.inc	12 Oct 2009 04:21:00 -0000
@@ -1357,7 +1357,19 @@ function theme_menu_link(array $variable
+    if (empty($link['localized_options']['html'])) {
+      $link['title'] = check_plain($link['title']);
+    }

Can you document why you're doing this, since at first glance this appeared to be a bug, since l() is also check_plain()ing these links? (though, granted, not for those with HTML set to TRUE.)

+++ includes/menu.inc	12 Oct 2009 04:21:00 -0000
@@ -1357,7 +1357,19 @@ function theme_menu_link(array $variable
+    $link_text = t('!local-task-title!active', array('!local-task-title' => $link['title'], '!active' => $active));

I think there be a space between these two strings, either in this t() function call, or above where we make it an 'active tab'. Right now, if you disable CSS, the words get all smashed together like:

"Content(active tab)"

This review is powered by Dreditor.

Here's a proposed comment that hopefully makes sense:

+++ includes/menu.inc	12 Oct 2009 04:21:00 -0000
@@ -1357,7 +1357,19 @@ function theme_menu_link(array $variable
+    // If the link does not contain html already, check_plain() it now.
+    // After we set html=true the link will not be check_plain'd by l().
+    if (empty($link['localized_options']['html'])) {
+      $link['title'] = check_plain($link['title']);
+    }
+    $link['localized_options']['html'] = TRUE;

Also, I propose we use t('!local-task-title !active',...) with a space in the translatable string. That seems to be cleanest but I am happy to re-roll this patch either way if the group prefers otherwise.

@webchick - I'm not always as patient as I should be. I'd misunderstood when accessibility issues were going to be addressed (but do think that others working on this had too).

I tracked back the check_plain()ing links to @sun's contribution here:
http://drupal.org/node/521852#comment-1980856

As he said a bit earlier "By setting 'html' = TRUE, you potentially expose unsafe HTML from the original menu title."

So I'm fine with putting in a space. I wondered about that myself as I'd seen the same issue months ago. However, screen-readers don't need the space to interpret it correctly. Still good for those that turn off their CSS every now and again.

Brandon, thanks for the offer to re-roll. I'm still recovering from the flu so had to apply the patch, and bring myself up to speed on this issue again before I could comment on it. So I've got a patch with the changes.

+++ includes/menu.inc	2 Nov 2009 16:09:20 -0000
@@ -1408,7 +1408,22 @@ function theme_menu_link(array $variable
+    // If the link does not contain html already, check_plain() it now.
+    // After we set html=true the link will not be check_plain'd by l().

1) HTML, when referring to HTML, always uppercase.

2) 'html', when referring to the 'html' property, uses the capitalization of the property, and, single quotes around it.

3) TRUE is always uppercase.

4) Replace "check_plain'd" with "sanitized".

This review is powered by Dreditor.

Thanks sun. Standards are important and sorry I wasn't aware enough of these ones.

> + // If the link does not contain HTML already, check_plain() it now.
> + // After we set 'html'=TRUE the link will not be sanitized by l().

I've updated the patch.

I'm not sure why this is failing now unless the node.test has changed.

Anyways, not sure how to resolve this error:

Edit tab found. Other node.test 209 PageEditTestCase->testPageEdit()

I tried asserting this line in (as this is what is inside the anchor tag:

    $this->assertLink(t('Edit <span class="element-invisible">(active tab)</span>'), 0, t('Edit tab found.'));

and then I get xpath errors:

SimpleXMLElement::xpath(): Invalid predicate Warning drupal_web_test_case.php 1696 DrupalWebTestCase->xpath() Exception
SimpleXMLElement::xpath(): xmlXPathEval: evaluation failed Warning drupal_web_test_case.php 1696 DrupalWebTestCase->xpath()

Not sure where to go from here.

That happens, because assertLink() uses xpath to search for all anchor tags in the document and tests each one against the passed string.

  protected function assertLink($label, $index = 0, $message = '', $group = 'Other') {
    $links = $this->xpath('//a[text()="' . $label . '"]');
    $message = ($message ?  $message : t('Link with label %label found.', array('%label' => $label)));
    return $this->assert(isset($links[$index]), $message, $group);
  }

So:

1) Basically, you should be able to test for "Edit (active tab)", because it tests for the contained text of an anchor tag.

2) However, you cannot use t('Edit (active tab)'), because t() does not return the expected value here. You need to pass the actual value to t() with the proper arguments (as in the original code).

Technically totally correct would thus be:

$active = '<span class="element-invisible">' . t('(active tab)') . '</span>';
$link_text = t('!local-task-title !active', array('!local-task-title' => t('Edit'), '!active' => $active));
$this->assertLink(strip_tags($link_text), 0, t('Edit tab found.'));

Ok, this sounds great. Detailed and useful explanation. Thanks!

But it didn't work. I tried the totally correct answer you provided and also just 'Edit (active tab)' and it didn't like either of them.

Still got the same error in SimpleTest. Don't know what I could have done wrong.

Please review this updated patch. The only change is to modules/node/node.test:

     // Check that the title and body fields are displayed with the correct values.
-    $this->assertLink(t('Edit'), 0, t('Edit tab found.'));
+    $active = '<span class="element-invisible">' . t('(active tab)') . '</span>';
+    $link_text = t('!local-task-title !active', array('!local-task-title' => t('Edit'), '!active' => $active));
+    $this->assertText(strip_tags($link_text), 0, t('Edit tab found and marked active.'));

@sun: this is nearly identical to your suggestion, but uses assertText() instead of assertLink(). I believe assertLink() does not handle checking for link text that contains HTML mixed in. I believe it's safe to check for text instead of a link because the text "Edit (active tab)" would not appear on the page for any other reason. I couldn't find assertLink(strip_tags(...)) used anywhere else but if you have a suggestion, let us know.

I applied the changes locally and it's working fine. With these changes and a happy bot I'm setting it back to RTBC.

Looks good! Committed to HEAD.

Excellent! Thanks everyone.