With the new menu system, tabs changed radically in how they work behind the façade. However, I noticed that the default tab doesn’t point to it’s parent.

Let’s take an example: on user/1, the (active) ‘view’ tab points to user/1/view rather than to user/1. The same happens all around Drupal, for example in admin/content/types vs. admin/content/types/list. This leads to several problems:

  • Duplicate content: user/1 and user/1/view share the exact same content, even though they have different URLs.
  • Help module breaks: If you specify a help text for admin/content/types (there is in fact one), it won’t show up on admin/content/types/list, even though it’s the same content.
  • user/1 and user/1/view don’t have the same page titles: While user/1 shows the user name as page title, user/1/view has the title ‘View’. That means, page titles do now change from tab to tab rather than staying the same on one tab level. This is not the intended behavior: Tabs are usually displayed below the page title and are therefore “sub pages” of that title.
CommentFileSizeAuthor
#14 local_tasks_path_3.patch4.15 KBpwolanin
#12 local_tasks_path.patch3.97 KBchx
#11 local_tasks_path_2.patch3.6 KBpwolanin
#10 tabtest.zip_.doc2.35 KBpwolanin
#9 local_tasks_path_1.patch2.66 KBpwolanin
#4 menu_defaulttask.patch2.56 KBChrisKennedy
#2 menu_86.patch4.1 KBchx

Comments

kkaefer’s picture

kkaefer commented

Ok, this doesn’t happen for user/1, because the page title is set somewhere else and not taken from the menu system. However, admin/content/types vs. admin/content/types/list is a good example.

chx’s picture

chx commented
Status: Active » Needs review
StatusFileSize
new menu_86.patch4.1 KB

This patch restores all behaviour mostly -- if you have an object for the default tab then it points to itself otherwise it's the parent.

kkaefer’s picture

kkaefer commented
Status: Needs review » Needs work

There are some glitches:

When you’re on a non-default tab:

  • …the default tab still points to the old URL (admin/content/types/list instead of admin/content/types)
  • …the non-default tab points to the default tab without the link_item appendix (e.g. admin/content/types instead of admin/content/types/add)
ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new menu_defaulttask.patch2.56 KB

Here is a patch from chx via IRC - I tested it and it correctly fixes default tab behavior. I had noticed some problems that popped up within statistics.module (http://drupal.org/node/143081).

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

Can we add some code comments please? Thanks! :)

pwolanin’s picture

pwolanin commented

I thought I fixed the problem with the Help module in an earlier patch?

(from menu.inc - look at the last line)

function menu_get_active_help() {

  $output = '';
  $item = menu_get_item();

  if (!$item || !$item['access']) {
    // Don't return help text for areas the user cannot access.
    return;
  }
  $path = ($item['type'] == MENU_DEFAULT_LOCAL_TASK) ? $item['tab_parent'] : $item['path'];
ChrisKennedy’s picture

ChrisKennedy commented

@Peter - this patch fixes the default tab link, not the help.

pwolanin’s picture

pwolanin commented

I need to work through this more, but I don't think this code is going to work at depths below the first level of local tasks.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new local_tasks_path_1.patch2.66 KB

this isn't maximally efficient, but seems to work. I'll post some additional code to use for testing as well.

pwolanin’s picture

pwolanin commented
StatusFileSize
new tabtest.zip_.doc2.35 KB

ok, trying to get around the upload restrictions ;-)

rename the attached file to tabtest.zip and unzip it. Inside is a small test module and a replacement for the page.tpl.php of garland. This version just shows tabs at depths below 0 and 1.

Substitute the page.tpl.php and enable the module. Visit /tabtest and /tabtest/6 (or any arg)

pwolanin’s picture

pwolanin commented
StatusFileSize
new local_tasks_path_2.patch3.6 KB

with additional/corrected code comments.

chx’s picture

chx commented
StatusFileSize
new local_tasks_path.patch3.97 KB

Steven reviewed the patch and complained that default tasks within default tasks does not point high enough. That's solved by a nice empty for loop which climbs up until it finds a non default local task. Also, he mentioned that we should not show single tabs. Fixed, too.

pwolanin’s picture

pwolanin commented

Since this patch stores the full item in the $items array, it seems the $tab_parent array is unused and can be eliminated.

+        $tab_parent[$item['path']] = $item['tab_parent'];
pwolanin’s picture

pwolanin commented
StatusFileSize
new local_tasks_path_3.patch4.15 KB

using chx's logic to find the right href for local tasks, but rewrote the code and re-tested.

flk’s picture

flk commented
Status: Needs review » Reviewed & tested by the community

yeap this patch works fine ;)

tested it on both blocks and the little tab module you gave...works great.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

All seems to be fine here. How does this affect the help use case and pwolanin's workaround in particular? It seems this patch should remove that.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Discussed the issue with chx on IRC, and the help issue is different, will be addressed in another patch.
Done another review and it looks good, so committed.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)