Move action "tabs" out of local tasks

yes.

from #394702: Admin/content/node should have a create content link:

Local tasks we have are rendered as tabs. Tabs are primarily a pattern for seperating different 'views' on data, different information spaces. Right now, we are also putting 'actions' in there, stuff that should be buttons or links. Besides being easily overlooked, we're mixing both 'view' and 'action' operations in the local tasks and we need a mechanism that treats the two as the seperate things they are.
Tabs for tabs, links and buttons for actions. Tabs along the top, actions right there where you need them, in context of the element(s) you perform that action on (probably meaning: free to be placed where needed).

Yeah, the http://d7ux.org/content Find content mocks include the 'Add content' link, which is how this is implemented in that page and styled in the admin theme via #536960: "Add content" link in Seven theme (once committed). I think we can generalize the underlying code from there and the theming from the other issue (once committed) to reuse on the mentioned pages.

BTW not at all about the node system.

Very negative side effect:

Arbitrary buttons are (most likely?) not contained in the menu system? How can admin_menu expose those pages?

That aside:

Great, this concept applies to nodes (or content). But what about contrib modules providing content or not-so-really-content pages? Are they just supposed to put their buttons "somewhere"?

If that is the supposed direction, then this will lead to a *very* inconsistent user interface.

Forget the 'can be placed anywhere'. For now. Main opbjective is the need to split up local_task rendering in two, one for page/list links, one for actions/buttons.

The consistency argument here doesn't make that much sense. I don't store my hammer and screwdrivers on the bookshelves, although that would be consisten with how I store my books…

We need a new place for the tools in local_tasks. I guess this needs local tasks identifiying themselves as one or the other and a default for the undefined scenario.

@sun:

Menu system: well, as @yoroy says, we can make these menu items; just add another type of menu item additionally to local tasks, eg. "local actions"? Or rename local tasks to "local tabs" and add "local actions", so we somehow communicate the notion that local tasks should be used for different views and not for navigation.

Consistency: My understanding is that we have CRUD screens, where you have a listing of stuff (users, products, nodes, taxonomy terms, vocabularies, etc). You can configure or delete them from operations there and you should have your create link right there at the top.

Attached patch is a total hack but some ideas might be possible to carry into a fine implementation.

1. Added a new flag to tabs: MENU_IS_LOCAL_ACTION.
2. Added filter to menu_local_tasks() to filter to this. (Added this filter to its internal cache key).
3. Added rendering of these into the template.php in seven (no other themes yet).
4. Applied some hack to the CSS, so that the + icon always shows.
5. Modified two menu module paths to use this for the Add action.

A couple problems/questions with this approach:

- the action shows on all paths on the same level (due to how it is a tab after all), which is certainly not desired - it should not show on sibling tabs to the main overview tab or on the add page itself
- distinct icons are not supported for distinct actions - what kinds of other actions should we support here additionaly to add?
- there is no rendition of the action links with the help text; should action links show above or below the help? (the patch shows them below, which kind of obscures them to a degreee)

So the patch adds the Add menu and Add link actions as can be seen here:

If we're going this route (not necessarily a bad one), we should bear in mind #484234: Big task cleanup: Remove type and tasks from hook_menu() or possibly make that one a dependency - last look it wasn't too far off.

Well, the experiment with reusing the local task code indicates that we might actually need to build these with simple menu items instead, which would only show on the page they were added for.

That would actually map to #460320: Standarized, pluggable entity loading (nodes, users, taxonomy, files, comments) - defining entities in code, along with their tabs/tasks/whateverYouNameThem. Menu router (node_menu()) becomes

  $items['node/%node']...

and all below is moved into the entity. Edit a field? Get me the local tasks for %field. Edit a block? Get me the tasks for blocks. Edit a node? Get me the tasks for %node. etc.pp.

The very same applies to separating action tabs from tasks. (whatever "buttons" are in this language)

Local tasks for menu_get_object() are derived from the entity, rendered as local tasks. Plus any other sub-router items defined via hook_menu().

"Buttons" use a special MENU_* 'type', which makes them '_visible' => FALSE by default, so they neither show up in the menu tree, nor as local tasks. admin_menu feels free to override that, so they show up in the menu tree. The logic for local tasks keeps the same, disregarding the '_visible' property.

Ok, here is a patch with an updated approach. I've used menu items resembling normal menu items, so they will show up in the menus by default. That might not be what we want (in which case we should require all themes to support this action area), but we need some feedback on that I guess.

So I've introduced a new menu item type called MENU_LOCAL_ACTION. That got a standalone function to grab child actions for the current page. Then converted the two menu administration items (which turned out to be surprisingly good guinea pigs), and applied the same changes to the theme as in the above patch.

Now this grabs local actions nicely on pages, where the parent is not a wildcard item (does not have %something), but as long as %something is on the router path, the menu_links table stops giving parent info, so we'd need some kind of other trickery to make this work. Anyway, this would indeed collect related actions under an action area as @sun explained, so it would be easy to get local actions from the menu even as opposed to trying to cherry-pick them from the tabs which contain related and other views of the content at hand.

I'd love concept feedback and specifically menu help on the parenting which does not work yet for the list menu page. (@todo in code).

Screenshot still as above (for the main menu page).

Why do we need the new type?

Let's just make them MENU_NORMAL_ITEM and put them manually into the page where needed.

This sounds like a solid idea -- it never hurts to make things more semantic. ;)

If we go this route, I'd definitely suggest that we rename MENU_LOCAL_TASK to MENU_LOCAL_TAB. (To be honest, I'm not sure the 'local' part is really valuable.)

Please keep the separation of the semantic meaning *_TASK and actual rendering (tab). We might end up using completely different interface for D8 for tasks.

MENU_LOCAL_ACTION seems consistent and semantic for me.

@kika, if we go with a completely different interface, we can rename the define to make more sense for said D8 interface? Until then, I'd recommend that we use clear names. Anyway, the idea is that "tabs" become "listings" rather than "tasks" or "actions". We can debate about terminology separately.

I think this is blocked on #204077: Allow menu links pointing to dynamic paths.

@pwolanin: the MENU_LOCAL_ACTION is a MENU_NORMAL_ITEM except it also has a bit for MENU_IS_LOCAL_ACTION. So it will show up in the menu by default but does not need any special hooks to list action items in the action list. Menu tools like admin_menu can highlight these items differently, and as @sun said above, the list of actions for a particular item can be retrieved from the menu too (not that I'm specifically aiming at this one, but it might be a nice side effect).

@Gabor - you seem to be confusing somewhat router items versus links.

I'm not clear on the actual use of these items - can they have multiple levels?

I'm going to suggest that they be manually added to the page versus trying to put in a new mechanism in the API.

@pwolanin: There is no parenting in router items (except for local tasks), and our job here is to "list all action type children of the current menu item", so how would I get that data from only the routers?

@Gabor - the "current menu item" is a router item.

We have a fairly developed mechanism for the local tasks - I think if it's really essential to have an automated way to put present "tasks" then the tasks should be a variant of a MENU_LOCAL_TASK. Also, I'd prioritize this equally or greater: #353208: Allow for runtime-conditional local tasks (hook_menu_local_tasks_alter())

@Damien - the issue here is somewhat different - we already can fill in the parts for local tasks based on the path.

@pwolanin: action items are not like local tasks in that there is no default action and local actions are only shown right under their parents, unlike tabs which are shown with all siblings. So local actions are way more like normal menu items compared to local tasks. Of course if you say regardless of their apparent lack of local task behavior (see the previous patch above for what happens if we apply the local task logic to them which I've attempted before) we should still make them local tasks because there is special casing parenting code for local tasks, we can also go that path, but then we can to bend that towards this behavior as well, making them even more special in some ways. As said, I've tried this path above, adding a marker bit to local tasks instead of making these items more like local tasks, but as the demonstration shows, treating them like local tasks is not the UI experience we want.

Well, looking at this current implementation http://api.drupal.org/api/function/menu_local_tasks/7 it's also sad that we are returning rendered data versus structured data. We should fix that as well.

@pwolanin: the code was inspired by the tab rendering code which does the same. Let's get a direction for the internal implementation first I'd say!

@Gabor - ok, yes - I'm getting distracted - looking at your previous patch...

A little rough, but I think this basically working.

Re big local task cleanup, unlikely to happen for Drupal 7 unless this issue really badly needs it.

Also? More links? #511218: Is this usable?: duplication of user menu links between Toolbar & Garland

Not more. Seperation of two different kinds of links that are lumped together now.

My God yes! The UI improvement this would enable is badly needed. I see this setting a clear guideline for contrib, improving the overall interface consistency and ultimately improving UX.

There was a lot debate about this distinction at #196758: Having delete as a button on the node edit form means certain users don't have access to it when they should, ultimately stalling the issue. Myself and others argued against making delete a local task specifically because tab styling should only be used for different data views. I see this issue as a blocker for moving delete (if that is even a good idea in the first place is another story).

The help test is failing due to how $root_path is sometimes becoming empty in the function call. This is due to the $root path not assigned to the $empty['root_path'] key by reference, so although it was already counted, it was not remembered in the array. This is for the return at the end

Reroll with bugfix attached.

I think this still needs a litle work to make sure the local actions are displayed on all core page templates.

Added $local_actions to default list of variables, documented in page.tpl.php. Garland and Stark have a default list treatment for the links. I'm not at all convinced that this is the way we should do it in Garland, but Stark is a starter theme, so might not be bad this way (even though tabs got their styles).

Looks in Garland:

Also, Mark had a longish discussion with UX people from the community and turned out that there can be a diverse type of actions, so making up special small icons for them is not extensible. So instead of going forward with icon-links, he suggested we go with buttons. This updated patch also includes that visual change.

New looks in Seven:

Previous direction of text link was just fine. These buttons are really awkward looking in this position above the main content of a page. It's not like there will be 10 different kinds of actions. Currently we have 1 spot in core that shows 2 actions, which is in forums, where you can add a forum discussion and a forum container. So, both variations on add, even.

I'd say, keep the focus on the actual seperation of actions from the tabs. Redesigning the look of actions is better served in a follow up, for now, please revert to the text links.

- Added theme_menu_local_action() to resolve the remaining @todo.

- Fixed some PHPDoc.

- Reverted the change from '_tab' to '_is_task', because "tab" correlates to some other menu system properties (tab_root, tab_parent, etc.)

- Changed the template variable to $action_links (likewise, the CSS class to .action-links) - we do not expose those internal "local" names to themers.

Looks good to me.

- Fixed missing registration of new theme function.

- Removed button style for action links.

- Added style to render multiple action links horizontally.

Now ready to fly.

For DX's sake, I still think it is useful to rename MENU_DEFAULT_LOCAL_TASK to something else; TASK and ACTION feel like they are too close to another, and therefore potentially confusing. I'd suggest to rename xxx_TASK to xxx_TAB. We can do this in a follow-up patch, if desired.

Otherwise, this patch looks 100% ready.

Other things which could be fixed, either in this patch, or follow-up patches:

* "Add new content" on ?q=admin/content
* "Add user" on ?q=admin/people
* "Add block" on ?q=admin/structure/block
* "Add content type" on ?q=admin/structure/types
* "Add menu" on ?q=admin/structure/menu
* "Add vocabulary" on ?q=admin/structure/taxonomy
* "Add style" on ?q=admin/config/media/image-styles

The renaming of the internal defines I'd like to defer to a follow-up patch, but ideally to another issue, because that's highly technical.

In this issue, I want to submit follow-up patches to convert the remaining actions like you stated, so the UX team can review + focus on those. I already started with that work.

+++ includes/menu.inc	22 Aug 2009 19:05:44 -0000
@@ -168,6 +173,14 @@ define('MENU_LOCAL_TASK', MENU_IS_LOCAL_
+define('MENU_LOCAL_ACTION', MENU_IS_LOCAL_TASK | MENU_IS_LOCAL_ACTION);

I agree with Dries that the distinction between a "task" and an "action" will be a total WTF to new developers. But I could see this getting into bikeshed territory, so agree with pushing it off to another issue.

+++ modules/system/page.tpl.php	22 Aug 2009 17:48:56 -0000
@@ -171,6 +173,7 @@
+        <?php if ($action_links): ?><ul class="action-links"><?php print $action_links; ?></ul><?php endif; ?>

I also brought up that that is ugly as sin, and ought to only be print $action_links; like we have print $primary_links and such. But sun pointed out that this is only conforming to existing code. Fair enough.

Since the task conversion requires this scaffolding in place, and Dries seems fine with it other than these two things, I committed to HEAD. Needs documenting in the module upgrade guide.

I'm on crack. Are you, too?

@sun, sounds great! I've committed the patch in #41 so we can focus on the follow-up patches. Thanks!

hah... obviously, something went wrong: http://drupal.org/cvs?commit=254832 :-D
Seems like we have 2 different new features now ;) Not sure to which issue this commit belonged to...

oopsie, didn't intend to revert all of that.

Follow-up patch is still in the workings.

This patch additionally converts all local tasks that Dries mentioned earlier.

However: Action links do not appear on admin/structure/block, nor on admin/structure/block/list, nor on admin/structure/block/list/seven. I've tried to fix that, but ended up in menu system nightmare. I'll create a separate issue for that (perhaps mixed with the renaming of defines), because that may require quite some rewriting of menu_local_tasks() for actions.

In IRC Bojhan is pointing out other places that did not get converted, so slapping it out of my queue. HA! ;)

To all of my beloved webchicks + Bojhans out there...

In parallel, I've opened a new, critical bug report about not properly working action links on some pages: #556872: Action links do not work on all levels (example: 'add block' action link does not appear) (including renaming of the defines, for Memphis' sake)

I can't replicate dblog test failures locally. Requesting a re-test.

Block is missing a place to add a block. This is a major problem, and holds this patch from being committed. The rest of these could be moved to follow-ups.

"Import OPML" is not really an add operation, but that can be debated so we don't have to fix here, but just to point out that we ideally will need more metadata on these including icons...

Roles page is inconsistent:

I would expect to see Add term links from taxonomy:

Ok. Sun informs me that the block issue is being sorted out at #556872: Action links do not work on all levels (example: 'add block' action link does not appear) and needs a menu system guru.

Marking back to RTBC to see if testing bot likes the patch any better.

Moved those roles/trigger/whatever specially crafted UIs into #556892: Revamp administrative "inline" forms to create new objects.

Hm. Bot is being pokey in returning a result, but http://testing.drupal.org/pifr/file/1/11970 shows a pass.

Committed to HEAD. Still needs work for documentation and follow-up issues.

While at it, should blog.module converted too?

blog.pages.inc line 69:

function blog_page_last() {
  global $user;
  $build = array();

  if (user_access('create blog content')) {
    $items[] = l(t('Create new blog entry.'), "node/add/blog");
    $build['blog_actions'] = array(
      '#items' => $items,
      '#theme' => 'item_list',
      '#weight' => -1,
    );
  }

if blog.modules is done, I assume forum.module should also be done? ("Post new Forum topic" plus all the other content types).

Note that this patch introduced inaccessibility into Drupal. Please follow-up at: #521852: Local tasks lack semantic markup to indicate an active task

The inaccessibility mentioned in that issue was already there before this patch.

I don't necessarily agree with Bojhan that this is an admin-only interface pattern, but for now I agree that we should limit the scope to administrative pages. However, the documentation for this should not include this (fake) limitation.

Yeah, I don't agree either that this is an admin-only pattern, but will see how we can apply it elsewhere.

Re-visiting this issue - can we please convert those remaining, non-administrative links that are currently stuffed into an otherwise clean main page content output, too?

I just recalled that I had to fork forum/blog theme functions a couple of times, just to be able to display those links differently. The new $action_links variable would have saved me from doing that.

Is there any simple way to add the required "rerouting" of the menu links or would this need to be done through page redirects?

The problem is that node/add/blog is not a child of the blog/% page, nor are any of the node/add/% of the forum pages...

I am not sure how this should be handled - through dummy redirects (such as something like blog/add/new redirecting to node/add/blog) or is there anther way?

And can we please not forget the bikeshedding on MENU_IS_LOCAL_TASK | MENU_IS_LOCAL_ACTION?
Shipping that in our code will be a big DX WTF.

@joachim: No regular developer has to work with those constants, because they are internally used by the menu system only.

@nbz: Yes, we have http://api.drupal.org/api/function/hook_menu_local_tasks_alter/7 now, so Blog and Forum modules can add actions to certain pages - which is why I re-opened this issue.

Please also note that we have some new ugly code in http://api.drupal.org/api/function/node_admin_content/7 that's similar to Blog/Forum.

So the actual task at hand is to write some respective hook implementations for those modules + drop the hard-coded action links in the markup. Should be quickly doable. Any takers?

This bikeshedding issue: #607656: rename one/both of MENU_LOCAL_ACTION / MENU_LOCAL_TASK ?

@sun surely anyone who develops a module that uses hook_menu has to use those constants? And anyway, is having bad code solely internal an excuse?

oh yay - this actually revealed a critical bug in menu_local_tasks() :)

Fixing the @todo in menu_local_tasks() should be done in a separate issue, because I assume we will have to split the retrieval of actions from tasks (i.e. move the logic into the existing stub function menu_local_actions()).

Aside from that, this works.

Created follow-up #630256: Local actions are not returned when there are no local tasks

Patch looks great, I've similarly had to do horrible things just to modify those links slightly. RTBC.

+++ modules/forum/forum.module	11 Nov 2009 22:44:33 -0000
@@ -134,6 +134,62 @@ function forum_menu() {
+function forum_menu_local_tasks_alter(&$data, $router_item, $root_path) {

I think it would be good to document why are we using hook_local_task_alter instead of hook_menu()?

$data is a rather poor name.

+++ modules/node/node.module	11 Nov 2009 21:28:25 -0000
@@ -1920,6 +1920,22 @@ function node_menu() {
+function node_menu_local_tasks_alter(&$data, $router_item, $root_path) {
+  // Add "Add new content" as local action.
+  if ($root_path == 'admin/content') {
+    $item = menu_get_item('node/add');
+    if ($item['access']) {
+      $data['actions']['output'][] = array(
+        '#theme' => 'menu_local_action',
+        '#link' => $item,
+      );
+    }
+  }
+}

Why do we need to do this in _local_task_alter()? Sorry, I think my brain isn't working yet.

So the reason we do that in _alter() is because you can't have a 'normal' local task without the router path being at admin/content/add or wherever (either duplicated or a redirect) - whereas alter doesn't require a router path. But both places could definitely use the docs to point this out, especially since this may be the only implementation of hook_menu_local_tasks_alter() in core.

err. I have difficulties to understand what's required to get this patch in.

The hook is documented already and not invented here. $data could perhaps be renamed, but that doesn't belong into this issue nor patch. This patch just implements an existing hook in some places. Not sure whether we explain every single hook implementation. Can someone explain to me what's exactly missing?

I'd do "// Add "Add new content" as local action. We use hook_menu_local_task_alter() to avoid creating a new router path"

Except wrapping at 80 characters.

Re-rolled against HEAD.

Changed the inline comments so that they state what they actual do: "Add action link to 'some/where/else' on 'other/page'."

The menu system only supports local tasks and actions that have the tab_parent router path as their ancestor. You can do a local action 'admin/content/add' and that will be displayed on 'admin/content'. But you cannot do a local action 'node/add' and display it on 'admin/content'. That is one of the use-cases the alter hook was implemented for.

Hence, this patch simply implements this alter hook to remove a theming WTF and bring consistency into the output.

Those comments look better, back to RTBC.

Is this supposed to bring back the 'Add new block' link on the block administration page? If so, I don't think it is working.

Nope, that's either #556872: Action links do not work on all levels (example: 'add block' action link does not appear) or #630256: Local actions are not returned when there are no local tasks

Committed to CVS HEAD. Thanks!

The unintended commit mentioned in #45 was never reverted. I just filed #804844: Revert revision 1.29 in default.settings.php regarding this.