Overlay breaks secondary tabs for some themes

Filing this under 7.x-dev so it will be seen by the right people.

This patch copies the way Garland does it, by settings $vars['tabs2'] for secondary local tasks.
Apparently Overlay module copies $vars['tabs'], then unsets it, but only renders the first element -- primary local tasks.

I think this is probably an issue with Overlay, but the patch to Bartik was easier.

If I can put together a patch for Overlay, I'll post it here and change the component on this issue.

OK, I don't have enough info to roll a patch for Overlay.

The gist is this:
Overlay and Bartik make conflicting assumptions.

Overlay assumes that $vars['tabs'] will only ever contain primary local tasks.
Bartik assumes that it's save to use $vars['tabs'] for both primary and secondary local tasks.

I'm not sure which is the correct assumption - maybe there is a convention that I am unaware of.
Someone with more theme development experience should weigh in.

This is not critical -- tabs work fine without overlay.

As #6 tabs still work without the overlay.

At root this is (another) issue with Overlay. Bartik and Stark display the same symptoms wrt secondary tabs.
This is definitely a half-baked implementation in Overlay, but part of the problem is also that there's no standard way to deal with tabs -- only conventions.

As noted above, in Bartik (and maybe stark), primary and secondary tabs are built in a multi-dimensional array, which Overlay blows away. In Garland, primary tabs and secondary tabs have their own variables, so when Overlay overwrites "primary tabs" and "tabs", secondary tabs stay untouched.

Here are the possible solutions (imo - maybe there are more):

• A standard should be adopted for building tabs; Overlay and core themes should adhere to it (best solution)
• Overlay should not destroy variables set by themes (half solution)
• Bartik and Stark should be updated to support Overlay (backwards solution)

Regardless of which solution is used, this needs to be documented somewhere.

Lastly, while this was not a critical issue for Bartik, this is a critical issue for Overlay.
Nuking secondary tabs effectively prevents site administrators and users from accessing critical parts of Drupal, like User Roles ( admin/people/permissions/roles ).

Overlay is core, so "it works fine without overlay" isn't grounds for bumping priority down. Agreed that ideally overlay should handle the issue, and IMO it's critical if we're nuking content people need to see to administer their site.

The dimensional array $variables['tabs'] is populated in the core function theme_preprocess_page()

  $variables['tabs']              = theme('menu_local_tasks');

Primary tabs are stored in $variables['tabs'][0] and secondary tabs in $variables['tabs'][1] — see theme_menu_local_tasks() in menu.inc
I think we can assume that this is the standard processing for tabs.

In the attached patch, overlay_preprocess_page() destroys $variables['tabs'][0] which contains the primary tabs — see theme_menu_local_tasks() in menu.inc. Any secondary tabs remains safely stored in $variables['tabs'][1] for themes expecting to find theme there — e.g. Bartik. As for themes using their own variables for tabs — e.g. Garland and Stark — they are not affected at all by that deletion and should include CSS rules — as Seven already does — to hide primary tabs inside overlay.

.overlay .primary,                       // MY NOTE: Seven always output primary tabs and hide them if Overlay module is active
.overlay #branding h1.page-title,
.overlay #left,
.overlay #footer {
  display: none;
}

Applying the patch, all primary and secondary tabs are displayed correctly in Bartik, Seven and Garland, both with Overlay enabled and disabled.
I was able to navigate to admin/people/permissions/roles and, for example, admin/appearance/settings/bartik

HTH
Riccardo

subscribing

$variables['primary_local_tasks'] is being set in seven_preprocess_page() and maybe other themes, but overlay shouldn't intervene with them.

Patch is at least better than current code. We could also add some extra css to overlay-child.css that hides .tabs.primary; if a certain theme wants it to be visible anyway it should override that CSS statement. But I think if possible overlay should remove the tabs from output (like patch does). We could do both though.

Anyways lets commit this patch first.

P.S. It is pretty weird that theme_menu_local_tasks() is returning an array instead of a string; you'd expect a theme function to return a string.

sub

Alright, let's start with this patch and follow-up if we need to do more. Committed to CVS HEAD.

Hmm, a year and a half later... this never got committed or solved?

I mean, this makes Drupal completely unusable unless you change your theme.

See #17; this patch got committed and seems to have solved the problem. If you're having problems please open a new issue with screenshots and specifics of your setup.