This issue is about making it easy to put in-context edit links on anything in Drupal that uses the new page rendering system (nodes and blocks for now - hopefully menus, fields, and other things later). This is based on http://www.d7ux.org/edit-on-page/ - however, this issue is specifically not intended to be about modal dialogs, inline editing, or some of the other, more complicated ideas discussed in that thread. It is, however, a critical step to enable those kinds of things to happen. And hopefully it will be a big usability improvement in its own right...

See the attached screenshot for an example of what this looks like in its initial form. Each user sees links on each object on the page that they have access to edit. The link takes them directly to the page where they can edit this object. After their edits are complete, they are automatically redirected back to the page where they came from.

To use the new system, developers simply need to put an #edit element on the return array of their page callback, which contains the URL of the link to edit the object. There are other optional elements they can include, but that is the only required one. The system automatically figures out whether the current user actually has access to perform the edits and, if so, provides appropriate variables to to the template file to display the edit links (as usual, these variables can be used or modified however you want). In essence, the system is designed to privilege edit links throughout Drupal and make a consistent, simple way for developers to mark different types of objects as editable.

In this patch, I also added some simple JavaScript that highlights the editable region when hovering over the edit link (as shown in the screenshot - you have to imagine a mouse cursor hovering over the "Management" edit link since I don't know how to get cursors to appear in screenshots). This might not belong in core, but it's useful for demo purposes, and it shows how the CSS classes provided by this patch can be used in interesting ways.

Things that still need work:

  1. The theming of the edit links is pretty ugly in some places.
  2. Some of the newer, "non-traditional" blocks in Drupal give odd or confusing edit links (for example, the System Help block, and in some ways the Main Page Content block) but for now I have not tried to exclude them from printing edit links. This could easily be done via hook_block() if we want to.
  3. There should perhaps be a theme setting or permission that allows toggling off these edit links entirely, since there are certain types of sites and user roles that might not want these links appearing at all.
  4. As mentioned, the next step is to extend this beyond nodes and blocks. Menus are probably the next easiest target; they would mainly need to be modified to use drupal_render() to return an array within the block that renders them, and then hopefully everything will work pretty nicely. Fields would be really nice as well, but that's more complicated since there is no edit interface for individual fields yet.

Comments are certainly welcome on the overall direction here - thanks :)

CommentFileSizeAuthor
#204 drupal.edit_.204.patch3.47 KBsun
#203 drupal.edit-follow-up.patch2.26 KBsun
#199 drupal-edit.199.patch38.01 KBsun
#198 drupal-edit.198.patch37.69 KBsun
#197 drupal-edit.196.patch36.53 KBsun
#195 drupal-edit.195.patch35.99 KBsun
#194 drupal-edit.194.patch32.76 KBsun
#192 drupal-edit.192.patch33.78 KBsun
#190 drupal-edit.190.patch30.77 KBsun
#188 drupal-edit.188.patch28.07 KBsun
#184 drupal-edit.184.patch29.66 KBsun
#183 drupal-edit.182.patch29.67 KBsun
#181 drupal-edit.179.patch25.48 KBsun
#177 drupal-edit.177.patch20.07 KBsun
#176 drupal-edit.176.patch20.44 KBsun
#175 drupal-edit.175.patch24.79 KBsun
#174 drupal-edit.174.patch26.8 KBsun
#172 drupal-edit.172.patch21.78 KBsun
#170 drupal-edit.168.patch22.22 KBsun
#167 drupal-edit.164.patch22.2 KBsun
#163 attached-links-on-everything-473268-163.patch34.34 KBDavid_Rothstein
#158 attached-links-on-everything-473268-158.patch33.89 KBDavid_Rothstein
#155 EditDashboard.png35.39 KBGábor Hojtsy
#154 EditIcons.png194.9 KBGábor Hojtsy
#152 attached-links-on-everything-473268-152.patch31.16 KBDavid_Rothstein
#133 drupal-edit.134.patch25.33 KBsun
#132 drupal-edit.132.patch25.76 KBsun
#130 drupal-edit.129.patch39.47 KBsun
#128 drupal-edit.128.patch25.85 KBsun
#126 drupal-edit.126.patch24.43 KBsun
#122 drupal-edit.122.patch24.44 KBsun
#120 drupal-edit.118.patch24.37 KBsun
#114 attached-links-on-everything-473268-114.patch25.55 KBGábor Hojtsy
#112 node_links_yhahn.png66.09 KBDavid_Rothstein
#110 attached-links-on-everything-473268-110.patch24.35 KBDavid_Rothstein
#106 edit_everything-473268-106.patch24.51 KBGábor Hojtsy
#102 edit_everything-473268-102.patch27.58 KBGábor Hojtsy
#102 NoDelete.png55.2 KBGábor Hojtsy
#99 edit_everything-473268-99.patch26 KBGábor Hojtsy
#97 edit_everything.patch26.02 KBGábor Hojtsy
#94 icons_1_0.zip1.1 KBseutje
#94 clip.png59.14 KBseutje
#93 edit_everything.patch24.99 KBseutje
#90 473268_admin_links_90.patch25.18 KBGábor Hojtsy
#87 473268_admin_links_87.patch27.36 KBsign
#87 admin_links_garland.png4.55 KBsign
#87 admin_links_garland_hover.png21.19 KBsign
#87 admin_links_seven.png28.08 KBsign
#85 473268_admin_links_85.patch27.2 KBsign
#75 MetaBar.png96.79 KBGábor Hojtsy
#72 473268_edit_links_on_everything_72.patch27.11 KBDavid_Rothstein
#70 473268_edit_links_on_everything_70.patch27.2 KBsign
#66 edit_links_on_everything-473268-61.patch28.12 KBseutje
#66 edit-links.jpg235.84 KBseutje
#60 edit_links_on_everything-473268-60.patch26.96 KBcwgordon7
#60 d7ux_edit_screenshot_60.png56.35 KBcwgordon7
#49 edit_links_on_everything-473268.patch27.64 KBstella
#47 editeverywhere.jpg180.63 KBBojhan
#44 edit_links_on_everything-473268-41.patch28.65 KBseutje
#42 edit_links_on_everything-473268-40.patch28.65 KBseutje
#40 edit_links_on_everything-473268-39.patch28.65 KBseutje
#38 edit_links_on_everything-473268-38.patch29.27 KBcwgordon7
#38 icons.zip1.1 KBcwgordon7
#38 d7ux_edit_screenshot_38.png51.35 KBcwgordon7
#31 d7ux_edit_screenshot_31.png85.16 KBcwgordon7
#30 edit_links_on_everything-473268-30.patch30.15 KBcwgordon7
#30 icons.zip1.21 KBcwgordon7
#26 edit_links_on_everything-473268-26.patch24.49 KBDavid_Rothstein
#25 edit_links_on_everything-473268-25.patch21.72 KBtstoeckler
#24 edit_links_on_everything-473268-24.patch24.43 KBDavid_Rothstein
#24 editable_elements_with_menus.png72.06 KBDavid_Rothstein
editable_elements.png65.97 KBDavid_Rothstein
edit_links_on_everything.patch15.07 KBDavid_Rothstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcrittenden’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
Issue tags: +Needs usability review

Marking this critical since it's a huge part of what Mark and Leisa are going for. Also, not sure if it should be posted against the theme system or not...seems to go a little deeper than that.

As mentioned in the blog thread, my ultra-quick initial concern involves Views. To the typical site editor (i.e., the client), content is content, and it will be confusing to tell them that this content (i.e., a node) is editable but this other content (i.e., a view listing nodes) is not editable, if there are edit links to both. Clients can usually understand that the content listed in the admin content list is all editable and everything else is not, but if there is an edit link on everything, including a view, that just goes to the views interface, chaos will ensue.

The alternative is to leave edit links off of views, but this isn't a very happy compromise.

Blocks could arguably be the same scenario. A lot of blocks are too complicated for clients to understand how to edit.

Dries’s picture

Issue tags: +Favorite-of-Dries

Tracking it!

Status: Needs review » Needs work

The last submitted patch failed testing.

ksenzee’s picture

@mcrittenden: I agree that if someone shouldn't be editing something, they shouldn't see an edit link for it. In theory that's easy enough to control with permissions. I usually don't give the kind of site editor you're talking about permissions to edit views anyway. If they absolutely need it, there's always Simpleviews.

The blocks scenario is more of a concern. Probably the right way to handle it is to improve the blocks interface, though, which I assume Mark and Leisa are working on.

Gábor Hojtsy’s picture

Thinking Views, that particular module is also famous or outputting editing links on its own. However, it also outputs other similar action links. How would this feature support Views in doing that? Would a View need to replicate this or are you planning on supporting more then just edit links? I am thinking #action_links or something, but that is probably too directed at the theming implementation (links). Anyway, it would be good to know how this relates to what Views does, which is conceptually quite similar.

David_Rothstein’s picture

Gábor: Good question about the Views links! I had thought about that a lot while working on this patch, and originally I was planning to implement it in a similar way as you describe (I was actually thinking we'd call it something like #local_tasks, by analogy with the terminology used in the menu system).

Perhaps we should still look into this, but there were two reasons I didn't do it that way for now....

1. I wanted to keep the initial patch as simple as possible.

2. I think it's important to make it very easy on developers - our goal should be that every module author who outputs an object on a page finds it very easy to use the same, standard method for marking the object as editable. If we tried to make it too generalized, then we might not actually be saving them any work over what they could do today on their own.

Perhaps we could get some mileage out of looking at how node links are rendered in D7? - basically, a function like http://api.drupal.org/api/function/blog_node_view/7 puts them in a specific place in $node->content, and then http://api.drupal.org/api/function/template_preprocess_node/7 renders them into a variable. I think that's as simple as you can get for a general list of links. So we could basically implement a #local_tasks sort of thing just like that (by taking what nodes do and moving it up to the page region level instead), but it seems that would be a bit more confusing of an API for developers to use? Instead of setting $element['#edit'] they would probably have to set something like $element['links']['edit'] and would probably need to fill in more information for it. Not sure what's best here... The "special casing" of edit links that I'm doing in this patch certainly doesn't feel 100% right, but on the other hand, maybe edit links really are special in some ways since they are common to just about any type of object (and, for example, might be rendered by a theme via a special kind of 'configure' icon, etc).

Anyway, to answer your specific question about how Views would work with the current patch, I think it would be quite easy. The patch provides an $edit_link_info variable containing the unrendered edit link, and I believe Views in D6 uses its own template_preprocess function to output its links. So if Views wanted to include the edit link as part of its standard list (rather than theming it separately), it could just insert $variables['edit_link_info'] into the array of links that it already has and everything should work OK..... (Although actually, thinking about this made me realize there's a bit of a bug in my patch - some of the CSS classes are in the wrong place and should actually be attached to the link itself rather than the "list" of links, but that's easy to fix.)

David_Rothstein’s picture

Regarding the other comments above, just to clarify: The way the patch currently works, each user only sees edit links for things that they, personally, have permission to edit.

I think the fact that the links might be confusing to some users is perhaps a good reason why the visibility of these edit links should also have an overall permission associated with them, so you can hide them from some classes of users altogether. Also, the way the patch is written, it should be easy to write a module that provided even more fine-grained control (e.g., hiding edit links on certain objects while showing them on others, regardless of the permissions the user has).

I also agree that blocks are going to get crazy and agree with @ksenzee that the solution is to fix the blocks interface. In fact, one thing I noticed while playing around with this patch is that putting these edit links everywhere has a tendency to surface preexisting usability issues that were there in Drupal all along but that I personally never noticed before. The edit link on the "Powered by Drupal" block is an interesting one for that... if you click on that link with the intention of changing the Drupal icon's color and making it bigger, boy will you be happy at how easy that makes things for you! However, if you click on it with the intention of getting that Drupal icon off your site altogether, you will be mighty confused because Drupal does not provide any way to disable a block when editing the block itself, only when viewing the overall blocks configuration page. You'll probably wind up fiddling with the page visibility fieldset or something, and disaster will ensue :) We'll also certainly see issues with blocks containing menus - if menus get edit links then there'll be "Edit the Management block" and "Edit the Management menu" links appearing right next to each other, which reflects the fact that in Drupal, these types of blocks require you have to go to two entirely different screens to configure the properties of the block and the actual content within it. So on and so forth. The red-border-javascript-hover thing I included in the patch was a partial attempt to address this by at least giving you some visual indication of which actual thing you'll be editing before you click on it, but it's not a real solution to the problem.

So I think if we continue down this road we'll continue to see other types of usability issues "revealed" by the patch, and hopefully there will be resources to fix these kinds of nuts-and-bolts issues also -- they are very important, but at the moment at least, they don't really seem to be the main focus of the D7UX effort (that could change as the designs and implementations get more specific, though).

catch’s picture

$elements['#links']['edit'] doesn't sound so bad - I can think of plenty of places where you might want a delete link, and having a separate API for that would be more confusing than the extra little bit of work to get an edit link there. Either way subscribe.

Gábor Hojtsy’s picture

Depending on the block, you might want to be more accurate by saying "configure" and not edit. The "powered by" block is a good example. Or an aggregator block. You actually just have some knobs to move, but not a whole "edit" experience. Also, supporting "delete" would help solve this usability issue to remove stuff from the page. However, that might conflict with whatever MBD has in mind for page/layout organization.

Also, I was thinking how this relates to users configuring their custom blocks. They are able to disable and enable them via their user edit form now. Is that something of an "edit"/"configure" thing for a user, or are we totally focused on just admins with this UI? (My instinct is that we should focus on admin only, just had this idea in my head, and could not resist to share :)

yoroy’s picture

Some quick thoughts while i subscribe to this issue :)

- maybe the best initial scope is to only provide *Edit* links on *Content*? Since Configure, Delete etc. takes this initial patch straight into all kinds of (semi) edge cases.
- I would expect the edit link itself to only show on hovering the element. Or is the 'show editable items' mode switched on globally? (Edit button in the proposed d7ux header)
- Just a single 'Edit' should be enough, no need to repeat the element title, that is sure to break layouts. But do show a border, this nicely outlines the part you will be editing if clicked.
- Have you seen how Zen theme does this for blocks? Nice and small text links top right, with links to block configuration and also to menu config if the block is a menu. I made a Zen sub theme that replace the text links with icons with the text moved to the links title attribute to make them show up as a tooltip. Not saying that this should be part of this patch, but icons are def. something to keep in mind here, re: screen real estate.

Dries’s picture

+1 for what yoroy said in #10. :)

dvessel’s picture

subscribing

David_Rothstein’s picture

The icons look great! I purposely did not put any work into theming the links nicely yet - so if we could commit this patch without any theming that would be fine with me, but if not, I'd much rather get it working with the icons than having us spend lots of time getting the text links to line up correctly on the page when we aren't going to keep that anyway :) Also, I sort of had in mind that the "longish" text I wrote might be a good candidate for a tooltip anyway. Are these icons OK to put in core (GPL, I assume)?

For now, I think we should still keep block configuration links as part of the patch. It's good for testing things out, and certainly should be the long term goal. We can always remove it right before committing. Frankly, if we can't get it working nicely for blocks and menus I'm not sure the patch is that exciting - Drupal already makes it relatively easy to figure out how to edit content, but getting to the configuration screen for a particular block or menu currently involves a very long series of unintuitive clicks which this patch would remove.

So I think before going much further with the code we should really figure out whether we want to keep this as an #editable property attached to page regions (as in the current patch), or instead switch to a more generic #links or #actions property, also attached to page regions. To clarify, it seems like from a "developer-facing" perspective the latter option would look, at a minimum, like this (example from node.module):

   $build += array(
     '#theme' => 'node',
     '#node' => $node,
     ...
     '#links' => array(
       'edit' => array(
         'href' => 'node/' . $node->nid . '/edit',
       ),
     ),
     ...
     '#teaser' => $teaser,
   );

And we'd still then need some special-case code (either in hook_page_alter or drupal_render itself?) that went around looking for these and applied certain default behaviors (e.g., setting 'query' => drupal_get_destination(), etc).

Waiting for consensus here, I guess :)

David_Rothstein’s picture

Also, I was thinking how this relates to users configuring their custom blocks. They are able to disable and enable them via their user edit form now. Is that something of an "edit"/"configure" thing for a user, or are we totally focused on just admins with this UI? (My instinct is that we should focus on admin only, just had this idea in my head, and could not resist to share :)

Interesting point - certainly it would be for a followup patch rather than this one, but worth thinking about anyway :) It points to the fact that the same item on the page might actually have two different "edit" links that go to two different URLs, which certainly isn't something I considered. I guess that would be an argument in favor of having a more generic #links-type property rather than a single #edit one.

David_Rothstein’s picture

Issue tags: +d7ux-design-question

I'm adding the 'd7ux-design-question' tag since some of @yoroy's questions in #10 are related to that...

Specifically, regarding the question in #10 of (a) showing the edit links only while hovering over the element vs. (b) showing them only when a global 'show editable items' mode is on vs. (c) showing them in all cases, I tried to write the patch so that it would easily support any of those options with only a tiny amount of extra JavaScript required (hopefully!).

Certainly in the case where there is no global "Edit Mode" button we need to provide some other kind of default behavior. I tried out Zen and have to admit that I found the "hovering" behavior confusing, at least for some blocks... It works well for a small block, but for any element that takes up a larger region of the page (the Powered by Drupal block is a good example of that), I found it awkward when I would put the mouse in one area, see the links appear, and then try to slide my mouse over to them but have the links suddenly disappear as soon as my mouse slipped off the 'one true path' - just my opinion, though :)

David_Rothstein’s picture

On the other hand, Zen does not have the 'red border' as included in this patch, so perhaps with that combination the hovering behavior would be more intuitive....

(By the way, the red border is clunky right now since when it appears it increases the width of the page element that it encloses and therefore breaks the layout as well as causing "jittering" when you hover over it. Any JQuery experts out there know a simple, clean way to fix that? I can think of some ways that might work, but none of them seem very clean.)

univate’s picture

This is a implementation of the same inline editing concept that Mark and Leisa seem to be aiming for...

http://incontextediting.adobe.com/

One thing I like about this example is that you have to click on the "edit" button to enter the edit mode before you get all the extra links/icons and effects to show you are editing stuff. I have also played with the contrib admin_hover module and the one thing I don't like about that module is that you can't turn it off without logging out.

You want content editors to be able to go to a page switch into edit mode and make any edits and then switch it off again and see the site exactly as should appear to everyone else.

mcrittenden’s picture

Re: #16, you might try an outline instead of a border since a border goes outside the bounding box (thus increasing size) while an outline stays on the inside.

Dries’s picture

The Adobe approach shown in #17 is nice because (i) the icons don't get in the way when you're not planning to edit the page, and (ii) allows for more complex interactions with the page's elements. Looks more scalable and flexible to me.

Noyz’s picture

subscribing. Also plus +1 on yoroy's #10. Re #17, I like the way the icons show, but personally struggle with what's region vs editable area. I think squarespace.com does this better.

dman’s picture

Subscribe - I liked what views and zen did to bring this UI method to the fore.
Has everyone checked out http://drupal.org/project/admin_hover - a contrib that does a lot of this already?
While I don't like the UI that much, the idea is great. Conor may have some insight in to this roadmap.

There has also been
http://drupal.org/project/block_edit - which will also be made redundant by this improvement.

jp.stacey’s picture

Subscribing. Linking the user's visual attraction to regions of the page - "here's a thing I want to edit" - with the actual action - edit, move, delete - is definitely a big d7ux priority.

We've done some work on adding inline links to node titles. It basically uses preprocess and views hooks to add edit links to the end of node titles, so it's nothing majorly clever. The biggest difficulty was that node titles pass through check_plain(), so you have to use HTML-safe tokens and then rewrite them as links with Javascript. We also use Thickbox to open the edit link in a lightbox overlay.

There's an alpha module for editing inline in subversion if anyone's interested. It could be a good complement to the similar links proposed on block and the existing links that Gábor mentions on views (if we keep them - we don't want too much page clutter.)

michaelfavia’s picture

Subscribing.

David_Rothstein’s picture

Here is an updated patch. Some changes:

  1. Edit links are now on menus as well - this required some tricky changes in how the page rendering is done, but it is working. The goal of trying that now is to allow more realistic testing of how this "edit links on everything" feature works with nested structures (menus in blocks are only one use case for this; nodes within views would be another important one). I thought it might be confusing to have multiple overlapping editable regions, but with the red border showing you what you are editing, it doesn't seem as bad as I thought? - see the attached screenshot (editing a menu) and compare to the original screenshot in this issue (editing a block).
  2. There is now a very rough implementation (and I mean very) of the "global edit mode" from the D7UX designs at http://www.d7ux.org/edit-on-page/ included in this patch. This is not something that would go into the final version of this particular patch, but it's there for now to give an idea of how this feature might work. Under normal operation, the edit links work as before (always present, and hovering over them gives a red border), but when you click on the page title to turn the "global edit mode" on, instead you get a scenario where hovering anywhere on an editable region of the page draws the red border, and clicking anywhere within that region takes you to the edit page. This is ultra-clunky right now (read: barely works at all), but it does give a feel of how the process might work. Basically it makes Drupal start to feel like Firebug :)
  3. I switched from using CSS borders to CSS outlines (thanks @mcrittenden!), which does remove the jitter - although I'm not sure that is cross-browser compatible?
  4. I edited the text shown on links to make it shorter; however, as mentioned previously, I am deliberately not doing any serious work to actually theme these links yet, which is why they look ugly :) Note, though, that @yhahn's patch at #484820: Initial D7UX admin header contains some CSS that makes inline edit links on nodes look very similar to the way we seem to want them to. That part of his patch properly belongs in this issue, and he has said he will try to move that CSS from there to here (or certainly, anyone else is welcome to grab it and try to integrate it too).

Otherwise, this patch is still held up on going much further based on the discussion about the API, as mentioned in #13...

tstoeckler’s picture

Didn't apply cleanly -> rerolled.

Noticed that the links in your screenshot didn't appear for me. Is there anything that needs to be done?

David_Rothstein’s picture

Thanks for the reroll! It looks like it missed a couple files, though, so here is a new version.

If the "Edit this menu" links were the only ones missing, that was due to the missing files. If none of the links were appearing, try visiting admin/build/modules to rebuild the theme registry, and I think that will make them appear (this should only be necessary when applying the patch to an existing D7 installation - if you apply it before installing, the links should be visible automatically).

Bojhan’s picture

Status: Needs work » Needs review
Issue tags: -Needs usability review +Usability

Is starting to look good, not sure how the interaction now acts though. The screenshots imply there is a hover, and hover over link state? The links should probally be above.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
30.15 KB

Here's an updated patch that works with all types of links rather than just edit links and uses Young Hahn's icons (attached). Supported core link types at the moment are "edit", "delete" and "configure". The patch also fixes the above mentioned menu problem, plus a few other updates to work with the latest HEAD code. It also introduces a hook so contributed modules can add their own icons for hover links. I also added a special-case exception for the main content block because it's confusing to have a "configure" link there - what exactly is the user supposed to be configuring?

There may have been a few other minor changes I've forgotten about, but those should be all the significant ones. The icons belong in the misc/ directory.

cwgordon7’s picture

FileSize
85.16 KB

Also an annotated screenshot.

webchick’s picture

Subscribe

Gábor Hojtsy’s picture

A video which includes how this works: http://rufzeichen-online.de/drupal-ui-report-screencast-admin-overlay-la...

Some observations from what I've seen on the video and in practice so far. The icons appear when the edit mode is not turned on; I've heard this is intentional, but I'm not sure it is a good idea. Since these buttons are not the core of how we administer a site (you have other ways to get to the listed pages), they would not be required to have this kind of "fallback" IMHO.

Also, the video shows nicely that even when a block is empty, it gets this icon (see Content admin screen) and anyway, the admin pages should not have this treatment IMHO, at least as long as we don't have a real configurable admin screen. ATM only the help region has blocks which one might want to configure and the Help on/off switch to be added on the theme/overlay will make that controlled without (there clumsy) configure forms.

Finally, while the overlay is not in yet, the icon links should really open in the overlay (theoretically you just need to add a class to the links).

Status: Needs review » Needs work

The last submitted patch failed testing.

Dries’s picture

While this is still very rough, I think this is huge.

- Patch no longer applies.

- When global edit mode is off, I can still edit blocks? The icons always show up.

- I don't get icons for nodes -- just a border? I certainly expected an edit icon.

mthart’s picture

subscribing

JeremyFrench’s picture

Subscribe

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
51.35 KB
1.1 KB
29.27 KB

Here's an updated patch and updated icons and an updated screenshot. Changes include:

- Rerolled for changes in HEAD.
- Minor coding standards-related corrections.
- Removed some redundant code.
- A change in the icons to be transparent rather than have a background so that cool stuff can happen, such as making them recolorable in garland and/or different themes.
- Changed the default color of the border to be a less obtrusive color than that bright red.
- Changed the styling of the icons to eliminate the whitespace between them.
- Changed the CSS so that the icons always appear in the upper right corner of the actionable item.
- Added a menu item to do the edit mode toggling and removed the page title hack. I'm open to a better way to do this if suggested, but for now it's using the toolbar module, if present, to provide a toggle edit mode link; if the toolbar module is not available, it adds a link to the main menu.
- Removed the different editing modes - several people found them confusing, so now it's just a single behavior, which only appears when edit mode is enabled.

This patch should fix the problems outlined in #33 and #35 - except the "I don't get icons for nodes -- just a border?" comment in #35, as I can't reproduce the problem - just looking at the screenshot I posted in #31, the icons appear to be showing up for nodes. Can you clarify the problem you're having?

Bojhan’s picture

I did a clean checout of the SVN repository here, and it does pretty much nothing. I enabled edit mode, and it opend up an loading overlay?

seutje’s picture

patch seems to have went stale, rerolled for current head

seems to work as expected

seutje’s picture

Status: Needs review » Needs work

test on IE6 showed js error, causing js to quit functioning, while retaining the js class on the html element which prolly wrecks havoc on collapsible fields and other js fancypants

taking a closer look

seutje’s picture

turns out IE6 didn't like this for some reason:

        var class = '.actions-enabled-at-' + matches[1];
        $(class).addClass('active-actions-region');

so I changed it into this:

        var match = $('.actions-enabled-at-' + matches[1]);
        match.addClass('active-actions-region');

dunno if this could be considered less elegant, not a js guru, but at least this doesn't throw an error on IE6 and still works on FF3.x (didn't test any others)

seutje’s picture

Status: Needs work » Needs review

forgot status

seutje’s picture

doh, "class" is reserved

changed it back to

        var match = '.actions-enabled-at-' + matches[1];
        $(match).addClass('active-actions-region');
Bojhan’s picture

Still not working, SVN repository that is.

Bojhan’s picture

-

Bojhan’s picture

FileSize
180.63 KB

My first look at this.

editeverywhere.jpg

Obviously, we won't have floating icons ass the final design.

Gábor Hojtsy’s picture

@Bojhan: from top right to bottom left:

1. You need a toggle, because otherwise the borders and icons showing up all around would be quite counterproductive to your site browsing when you are logged in as admin.

2. Secondary menu item indeed should not be there if toolbar is enabled.

3. One of the icons is for configuring the block the other is for the menu I'd guess.

4. Yeah, the management and the powered by drupal blocks are there by default. The menus (primary, secondary), the search box, etc. are not blocks in Drupal 7. They should be, but they are not. Any work is welcome there :) Actually, the main content item is also a block, but that is explicitly excluded from this mode (see above reasoning from Charlie).

stella’s picture

The last version of the patch didn't have the 'edit this menu' link because of an error in menu_page_alter(). I think I've fixed it.

@Bojhan: it may explain your alignment errors. However I don't have the 'disable edit mode' links like you have in your screenshot.

Bojhan’s picture

1. Is debatable, but I will defer to the decision made on that for now.

2. Even without, it is going to be a mandatory link - how is this handled, a permission?

3. Its mainly the alignment.

4. I would almost consider it mandatory for this patch, as it would confuse people if there is only two block area's while the others look as block area's too.

5. On the icons issue, I will not talk about this at all - since its a utterly useless one. They have to be created, well by someone.

6. Floating icons, this is my main design issue - right now we have icons just floating around. Maybe by redesigning them, maby adding rounded corners we could fix this.

webchick’s picture

This patch is interesting. Not looked at the code yet, just played with it as a user and read through the issue comments.

For those who haven't tried it, what this patch does is provide an "edit mode" which will pop-up edit/configure links on various items on the page:

edit mode disabled

edit mode enabled

highlight icon

By clicking those links, you're taken to whichever admin pages make the most sense. (ex: node/X/edit, admin/structure/block/configure/Y)

Thoughts:

1. I'm wondering if "edit mode" any longer makes sense. I tried the demo of the Adobe tool at http://www.adobe.com/go/incontextediting_demo, which does inline editing. A separate edit mode definitely makes sense there, because you would not want to try and click on a node title to go somewhere and instead get prompted to edit its contents. In-line editing was originally what Mark and Leisa were going for, but that's not what this patch does.

2. What this patch *does* do, however, is basically replicate the Zen theme's "show configure/edit links on hover" approach, but slightly differently because these icons are either on or off -- they don't appear contextually when your mouse is near an editable thingy.

My problem with this approach is two-fold:

a) These icons are basically floating out in outer space atm, and it's not at all clear which spots on the page they're part of.
b) The spot on the page I need to highlight to get the outline to appear is *tiny* and hard to aim for. The block/node itself would be a much larger target area.

(side-note: for accessibility, "Configure this block" should probably be "Configure the XXXX block" and so on for nodes)

If we decide to keep edit mode, we should likely give people some indication of how these icons relate to the content items immediately upon pressing it. Something like this maybe:

outlines

The solid border would then come into play when you hover over one of these sections.

If we had the concept of "default" hover-icons like we do "default" local tasks, it's possible that maybe by clicking anywhere in the box, it would initiate the action, although we'd need to test that and see if it actually worked or was really annoying.

One thing I did like is that edit mode stayed enabled across page views. Eliminated some clicks that way.

And finally, I know that those menus are not blocks, but could we get menu module to demonstrate how other contrib modules can add their own configurable links? Charlie made it sound like there was a hook for this.

Dries’s picture

- @webchick, this patch is a first step. Once this patch lands, we can do inline editing of nodes as a follow-up patch. Inline editing of nodes would leverage parts of this patch. This patch creates the framework on that we can leverage in other parts of Drupal, including the node system.

- Because everything is a block, including the context-sensitive help text in the administration section, I _think_ I'd prefer a toggle mode. I've used the patch for a while, and it is slightly odd to have icons pop up all the time when you don't intend to make these kind of inline edits. This might be personal taste.

- I agree with webchick that when you click the toggle, you want to see something change instantly. Drawing borders around blocks sounds like it would be effective. It could be quite confusing to have a toggle that appears not to work until people start moving their mouse over block elements. Curious to learn what Mark and Leisa suggest we do.

- Personally, I think this patch is a huge plus for new users _and_ for power users.

sun’s picture

This is a JS-only feature. Therefore:

- The edit mode should not be enabled (visible) permanently.

- But the edit mode should work immediately, i.e. without a page refresh.

- To accomplish this, we use a CSS class that is applied to the overall page body.

- The hover behavior that displays those edit links checks first, whether the context has this CSS class. If it has, edit links appear. If not, nothing happens visually.

- That means, admin edit links are always output for users with the appropriate permissions. Whether they are actually displayed or not, depends on the global toggle, resp. CSS class.

- The "toggle edit mode" link thereby is a JS-only link, which has to be injected via JS.

- The global toggle should use a cookie or an AJAX request in the background to store the user's current preference.

- Lastly, I do not think that we need additional visual containers. Nor do I think that we need icons (that's something for contrib). Have a look at Views' admin links: they just work, as text links, appearing on hover.

+++ includes/common.inc	15 Aug 2009 16:56:48 -0000
@@ -3940,6 +3940,18 @@ function drupal_render(&$elements) {
+  if (isset($elements['#actions'])) {
@@ -4247,6 +4259,9 @@ function drupal_common_theme() {
+    'action_links' => array(

I don't like the term "actions". These are "admin links", and that we should name them.

+++ includes/menu.inc	15 Aug 2009 16:56:52 -0000
@@ -775,28 +775,30 @@ function menu_get_object($type = 'node',
-function menu_tree_output($tree) {
-  $output = '';
+function menu_tree_unrendered($tree) {
+  $content = '';
@@ -819,13 +821,28 @@ function menu_tree_output($tree) {
+      $content .= theme('menu_item', $link, $data['link']['has_children'], menu_tree_output($data['below']), $data['link']['in_active_trail'], $extra_class);
...
+function menu_tree_output($tree) {
+  return drupal_render(menu_tree_unrendered($tree));

This change to use drupal_render() makes little sense, because only the top-level uses a drupal_render-style array.

+++ includes/theme.inc	15 Aug 2009 16:56:52 -0000
@@ -1322,6 +1333,36 @@ function theme_links($links, $attributes
+  if (empty($_SESSION['edit_mode'])) {

We should not store this user setting in the session. Either use a cookie, or store it in user.data, or in a dedicated table.

+++ modules/block/block.tpl.php	15 Aug 2009 16:56:53 -0000
@@ -30,12 +30,26 @@
+ * Action variables:
+ * - $has_actions: TRUE when the block is editable by the current user.
+ * - $action_links: Already-themed link(s) for actions that may be taken on the
+ *   block; may be empty.
+ * - $action_links_text: An array of captions for the links to take action on
+ *   the block; may be empty.
+ * - $action_links_info: An array of information describing the links to take
+ *   action on the block; may be empty.

- $has_actions seems superfluous, not even used in here.

- Those _text and _info variables are very confusing (both the variable names and the description here). Can those be merged into one variable?

+++ modules/node/node.module	15 Aug 2009 16:56:56 -0000
@@ -1094,6 +1094,16 @@ function node_build($node, $build_mode =
+    '#actions' => array(
+      'edit' => array(
+        'href' => 'node/' . $node->nid . '/edit',
+        'title' => t('Edit this @type', array('@type' => drupal_strtolower(node_type_get_name($node)))),
+      ),
+      'delete' => array(
+        'href' => 'node/' . $node->nid . '/delete',
+        'title' => t('Delete this @type', array('@type' => drupal_strtolower(node_type_get_name($node)))),
+      ),
+    ),

We have this information in the menu system already.

Even nicely wrapped into menu router paths using the corresponding system object (%node here), identifiable by 'type' == MENU_LOCAL_TASK.

+++ modules/system/system.api.php	15 Aug 2009 16:56:59 -0000
@@ -12,6 +12,24 @@
+ * Register icons to be used for the action links on in-page items; core
+ * registers "configure", "delete", and "edit".
+ *
+ * @return
+ *   An array of icons to be used for the action links on inline items; each
+ *   icon must specify a 'path', the path to the image, and an 'alt', the text
+ *   to display if the image cannot be.
+ */
+function hook_action_links_icons() {
+  $icons['rearrange'] = array(
+    'path' => drupal_get_path('module', 'hook') . '/rearrange.png',
+    'alt'  => t('Rearrange'),
+  );
+
+  return $icons;
+}

That's a completely wrong way to render icons. Icons should be based on CSS classes in admin links, using appropriate CSS to turn text links into icons (using an icon in the background).

The approach taken here is too hard-coded and requires too many PHP skills from themers to override those icons. Contrary to that: CSS, they understand. - Which also allows to completely disable those icons by not loading the stylesheet file.

Beer-o-mania starts in 16 days! Don't drink and patch.

Damien Tournoud’s picture

I do agree with most of what sun is saying:

  • we already have the information about the actions in the menu, that's the best place to put that. Or more precisely, we will have all the information will need when #542658: Move action "tabs" out of local tasks will land.
  • the PHP should not know about icons, but only about classes.

I do disagree on one point:

We should not store this user setting in the session. Either use a cookie, or store it in user.data, or in a dedicated table.

I do believe that the session is perfectly right to do that.

sun’s picture

@DamZ: Sessions end. But I'm still the content manager who wants to edit pages. Do I have to enable the edit mode all over again? After every single login?

catch’s picture

I agree on the session - this should be remembered properly so if you like having the links all the time, you don't have to keep setting it. The toolbar is using a cookie for collapsed/not collapsed, if we use a cookie here, those preferences will last the same length of time.

Stefan Nagtegaal’s picture

Nice to see we are getting this feature back into D7. I think it was D3 or 4 when we had these edit links too, only it wasn't properly thought out, as some people did right now.. Great job!

Some comments:
- the icons look a bit ehm... odd to me; They do not fit into the elegance of "Garland";
(I would rather see the actual icon-symbol coloured than a square with the icon-symbol transparent);
- because we use outline to hover-up certain divs, it looks a bit dull sometimes. (due to margina/paddings and elements surrounding the actual hovered-up divs) Hard to explain, not sure if I am clear what I mean...

Everett Zufelt’s picture

Issue tags: +Accessibility

Wondering how much thought has been given to ensuring that this functionality is accessible to screen-reader and keyboard only users? What is the experience of a keyboard only user when this feature is toggled on?

Dries’s picture

This patch is holding up (part of) #376103: Make /admin into a REAL dashboard..

cwgordon7’s picture

First of all, thanks everyone for all the feedback, it's really been very helpful in moving this patch along. Here's everything new since stella's patch in #49:

- The patch in #49 changed the behavior of the menu module to simply add a link to the block instead of adding a link to the menu, which I completely support, it looks much nicer / cleaner now. I removed the now-extraneous menu template file.
- I changed the patch to use the dashed borders as suggested in #50 rather than disappear entirely. I very much like how it looks now.
- I agree that this is great for both new users and power users - it eliminates a lot of needless clicking for power users while eliminating a lot of needless confusion for new users.
- I disagree that this is a JS-only feature. It will not be as awesome without JS, yes, but it will still fundamentally work. I believe this makes a few of the other points in #53 void.
- The reason I used the term actions (which I admit is a bit clunky) rather than admin is because it is not necessarily reserved for admin links - node editing, for example, is not necessarily an administrative task. I'd be happy to change the property name, though, if the consensus is that admin links is a better name.
- I think the session variable is the perfect place to store it - Drupal should reset itself to a non-editing mode after a user logs out, and it should not reinstate the editing mode when a user logs back in, that would be confusing and weird. Also, that would not work if a user were to switch browsers or computers. Really, though, it comes down to: I don't think edit mode is something that should be enabled permanently. It would be good to present site admins with their site as it will appear to most users, to the extent possible; most of the time, admins will not want to be bothered with those pesky edit icons and/or the somewhat aesthetically strong borders. Sometimes they will, and their setting should be remembered, but I think the session variable is the perfect place to store that data. That is how I envision the use case for this feature. I am open to an opposing consensus, however.
- I removed the _text and _info variables from the templates, they were mostly unnecessary. I left has_actions in there because although not used in this example it could potentially be quite useful.
- I disagree that all the information we need is available in the menu system. I also changed the link titles as per the accessibility suggestion, so there's no way to do that strictly from within the menu system.
- I changed the links to use CSS to add the icons rather than PHP. This should make it easy to override by themers and should resolve some of the theming-related concerns brought up in #57(?). The icons hook is also removed as it is no longer necessary; thanks for the suggestion to use CSS rather than the img tag. (Note: implementation of icons using CSS might not be perfect, I'd welcome input from CSS gurus here).
- I'd welcome theme-specific change suggestions for the core themes - it should mostly be simple CSS changes now that I've moved the icons out of HTML and into CSS. (Rounded corners would perhaps be awesome, but also might belong, perhaps, in a followup patch).
- I believe the current patch should be accessible to screen-reader-only users but I'm hardly an expert on accessibility; I'd encourage you to try out the patch and suggest any improvements that can be made! Note that I implemented the suggestion in #50 of more specific link titles for increased accessibility. I also don't see a reason why keyboard-only users should be particularly impeded by the links on everything patch.

I think that should address all the points above, so this can absorb some more thoughts on the outstanding questions as well as some more reviews. :)

Everett Zufelt’s picture

@cwgordon7

Tested this with two screen-readers and browers (JAWS 10 / FF3.5 and VoiceOver 10.5.7 / Safari 4.01).

Functionality is working well. I could make some minor recommendations, but these can wait for code freeze.

mcrittenden’s picture

Everett Zufelt: I'm not cwgordon7 but that's good to hear :) thanks for the tests.

Patch works great for me. I'm in favor of RTBC'ing and saving minor fixes for code freeze.

Dries’s picture

Last patch fails to apply for me. Asking the test-bot to test it again, but we might need a reroll. Call 911!

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

Great work on this patch, Charlie. I think generalizing from edit links to action links was definitely the right move, and you also transformed my initial ugly-looking versions into something a lot more pretty :)

I've been studying the latest version of the patch and intend to write up a real review, although I probably won't have time until tonight or over the weekend. I think I have some new issues to raise, but it wouldn't pain me to see it committed before then - I think the patch is pretty close as is, and followups are always possible. This is absolutely a killer feature, though, so I hope we can make it as perfect as we can in the time that is remaining.

seutje’s picture

@60:
Attempt to apply patch showed that theme.inc node.module and system.module were wrong version so I tried to apply it manually and some things got me wondering

for instance, I noticed there's still a link added to the main menu, whereas your screenshot does not show this

+++ profiles/default/default.profile	19 Aug 2009 07:38:20 -0000
@@ -216,8 +216,12 @@ function default_profile_site_setup(&$in
   menu_rebuild();
 
   // Save some default links.
-  $link = array('link_path' => 'admin/structure/menu-customize/main-menu/add', 'link_title' => 'Add a main menu link', 'menu_name' => 'main-menu');
-  menu_link_save($link);
+  $links = array();
+  $links[] = array('link_path' => 'edit-mode/toggle', 'link_title' => '', 'menu_name' => 'main-menu');
+  $links[] = array('link_path' => 'admin/structure/menu-customize/main-menu/add', 'link_title' => 'Add a main menu link', 'menu_name' => 'main-menu');
+  foreach ($links as $link) {
+    menu_link_save($link);
+  }

also, my navigation menu block is shown, even though it is empty

attaching updated patch for current HEAD (hope I didn't break anything this time, sry if I did)
attaching screenshot as well

seutje’s picture

Status: Needs work » Needs review

forgot status again

sun’s picture

+++ includes/common.inc	20 Aug 2009 11:41:00 -0000
@@ -4248,6 +4260,9 @@
+    'action_links' => array(
+++ modules/block/block.module	20 Aug 2009 11:41:00 -0000
@@ -278,8 +278,20 @@
       $build[$key] += array(
         '#block' => $block,
+        '#actions' => array(
+          'configure' => array(
+            'href' => 'admin/structure/block/configure/' . $block->module . '/' . $block->delta,
+            'title' => !empty($block->subject) ? t('Configure the @block block', array('@block' => drupal_strtolower($block->subject))) : t('Configure this block'),
+          ),
+        ),
         '#weight' => ++$weight,
       );

These are still "admin_links". Using #admin_links also for the FAPI property would make this less ambiguous.

Also, it would not use reserved word we have in core already. What if we decide to add Actions for the rendering event someday?

These are not actions, these are administration links.

+++ includes/menu.inc	20 Aug 2009 11:41:00 -0000
@@ -775,28 +775,30 @@
 function menu_tree($menu_name) {
-  $menu_output = &drupal_static(__FUNCTION__, array());
+  $menu_tree = &drupal_static(__FUNCTION__, array());
 
-  if (!isset($menu_output[$menu_name])) {
+  if (!isset($menu_tree[$menu_name])) {
     $tree = menu_tree_page_data($menu_name);
-    $menu_output[$menu_name] = menu_tree_output($tree);
+    $menu_tree[$menu_name] = menu_tree_unrendered($tree);
   }
-  return $menu_output[$menu_name];
+  return $menu_tree[$menu_name];
 }
...
-function menu_tree_output($tree) {
-  $output = '';
+function menu_tree_unrendered($tree) {
+  $content = '';
   $items = array();
 
   // Pull out just the menu items we are going to render so that we
@@ -819,13 +821,28 @@
@@ -819,13 +821,28 @@
     $extra_class = implode(' ', $extra_class);
     $link = theme('menu_item_link', $data['link']);
     if ($data['below']) {
-      $output .= theme('menu_item', $link, $data['link']['has_children'], menu_tree_output($data['below']), $data['link']['in_active_trail'], $extra_class);
+      $content .= theme('menu_item', $link, $data['link']['has_children'], menu_tree_output($data['below']), $data['link']['in_active_trail'], $extra_class);
     }
     else {
-      $output .= theme('menu_item', $link, $data['link']['has_children'], '', $data['link']['in_active_trail'], $extra_class);
+      $content .= theme('menu_item', $link, $data['link']['has_children'], '', $data['link']['in_active_trail'], $extra_class);
     }
   }
-  return $output ? theme('menu_tree', $output) : '';
+  return array(
+    '#theme' => 'menu_tree',
+    '#menu' => $content,
+  );
+}
+
+/**
+ * Returns a rendered menu tree.
+ *
+ * @param $tree
+ *   A data structure representing the tree as returned from menu_tree_data.
+ * @return
+ *   The rendered HTML of that data structure.
+ */
+function menu_tree_output($tree) {
+  return drupal_render(menu_tree_unrendered($tree));
 }

This entire part should not be part of this patch. It still invokes menu_tree_output() for any levels below the starting depth of a menu, which is wrong. #283723: Make menu_tree_output() return renderable output is the real issue to tackle this change.

+++ includes/theme.inc	20 Aug 2009 11:58:13 -0000
@@ -805,7 +805,12 @@
-                $processor_function($variables, $hook_clone);
+                $result = $processor_function($variables, $hook_clone);
+                // Allow process functions to abort rendering the item by returning
+                // FALSE.
+                if ($result === FALSE) {
+                  return '';
+                }
@@ -848,16 +853,22 @@
-    $variables = array(
-      'template_files' => array()
-    );
+    $variables = array();
     if (!empty($info['arguments'])) {
+      // Populate the variables with arguments passed to the theme function.
+      // Note that the first argument may be treated specially by template
+      // preprocess functions, so it must go into the variables array before
+      // anything else does.
       $count = 0;
       foreach ($info['arguments'] as $name => $default) {
         $variables[$name] = isset($args[$count]) ? $args[$count] : $default;
         $count++;
       }
     }
+    // Add an empty array of template files as a default; preprocess functions
+    // will be able to modify this. We include it last, as per the comment
+    // above.
+    $variables += array('template_files' => array());

I'm also not sure how this change relates to this patch.

+++ includes/theme.inc	20 Aug 2009 11:58:13 -0000
@@ -1431,6 +1442,33 @@
@@ -1948,6 +1986,63 @@

Please re-roll with diff -upN

+++ includes/theme.inc	20 Aug 2009 11:58:13 -0000
@@ -1948,6 +1986,63 @@
+      foreach ($element['#actions'] as $key => $data) {
+        $variables['has_actions'] = $variables['has_actions'] || !empty($data['access']);
+      }
+    }
+    // Do not show edit links when the user is already on the page that is
+    // being linked to.
+    $show_action_links = FALSE;
+    if ($variables['has_actions'] && isset($_GET['q'])) {
+      foreach ($element['#actions'] as $key => $data) {
+        $show_action_links = $show_action_links || ($data['href'] != $_GET['q']);
+      }

The second foreach is superfluous, can be merged into the first.

+++ includes/theme.inc	20 Aug 2009 11:58:13 -0000
@@ -1948,6 +1986,63 @@
+      foreach ($element['#actions'] as $key => $data) {
...
+      foreach ($element['#actions'] as $key => $data) {

3rd + 4th time we iterate over the very same array...

+++ includes/theme.inc	20 Aug 2009 11:58:13 -0000
@@ -1948,6 +1986,63 @@
+    else {
+      $variables['action_links'] = '';
+    }
+  }
+  else {
+    $variables['action_links'] = '';
+  }

Please declare this once upfront, not multiple times for every not passing condition.

+++ modules/block/block.module	20 Aug 2009 11:41:00 -0000
@@ -833,6 +845,22 @@
+    // TODO: This is necessary to match the behavior in _block_render_blocks()

Use @todo here. Following lines should be indented with 2 spaces, to make clear which other lines of this comment belong to the @todo.

+++ modules/block/block.tpl.php	20 Aug 2009 11:41:01 -0000
@@ -30,12 +30,22 @@
+<?php if ($action_links): ?>
+  <?php print $action_links; ?>
+<?php endif; ?>

Amendment to the above: We even have another issue that wants to output "action links" (splitted from local tasks) in the templates. Name clash. Yet another reason to name them correctly.

+++ modules/node/node.module	20 Aug 2009 11:58:10 -0000
@@ -1143,6 +1143,16 @@
+    '#actions' => array(
+      'edit' => array(
+        'href' => 'node/' . $node->nid . '/edit',
+        'title' => t('Edit @type @node', array('@type' => drupal_strtolower(node_type_get_name($node)), '@node' => drupal_strtolower($node->title))),
+      ),
+      'delete' => array(
+        'href' => 'node/' . $node->nid . '/delete',
+        'title' => t('Delete @type @node', array('@type' => drupal_strtolower(node_type_get_name($node)), '@node' => drupal_strtolower($node->title))),
+      ),
+    ),

This redefinition of links that are already defined elsewhere still makes me cry.

Additionally, it seems like these links here link to node edit/delete pages, but contain strings for content types.

Furthermore, if we would re-use the menu system for those admin links, then Devel's "Load" and "Render" links would automatically appear as well.

+++ modules/node/node.tpl.php	20 Aug 2009 11:41:01 -0000
@@ -45,6 +45,15 @@
+ * - $action_links_text: An array of caption for the action links of the node;
+ *   may be empty.
+ * - $action_links_info: An array of information describing the links to take
+ *   action on the node; may be empty.

There are obsolete now, no?

11 days to code freeze. Better review yourself.

Status: Needs review » Needs work

The last submitted patch failed testing.

sign’s picture

Rerolled to work with head and renamed actions to admin_links

yoroy’s picture

Status: Needs work » Needs review

bot?

David_Rothstein’s picture

The patch no longer applied and also needed to be rerolled for the drupal_function_exists() changes. Plus a number of regressions seem to have slipped in. The attached version fixes:

  • The appearance of the empty Navigation block noted in #66.
  • Double appearance of the "Add a main menu link" in the default install profile.
  • The javascript hover behavior had totally stopped working; I'm not 100% sure my fix was the right one, but it seems to at least work again.

The theming seems to have regressed also (I'm not getting the icons aligned to the right anymore, as in the previous screenshots), but I didn't fix that.

More comments on the patch will have to wait until tomorrow.

xmacinfo’s picture

After code freeze I'll try my hand at creating better looking admin links icons.

Awesome!

David_Rothstein’s picture

Here are some comments on the current state of the patch:

  1. +function menu_page_alter(&$page) {

    I added this function originally, but it was always a hack :) The menu should not have to know that it is being displayed in a block (which might not always be true), nor should other items that display in a block (e.g., a View) have to do the same thing. I am wondering now if it would be possible to handle menus (and Views) the other way around? That is, in order to get their links to appear with the parent block's links, perhaps block module should go through its children and "steal" their links, attaching them to its own?

    It almost seems like this would even make things easier - we could simply include the links as part of the structured array that the patch already has menu_tree() return, and menu_page_alter() could disappear entirely?

  2. Always showing the dotted borders is nice in some ways, but one thing we seem to lose with that is the ability to cleanly show borders in the case of "nested" items (think of an editable View which contains a bunch of editable nodes - that would lead to lots of borders on the page).

    Also, in the one case of nested items we have in core (menus and blocks), the patch no longer seems to show different borders for each; in earlier versions of the patch, hovering over the "configure block" link put a border around the whole block, whereas hovering over the "edit menu" link only put a border around the menu items. I think this feature is very useful in helping people understand what the different links do. (It might automatically be added back by addressing my first comment above, but that's when you'd hit problems with the permanent dotted borders overlapping each other.)

    In short, I think we should consider taking the permanent dotted borders out, especially if it's possible to theme the links in a way that they are more prominent and more closely tied to the page item (e.g., inline with the title and possibly containing a single descriptive word such as "configure" or "delete" next to the icon? - I believe that's how @yhahn did it in his initial patch at #484820: Initial D7UX admin header and it looked good).

  3. I think the global edit mode should be optional, at the very least. Each person has their own taste, but overall, it seems too complicated to be the default behavior. I think the default behavior should be something simple (either show the links always or show them on hover, subject to either a permission or a sitewide setting which would hide them completely). Then, modules could override this if they wanted to -- e.g., it could be up to the toolbar module to add some kind of global edit mode behavior?

    Technical translation: I think it would be better if the $_SESSION['edit_mode'] code were not in template_preprocess(), but rather either taken out of this patch entirely or used somewhere else, and wherever the "somewhere else" is, its main function would be to trigger adding the appropriate CSS/JS to the page for hiding/showing the icons. Does that make sense?

  4. @webchick in #51 suggested that when the global edit mode is enabled, perhaps clicking anywhere in the box should trigger the edit behavior. Note that an earlier version of the patch had this feature (I think #24), although it was pretty clunky. One issue you run into is what happens when someone clicks a link? - do they trigger the edit mode for the box that the link is in, or do they actually follow the link and go to a different page? Otherwise, it's kind of neat to have, though.
  5. The current patch does not have the menu-editable.tpl.php file, but still has all the supporting code for it, so it looks like something is wrong there. As per my first point above, ideally menus should be capable of dealing with their own links without knowing that someone else might want to render them, so that suggests this functionality should stay. Maybe it can now be a theme function rather than a template file (since in D7, theme functions now have preprocessors too)?
  6. Could the code that this patch adds to drupal_render() be moved to template_preprocess()? I should be asking this of myself, since I'm the one who originally put it there, but I can't remember a good reason for it :) If the code were consolidated in one place, this might also make it easier to address the suggestions that people made above, that the patch should be using the menu item to provide values for things like the title (in addition to using it for the access checks). It seems to me like it would be good to use the menu item's values as fallbacks, although with the ability to override them like the patch has now.
  7. The code in template_preprocess() is overriding several things that could perhaps have been defined in the admin/action link itself (for example, the attributes array). Do we want more of a $info_items = $data + $defaults kind of approach here? (I realize it wouldn't be exactly that because some array elements have to be moved around and such, but something along those general lines, perhaps.)
  8. I don't want to get into bikeshedding, but I agree with Charlie that these should not be called "admin links" - there is no requirement that modules use them for admin functionality (whatever that even means). #action_links or even just #links seem fine to me (though I suppose just "links" is too generic for a CSS class).
  9. We're still using the CSS outline property for the borders. I was under the impression that that doesn't work in IE. Is that true? (If that's a problem, I have no idea what to use in its place, though.)
  10. @sun in #68 asked how the "first argument may be treated specially by template preprocess functions" code in theme() relates to this patch. See the code in template_preprocess() for the answer to this. It's a little awkward that this is needed, so suggestions for improvement are welcome.
Gábor Hojtsy’s picture

FileSize
96.79 KB

@cwgordon7: while the dashbed borders might seem like highlighting the items you can click on, this can get very busy easily on a complex site. Also, it is very much in the way of the site theme and can easily get ugly. Mark designed a bar at the top of the screen to show up when in edit mode, which would indicate the change from the normal mode and help users orient themselves. The hover state would highlight the given regions, much like in Firebug. So this edit mode would basically be a discoverable Firebug for your website but it would know about Drupal stuff :)

See http://www.flickr.com/photos/mboulton/3594826005/

Excerpt:

Note that since we are not applying the edit mode to a node but the whole page instead, we'd need to somehow morph this meta bar to have a reasonable meaning in this case.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review
mgifford’s picture

I applied this patch and am now getting this error:

Notice: Undefined index: #block in menu_page_alter() (line 317 of /Applications/MAMP/htdocs/drupal-cvs3/modules/menu/menu.module).

int’s picture

Issue tags: +Exception code freeze

add tag

Everett Zufelt’s picture

I can confirm the error @mgifford is getting in comment #78 on clean head.

Everett Zufelt’s picture

Status: Needs review » Needs work

Forgot to set to needs work in comment #80

Manuel Garcia’s picture

Results from applying the patch:

manuel@manuel-laptop:~/htdocs/drupal7$ patch -p0 < 473268_edit_links_on_everything_72.patch 
patching file includes/common.inc
Hunk #1 succeeded at 4070 (offset 119 lines).
Hunk #2 succeeded at 4507 with fuzz 2 (offset 236 lines).
patching file includes/menu.inc
patching file includes/theme.inc
Hunk #3 succeeded at 1488 (offset 16 lines).
Hunk #4 succeeded at 2027 (offset 16 lines).
patching file misc/admin_links.css
patching file misc/admin_links.js
patching file modules/block/block.module
Hunk #1 succeeded at 232 (offset -52 lines).
Hunk #2 succeeded at 778 (offset -72 lines).
Hunk #3 succeeded at 802 (offset -72 lines).
patching file modules/block/block.tpl.php
patching file modules/menu/menu.module
patching file modules/node/node.module
Hunk #1 succeeded at 1006 (offset -137 lines).
patching file modules/node/node.tpl.php
Hunk #1 succeeded at 44 (offset -1 lines).
Hunk #2 succeeded at 83 (offset -1 lines).
patching file modules/system/system.module
Hunk #1 succeeded at 529 (offset 11 lines).
Hunk #2 succeeded at 1348 (offset 39 lines).
Hunk #3 succeeded at 3010 (offset 21 lines).
patching file modules/toolbar/toolbar.install
Hunk #1 succeeded at 28 with fuzz 1.
patching file profiles/default/default.install
Hunk #1 succeeded at 200 (offset -2 lines).
patching file themes/garland/block.tpl.php
patching file themes/garland/node.tpl.php
patching file themes/garland/style.css
Hunk #2 succeeded at 290 with fuzz 1 (offset -4 lines).
Hunk #3 succeeded at 654 (offset -4 lines).
Hunk #4 succeeded at 813 (offset -4 lines).

I confirm the error mentioned above,
Notice: Undefined index: #block in menu_page_alter() (line 317 of /home/manuel/htdocs/drupal7/modules/menu/menu.module).

I see no edit icons, nor the Enable/Disable edit mode menu item.

my-family’s picture

First, the idea is great, thanks for it!
My remarks and suggestions:
1.„edit“ is confusing, „admin links“ would be better, it is more general (even for additional functionality in future, maybe... = better consistency)
2.”themed” icons (blue for garland, etc.) are too outstanding, they break design; the other problem is, that it is quite demanding to theme icons for different themes individually. Instead, I would suggest: slightly gray icons >> black on hover (or, alternatively, white on hover for very dark themes, or white icons with dark background – box – for all themes).
3.It seems to me that permanently visible icons are redundant. If I see icons almost EVERYWHERE in the edit mode, it scatters my attention very strongly. And, frankly, I feel it as a kind of “joomlism” (sorry … :-)), without any good reason... I would prefer very similar solution, as is now (D6) in the already mentioned Zen theme, or which is used in the views module. I feel the “Zen solution” very user friendly, except for one thing: the words (edit etc.) are too long and sometimes interfere with menu or other links. In the case of D7 core, I would suggest:
a) edit mode (“admin links” on) as a default state for admin, but not visible
b) on the hover on any part of certain block, dashed border and slightly gray small admin links (icons) in the corner of this block
c) more outstanding (e.g., white with dark background) icons on hover of the icons only.

my-family (with co-operation with mysak)

Manuel Garcia’s picture

OK, to apply the patch properly, you have to do it before the installation (not sure if it's mentioned earlier).

@my-family: The colors of the icons are dependent on the theme, like this:

ul.admin-links li a span {
  background-color: #whatevercoloryouwant;
}

So you can change it to anything you like in your theme, as it's just a transparent png. I think this is a good thing, as it doesn't require your theme to actually provide their own icons just to change the color.

About having the icons appear zen-style, (on hover), to me that is a bit annoying... personally i prefer this, that I just enable when I'd want it, and instantly see what I can edit/configure just by looking at it and not have to hover over items, to see if I can edit it or not.

sign’s picture

FileSize
27.2 KB

re-rolled patch,
fixed the error above (Notice: Undefined index: #block in menu_page_alter() (line 317 of modules/menu/menu.module).)

Everett Zufelt’s picture

Applied the patch in #85 before installing head.

Everything seems to work well.

My only concern is that on the default frontpage of the install, with no content and edit links enabled, Near the bottom of the page in DOM order I get the link "Configure this block". Which block is "this block"? Can we possibly give this link more context?

sign’s picture

Status: Needs work » Needs review
FileSize
28.08 KB
21.19 KB
4.55 KB
27.36 KB

Adjusted icons (floated icons) and border highlighting

@Everett Zufelt the issue is that we are not getting a block description in block object. Only title is exposed which is not always there, like in case of "Powered by drupal" button. The default behaviour is if there is no title "Configure this block" is shown

xmacinfo’s picture

@my-family I agree for the icons. The ones included here are temporary. Once this patch is landed, we will have plenty of time to create better looking generic icons, since enhancing the icons won't affect the API or the strings for translation.

I will definitely try to create new icons for this in the next few weeks.

Everett Zufelt’s picture

@Sign

It sounds reasonable to me to use "Configure this block" if there is no title for the block. Makes me wonder if we should open a new issue to ensure that all core blocks have appropriate titles assigned?

Gábor Hojtsy’s picture

FileSize
25.18 KB

Here is a quick reroll (without deep review) since most hunks were only applying with fuzz and the block.tpl.php hunk did not even apply. Icons are still the same as in #38. Some notes:

- The edit toggle does not work with the overlay patch applied at the same time due to how the overlay assumes all toolbar shortcuts to be targeted at it. As discussed before, the toggle should probably not be a regular shortcut, or we need an opt-out class for the overlay to not jump on this item.
- I do not see hover borders for the areas being edited in edit mode, and see all icons for all items at the same time in Safari. This is contrary to the stated intention to have a firebug type discovery mechanism here, where icons and hover borders appear as you go through the page. (See also notes in #75).
- Again, at least on Safari, the edit icons goes above the block contents, eg. it displays above my help text in Seven, obscuring some of the text. Not good.

mgifford’s picture

I applied this. No problems with the error I identified in #78. However when I hover over a block I don't see the edit options that are defined here #87.

Do like this concept though.

Cliff’s picture

Subscribe

seutje’s picture

FileSize
24.99 KB

@Gábor: You forgot admin_links.css and admin_links.js

didn't review yet

seutje’s picture

FileSize
59.14 KB
1.1 KB

re-adding images because I spent a while scrolling back up

is there any way we can avoid the clipping caused by using outline on hover (see screenshot)

heather’s picture

Issue tags: +Needs usability review

To anyone else testing this patch FYI: put images in the 'misc' folder directly.

The encoding of punctuation is coming out funny in Firefox.

I made a video for others who would like to see this patch in action at the stage it is at.

http://vimeo.com/6558200 (will be done processing in 30 mins from time of posting this)

What it does: At this point in time, the patch adds edit links to blocks on a fresh install. I don't know what it does when you have views/cck installed. Those has simple edit links before anyway.

I was surprised myself when testing this. I actually know how the block & menu systems work, but I was surprised by wanting to have a 'trash' button on a block. Or some way to 'disable' a block from the menu options.

It doesn't make sense why some items can be configured and some not. (well it does when you know that somethings are menus, and some are default blocks, but still is confusing). When you get to block configuration of course, it's like a mad house in there with the many collapsible options. Could they be chunked?

The block configuration page has nothing directly to do with this patch, but it affects this patch. Specifically because the user may want a shortcut to controlling visibility.

The abstract titles of the collapsed sections are better in their descriptions. It's active language, e.g., do this, do that.

'Page specific visibility settings' versus 'Show block on specific pages'

'Role specific visibility settings' versus 'Show block for specific roles'

Perhaps the various visibility options could be chunked? Then a link from edit menu could take you there, anchored to the position in the page?

Nick Lewis’s picture

I wouldn't suggest turning all icons into sprites, but i'd really like to see us use sprites for our *new* icons. Better yet -- an icon framework that will encourage average joe module developer to use stand drupal UIs instead of inventing their own. Also a single sprite file is much easier to edit if a theme has a clashing color scheme from the default drupal ui. (the current icon set could be recolored in 30 seconds if it was in one file).

We already have a sprite framework in core (look in the jquery ui images folder in misc). And its quite easy to use. Not insisting we use it, but it has a pretty good pattern for how the css should work, so that a themer can go theme('icon', 'image'). if your unaware of it, check it out in action at http://jqueryui.com/themeroller/ -- with firebug of course.

Example of sprites: http://www.alistapart.com/d/sprites/ala-buttons1.html
Good article on their usage: http://css-tricks.com/css-sprites/

Not going to throw a fit over this, just a friendly suggestion.

Gábor Hojtsy’s picture

FileSize
26.02 KB

Recent changes in core's menu.inc totally broke this patch. Here is a partial attempt to restore it to a working state. The menu.inc rendering / array building hunks of this patch do not apply anymore, since a different patch went in to do a similar rework. So I also needed to modify the block altering code to look for the newly expected structure. This restores the configuration link for menus on the block.

For some reason the menu part of the block is still rendered as a sole "<", something which I've tried hard but did not find the reasons. Other then that, this seems to work again.

Also, I've **intentionally** removed the edit toggle link from the toolbar. As discussed multiple times, that is not actually a page shortcut but a mode switched / toggle, so does not belong there. The shortcuts patch at #511286: D7UX: Implement customizable shortcuts would definitely not have a switch for stuff like "do not open an overlay for this shortcut", because it would be a highly special exception between the edit mode, the overlay and the shortcuts. Easiest to resolve this is to just return to where we were before: that this is not something we should put up as a normal toolbar shortcut. It is not. (I've kept the main navigation link which is show in Garland and can be used to switch the mode).

Status: Needs review » Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
26 KB

Gosh, realized the error in the theming function that cause the menus to broke and consequently the test failures. I must have been pretty tired and/or disoriented to not find that out when I rolled the previous patch. Fixed one attached. At least works for me, let's see what does the testbot has to say.

Dries’s picture

Looking at the code, this looks like it is ready to be committed to me. This is one of the singlest biggest UX improvements in D7.

Unfortunately, we still need to figure out how/where people are going to toggle between both modes. Right now it is a menu item, but that is only a temporary solution.

I'm torn. I'd like to get this patch in because it provides an important pattern/mechanism for other modules to leverage -- making it available will accelerate our experience with it.

At the same time, we need to figure out where the toggle will live -- probably it ought to be integrated in the toolbar design somehow.

Thoughts?

All things considered, I think our best strategy is to get this in, and to follow-up on the UX design task.

markboulton’s picture

I just spoke to Gabor about this. Here are my thoughts and recommendations for placement of the edit icon:

1. I don't think there is any better placement for the edit toggle than in the shortcut bar. If you look at alternative placements for it: a) the top nav (Nope - that's for top level IA items only). or b) in-page (nope - It gets lost).

2. The shortcut bar is probably the best place for it - it is actually a shortcut to an action. It's just not firing up an overlay. I'm happy for you not to special case it IF there is a better place to put it. Currently, after *much* thought, I don't think there is.

Edit in place, it could be argued, is a shortcut. But, like Dashboard, it's one that should be fairly default for most content creators.

I recommend that D7 ships with a set of default shortcuts in the bar, edit in place being one of them. This default set can be added to by the user AND disabled from the shortcut bar but NOT deleted. Only user-created shortcuts can be deleted. 'System-shipped' actions can only be disabled as, with edit in place specifically, it's not associated with a path but a whole load of other behaviour stuff so edit in place can be actioned. We don't want the user to lose this information, so we let them disable it if they want, but not delete it.

Gábor Hojtsy’s picture

All right, made the following changes based on the feedback:

- readded the Edit mode toggle to the shortcuts (and removed from the normal navigation menu)
- switched the menu links added to be saved under the toolbar module, not menu module and added extensive comments on the reason (so that they cannot be deleted but disabled only)
- added special case to toolbar, so when it sees a link with 'edit-mode/toggle' as the path, it will not add a to-overlay class (I still maintain that this is a hack, but don't know of a better way to retrieve this information from the menu tree)
- also documented why is that menu link lacking a title (which causes a bugos looking edit screen for this shortcut though as I've just realized)

Rerolled patch attached. This is how it looks ATM on the customization page due to the now protected items:

David_Rothstein’s picture

1. I agree that this is close, but some cleanup of the patch is still needed as per other comments above, and I will try my best to carve out some time for that tonight.

2. I also agree that the "edit mode" toggle looks best in the shortcut bar, but that doesn't completely solve the issue: This feature needs to be available to people who aren't using the Toolbar as well. So we either need an alternate location for the link or a "non-edit-mode" way in which the edit-everywhere functionality is still available (probably we need both). These are some big UI issues to solve, but I think they can reasonably be done as a followup rather than in the initial patch.

yoroy’s picture

David_Rothstein’s picture

Status: Needs review » Needs work

Well, I thought I'd spend a couple hours doing some final cleanup on this patch, but it turned out not to be that simple.

In studying how to clean it up, it seems to me that there are some fundamental things that need to be reorganized in the underlying API before this can be ready to commit. Currently, the patch tries to invent too many of its own methods for rendering, which results in lots of extra code and less flexibility. The root of this problem is probably the fact that when I wrote the original patch, I was just learning how to use some of the new Drupal 7 rendering APIs myself and really only got it about half right; then in the intervening months, Drupal 7 rendering got even better and left this patch even further behind...

After some studying, here are the changes I suggest we make - and since I haven't actually tried writing this code yet, I reserve the right to be totally wrong :)

  1. A slight change to the module-facing code, so that it looks more like this:
    ...
    'links' => array(
      '#type' => 'attached_links',
      'edit' => array(
        'href' => 'node/' . $node->nid . '/edit',
        'title' => t(...),
      ),
      'delete' => array(
        'href' => 'node/' . $node->nid . '/delete',
        'title' => t(...),
      ),
    ),
    ...
    
  2. Next, take almost all the code that the patch currently adds to drupal_render() and template_preprocess() and move it into a standard #pre_render function instead. This should also simplify that code quite a bit.
  3. The combination of the above allows us to delay the rendering of these links until as late as possible. This gives themers more flexiblity and consistency; they can use something like print render($attached_links) which is consistent with the way node links and other objects in Drupal are now rendered, and (similar to node links) there would also be the possibility of rendering links in different groups (e.g., menu and block configuration) in different places in the template, which I'm pretty sure is not possible with the current patch.
  4. We can also have block.module grab its children's links (as I suggested in a comment above) rather than the other way around, thereby getting rid of the (very ugly) menu_page_alter() function from this patch and also saving contrib modules such as Views from having to implement such a function themselves in order to have their block configuration links show up in the right place. This is a key improvement, IMO, and the main reason the patch is not currently committable.

Note: In the above I'm using the terminology "attached links" and suggest we use that in the final patch. (We can't use "action links" because it's already used elsewhere in core, and as mentioned previously, we really should not be calling these "admin links" as that assumes a certain use case that the feature is not intended to be limited to.) I think "attached links" is good since it describes what they are; they are links that are attached to a particular element on the page.

If there are no holes in this reasoning, I suggest we make these changes quickly and then try to get it committed right after. It should hopefully lead to a more powerful feature with much simpler code.

Gábor Hojtsy’s picture

I've started off on the path explained by David, but what I arrived at might be more broken then expected. I believe that someone with more hands-on with all the rendering system details will be able to carry this forward, so that is why I'm adding it here despite its many errors.

1. I've renamed admin links to attached links. Not entirely sure that this would not entail confusions with #attached and friends, but the name attached links does reflect that it might be non-admin links even.

2. I've added a #type => attached_links which is themed with theme_links() via its element definition.

3. I've moved the drupal_render() and the template_preprocess() code to a pre_render on type attached_links. Now here comes one of my issues. There is no way to bubble up classes to the parent element, so I've left those classes commented out for now and added a @todo. Otherwise there were some variables added with the previous patch but not used, so I'm not sorry for loosing them.

4. Then I'm not sure how could we alter items in the links array, like menu_page_alter() does, so I've left that alone (modified to use the new element naming). However, now that we free up naming of the element to be whatever the coder wishes, we cannot reliably look for 'link' as an element key, so that would either evolve to a full-on "traverse all children recursively looking for #type attached_links" or I'm missing something here again.

5. Finally, I'm not sure how is the actual template variable envisioned to be created. Again, without grabbing this stuff in template_preprocess(), we only get the attached links in the element array, which in practice was not renderable as far as my experiments found. And once again, that is dependent on the children being named "links", so we keep two things mandatory, the type and the name, which is unheard of elsewhere as far as I know.

So all in all, I'm posting this half-morphed patch in the interest that it might help. I took already too much time on debugging it, so would welcome if someone took over.

int’s picture

Status: Needs work » Needs review

go bot

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

I have been working a bit on updating this patch, but with the holiday tonight and tomorrow, I won't be able to post anything until at least tomorrow night (if not later).

So in short, if someone else wants to try their hand at making this work, please do :)

It looks to me like it will all be possible, but might require a couple foreach(element_children() as ...) stanzas in places where we'd ideally like to not have them, so I guess we'll see what happens.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
24.35 KB

OK, here is an updated patch which now works fine again and is overall much, much cleaner. The #pre_render solution is now working correctly, and so is the "block-module-grabs-its-children's-links-automatically" feature, and I also cleaned up or fixed a lot of other things that were wrong.

To solve the problems raised by Gábor, I went with the "double-required-name" solution -- i.e., the name of the element and its #type must both be (#)attached_links. At first I thought that was a bad idea and redundant (just like he said), but then I realized that it's quite similar to the way node links already work, so if it's good enough for node links, it's good enough for these too, as well as promoting some kind of consistency throughout Drupal. Also, although you'd almost always want to use them in tandem, there are actually some situations where you might not (one controls what items go into the $attached_links variable that gets passed to templates, and the other controls how the items are themed when they are rendered), so it actually makes some sense to keep them separate.

In addition, a fair amount of scattered code that had been added in previous patches but was no longer necessary has been removed from this version. I also left out the overlay hack for the edit mode toggle, since the overlay isn't even in D7 -- and in the long run, I'm not sure if we'd want a hack for that vs a different solution, so it's best to leave it out for now.

In terms of the code, there are a couple things still to do (chief among them is to figure out the correct separation between the pre-render and theme functions - currently essentially everything is in pre-render and I'm not sure that's good?). But I think the code is now in good shape and hopefully getting very close to being commit-worthy.

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

FileSize
66.09 KB

Also, as mentioned previously, though this may be close to a commit, we will need to do some serious UI work afterwards. In particular, I think we need to de-emphasize the "edit mode" and make it more of a helper mode as opposed to having it be the only way of accessing these attached links. When edit mode is off, the attached links should appear on hover (as many people have said above), similar to the way Views or Zen does it now, whereas when edit mode is on, it should behave similar to the way the patch behaves now.

The reason I bring this up again now is that occurred to me there is one small way in which that might affect the API. I dug up @yhahn's original work from #484820: Initial D7UX admin header for theming these kinds of links (he had it for nodes only) and made a screenshot from it (attached). I think having the words "Edit" and "Delete" actually showing on the page is an improvement that we will likely want, and if so, it could be a small API change since modules would have to define what word goes there somehow (unless we try to restrict them to the three provided by core).

I think something like the attached screenshot (positioned closer to the node title, and likely without the border) is likely what we want for the standard hover behavior - we need something that is easy to use and consistent with what, e.g., Views does now. The only issue I can think of is that it might start looking a little crowded when you try to make this apply to a block in a narrow sidebar region.

Gábor Hojtsy’s picture

Looks like testbot thinks this will not pass, so I've applied and tested the patch locally:

- did not manage to reproduce the CSS failures
- did manage to reproduce the locale failures, which were due to how locale expects to find the DOM structure in the block, which is changed with attached links

Also fixed some issues with Safari support and made the CSS leaner:

- added wekbit border radius to the CSS, so it rounds on Safari too
- shortened color and the radius specification where applicable
- fixed order of items in the outline property up to standards
- fixed "class" variable in JS (which is a reserved word) to className, so the outlining works in Safari too (and whetever browser which enforces reserved words to not be reused as variables

Now it passes tests on my machine and works the same in Firefox 3.5 and Safari 4.

I've also did a static review of the code:

- this approach to attached links seems more decoupled and cleaner then it was before, indeed
- good to see support for separate rendering of the block and block content links - would it make sense to add provisions for similar functionality in nodes? so people can edit their attached image(s) and such via these links if exposed by the modules they use?
- I'm seeing menu module and system module both add menu edit attached links; not sure why is this the case, and I'd agree with the only @todo you have in here that it is not at all nice in system.module
- understanding your reasoning, but not entirely happy that you removed the compatibility fix/hack for overlay module, since it does not let people test the two patches together; which would be important in this late stage to figure out any incompatibilities

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
25.55 KB

Here is the actual patch, see my comments above.

sun’s picture

+++ includes/theme.inc	6 Oct 2009 10:50:06 -0000
@@ -2071,6 +2079,29 @@ function template_preprocess(&$variables
+  if (is_array($element) && !empty($element['#attached_links'])) {

We now have a generic pattern of $element['#attached'][CALLBACK], which we should use here (and thereby free up template_preprocess() from implementing feature-specific stuff).

+++ misc/attached_links.css	6 Oct 2009 10:50:06 -0000
@@ -0,0 +1,62 @@
+/*
+** Highlighted regions for attached_links.js.
+*/

This should adhere to CSSDoc standard -- basically following the same rules as PHPDoc.

+++ misc/attached_links.css	6 Oct 2009 10:50:06 -0000
@@ -0,0 +1,62 @@
+.attached-links-link-icon-configure {
+  background-image: url(configure.png);
+}
+
+.attached-links-link-icon-delete {
+  background-image: url(delete.png);
+}
+
+.attached-links-link-icon-edit {
+  background-image: url(edit.png);
+}

Icons we need to drop. Period.

Here is why: No one took the third dimension into account; all contributed modules can add edit links, but that will mean you'll see 3 icons next to a variable amount of text links.

I already commented above that we basically want to implement Views' plain-text-hover admin links.

+++ modules/system/system.module	6 Oct 2009 10:50:07 -0000
@@ -503,6 +516,14 @@ function system_menu() {
+  $items['edit-mode/toggle'] = array(
...
+    'page callback' => 'edit_mode_toggle_page',
+    'access callback' => TRUE,

I thought this would have been removed? Anyway, please drop the _page suffix, because this does not render a page.

Uhm, and how is it possible that anyone can access this? We have a nice "access administration pages" permission, for example.

Sorry, that's all for now, as I'm too tired to grasp the rest.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dries’s picture

Talked about this patch with several people and here is what we agreed to do:

1) We'll move the toggle icon from the shortcut bar to the header. It's just weird to have it in the shortcut bar.

2) The icons (e.g. the cog) needs some work but we can postpone these to a follow-up issue. We should keep the icons for now.

3) We'll change the behavior of the hovering to be like Views; e.g. you get to see the icons (e.g. the cog) when you hover over the block. In a follow-up issue we should discuss how we'll add an extra mode so we can support the following behaviors: (i) disabled, (ii) enabled but only show icons when hovering over and (iii) enabled and permanently show the icons. For now, let's go with (i) and (ii).

webchick’s picture

Darn. I tried to take this for a spin but it no longer applies. :(

JacobSingh’s picture

It is pre-theme functions losing their signatures. I imagine that is why.

sun’s picture

Status: Needs work » Needs review
FileSize
24.37 KB

(Almost straight) re-roll. I'll see whether I can take a stab at this. Some stuff in the implementation looks wrong (or rather to complicated) to me.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
24.44 KB
David_Rothstein’s picture

Status: Needs review » Needs work

@sun, I was hoping to take a look at rerolling this later in the weekend to try to fix some of the remaining issues, but if you get a chance to look at it also, that would be great!

1. As per the comment from Dries in #117, I've created #601150: Improved UI for contextual links as a pre-emptive followup for this issue. There may have been some confusion, but for now I'd suggest we keep the current user interface of the patch - i.e., (i) and (iii) in Dries's comment - since it's been like that for a while and in general people seem to like the "blue outlines" in some form. Probably doesn't make sense to do a last minute switch of the UI only to potentially add it back in a followup issue, so unless there's a good reason not to, I suggest we make #601150: Improved UI for contextual links about adding the Views-style interface to what we have here (which is, of course, a good idea).

2. @sun, I'm not sure what's so bad about the icons? We're providing icons in core for three very common and useful types of actions (and could easily add another one or two if desired). I don't think we want to encourage people to load up page regions with tons of different kinds of links anyway, so I think it's good to encourage people to use a limited set. However, if a contrib module wants to add their own for some kind of crazy new kind of action, nothing stops them from doing that, right?

3. Especially as possible preparation for #601150: Improved UI for contextual links, I do think we want to solidify the API here and provide the ability for a text link (which even if core doesn't wind up using, different themes may want to use in their own implementation). So I'm thinking instead of the single 'title' attribute we return with the links at the moment, we should try to "future-proof" the API by changing that to separate 'title' and 'description' attributes - the 'description' would be what currently appears on hover, and the 'title' would not be used at all at the moment, but would be intended to be used as a text link.

4. @sun: Interesting point about $element['#attached'][CALLBACK] -- I did not realize that accepted arbitrary callback functions now. Off the top of my head I'm still not sure how that would be used here though... the issue is that we need to somehow pass information "up" to the parent item of the attached links, which is used to construct variables for that items template... but if it can be used, that would be great!

David_Rothstein’s picture

Status: Needs work » Needs review

Oops, crosspost... looks like you already started :)

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
24.43 KB

Pass again.

sun’s picture

Assigned: Unassigned » sun

Short overview of what we will do now:

- Use hover-state text links. To get the actual implementation right.

- Drop the edit-mode toggle. Was bound to Toolbar module installation (WTF?) and can go in any time after 10/15, because it's a UI change only. A permission-based trigger is all we need right now.

- Remove icons. Derail this particular issue elsewhere, please. (I think someone linked to some dedicated issue in one of the D7UX issues)

- Leverage the menu system. The current patch did not cater for user permissions at all (WTF?).

I'm on it.

To get this done, we need to get #599706: Allow to alter local tasks/actions in ASAP, so please also help over there.

sun’s picture

FileSize
25.85 KB

Now I hit the state where #599706: Allow to alter local tasks/actions is absolutely required to get this done. Attaching work in progress - will come back in a few minutes. ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
39.47 KB

Don't be scared by this sucker. Also contains the patch from #599706: Allow to alter local tasks/actions, so we can move forward here.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.76 KB

Back to business.

sun’s picture

FileSize
25.33 KB

In progress, need a quick break.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dries’s picture

Some quick feedback after having glanced at the patch: "Attached links" is a funny and fairly meaningless name. Can't we rename this to "Operations"? That would make it more explanatory and make for cleaner CSS tags too.

Dries’s picture

1) In #127 @sun wrote: "A permission-based trigger is all we need right now." -- not sure what is meant with a permission-based trigger, but I'd suggest we make it a user setting. Like that, people can enable/disable the behavior as they wish. Plus, in the future, we could re-introduce an _additional_ toggle in the toolbar if desired.

2) We should consider migrating the comment administration links on individual comments to this mechanism. Right now, users with the "administer comments" permission have an 'edit' and 'delete' link on each individual comment. That could be a nice additional "clean-up", and would validate the framework some more. Could be done in a follow-up patch, of course.

3) The function name menu_context_links() wasn't self-explanatory so I had to read the code to make sense of it. I'm not sure we want to show all sub-tabs for nodes because some modules add a lot of cruft to it, that is well beyond the notion of "edit in place". Just look at all the extra stuff that is added to group nodes on groups.drupal.org. Even for core, I don't want a link to the "Revisions"-tab, for example. It was not part of the mockups designed by Mark and Leisa either. In other words, I think menu_context_links() is a step in the wrong direction, at least for nodes.

4) Related to (3), Mollom should probably add a 'report as spam' link. Currently, that link is shown as part of the regular node links along with 'Add new comment'. Clearly, 'report as spam' shouldn't be a tab. I haven't tried yet, but I assume that Mollom will be able to alter in those link for comments and nodes.

David_Rothstein’s picture

The current patch did not cater for user permissions at all (WTF?).

Sure it did.

Leverage the menu system.

The previous patch already leveraged the menu system. But we can't blindly assume that the page titles used in the menu system are the same ones we want to display with these links - as Charlie Gordon already discussed above, this is usually not true. Also (as Dries appears to have written right before me), we shouldn't assume that just because something is a tab it should also be showing up as an attached link - that would likely lead to way too many of them, and quite a number of them won't make sense.

Drop the edit-mode toggle. Was bound to Toolbar module installation (WTF?) and can go in any time after 10/15, because it's a UI change only.

Perhaps it should be removed, but I'm not entirely sure we should do all these large, last-minute UI changes to a patch when the UI has been relatively fixed for months and we already have a follow-up issue for more UI work...

Regarding the toolbar dependency, @yoroy already created #585776: Make 'edit mode' toggle available even when toolbar module is disabled above, and assuming we do what Dries said in #117 (moving the toggle from the shortcut bar to the header), the link would probably just become part of the Management menu, thereby solving that problem right away.

Remove icons. Derail this particular issue elsewhere, please.

Again, while I don't care much either way (and would certainly like to see a text links option as well!), the icons have been part of this issue for months, and we already have a separate follow-up issue to try to perfect the UI, so it's hard to see how leaving them in would count as derailing the issue...

David_Rothstein’s picture

@Dries:

Some quick feedback after having glanced at the patch: "Attached links" is a funny and fairly meaningless name. Can't we rename this to "Operations"? That would make it more explanatory and make for cleaner CSS tags too.

Makes sense to me.

In #127 @sun wrote: "A permission-based trigger is all we need right now." -- not sure what is meant with a permission-based trigger, but I'd suggest we make it a user setting. Like that, people can enable/disable the behavior as they wish.

I think the intention of a permission is that you might want to hide this feature from certain classes of users on your site (e.g., the proverbial "content creators") because it might feel too "administrative-y" for them. However, it's certainly true that it isn't really a permission in the traditional sense, and in some ways a user setting would be more flexible.

The problem with a user setting might be that for many users it won't mean much. They can toggle it all they want, but if they don't have permission to do any of the things that have links associated with them, they won't see any difference when navigating around the site. (The same is technically true for a toggle in the toolbar, but at least there, it's pretty unlikely that someone who has access to the toolbar would not be allowed to do anything else.)

So it almost sounds like we'd be talking about a permission combined with a user setting, but now that's getting pretty complicated....

4) Related to (3), Mollom should probably add a 'report as spam' link. Currently, that link is shown as part of the regular node links along with 'Add new comment'. Clearly, 'report as spam' shouldn't be a tab. I haven't tried yet, but I assume that Mollom will be able to alter in those link for comments and nodes.

I thought the answer was an obvious "yes" given the number of hooks the node module provides, but looking more closely, it seems like for nodes these links are somehow managing to appear in a place that avoids all the hooks (all of this is technically alterable via hook_page_alter, but that doesn't really work in the generic case).

Perhaps the node module needs another hook in which runs at the end of node_build()? Alternatively, we could move these links to node_build_content() where they would then be alterable; however, they are not really part of the node "content" and we don't, for example, want them winding up in search results.

This actually points to a similar problem discussed above - the reason for the difficulty over combining menu.module's links with system.module's menus is that there is no hook_block_view_alter() that menu.module can use to easily insert them.

I think in general it makes sense for each type of "object" in Drupal to ensure that the content it provides is fully alterable - however, based on the above examples, that doesn't seem to be happening here :) So should we be looking at some generic way to guarantee that all attached links are alterable? (This is something that @sun's approach provides, although I don't see how we can get around the other issues discussed above.) A dedicated hook would work, but might be overkill.

sun’s picture

What we need to understand:

  1. We can implement this as a feature that puts lipstick on the pig. Fulfilling the desire for edit links on some boxes, so let's just add them. Ignoring all sub-systems and patterns we already have and just squeeze in that thing. We can do this now and live with the added cruft for a few years.
  2. We can implement this by re-thinking the sub-systems and patterns we already have. Re-applying and extending a well-known concept that already served Drupal sites in the past. Implementing it cleanly to make even more sense of the sub-systems and patterns that are already there. We can do this now and heavily advance on it in the next years.

If you want 1), then count me out, and go ahead with any implementation. In that case, however, I need to amend that it needs to work entirely optional, because you will utterly break the performance of my sites.

Otherwise, let me explain the concept:

  • We already have these links since years.
  • We just didn't display them everywhere.
  • All modules and all developers already know the concept.
  • The links are already registered and stored in a canonical and high-performant way.
  • They provide in-context tasks for whatever entity and object you provide.
  • They work with everything, literally everything, and again, they already exist.
  • We call them local tasks.
  • We display those when displaying a page to provide tasks for the context at hand.
  • And what we want is dead simple: Removing the limitation of displaying them on a page only.
  • It is a paradigm shift.
  • And you need to re-think how you think about local tasks.
  • But after you did, it totally makes sense, and there is nothing that would make more sense.

We can certainly deal easily with the question which local tasks are displayed contextually and which should not. It's just a matter of will. And we can also display contextual links for comments - no deal at all, because all of them are already there. Heck! This concept can even be applied to much more, but requires a fair amount of imagination.

Hence, the first and foremost point of my initial summary: Get the implementation right.

Think about it.

(At minimum) 50% of this patch is still the same code as before, because I was distracted too long with the hook_menu_local_tasks_alter() patch. I will now continue to work on this patch, although I have far more critical issues on my plate, so I hope that I won't waste my time here.

Dries’s picture

@sun, I'd love to better understand what your plan is for (2) but it is not clear where you are going with it, or what exactly you are proposing to do.

Either way, per #136, I believe leveraging the menu system's local tasks has its own problems:

  1. You do _not_ necessarily want all local tasks to show up as "edit in place" links (e.g. 'node revisions', 'signup' on event nodes).
  2. You do want to allow for "edit in place links" that are _not_ local tasks (e.g. 'report as spam', 'delete', etc).

Given the above, I don't fully understand why we want to create any tight "coupling" between "edit in place" links and "local tasks" in the menu system. We can choose to couple them, but then we need to have flexibility to add and remove local tasks. After all, there is no guaranteed coupling or relation here -- and there shouldn't be one.

So, the only thing we seem to gain from using the menu system is a permission/access check?

Dries’s picture

The problem with local tasks is that they are not local tasks. Increasingly more, these so called local tasks provide content listings (e.g. moderation queue, node revisions, people that signed-up, statistics on node pages, etc). Those listings are not "edit in place"-style operations; they are just listings of all kinds.

So if we want to get this right, the menu system needs to better understand the difference between real tasks and listings (but not be confused with local actions, which is yet something else).

Then, when we have actual tasks or operations, some real tasks are not registered to the menu system as such. For example, the 'Delete' functionality is often exposed as a button rather than a local task. That is also a problem, because you might want a 'delete' icon or link as part of the "edit in place" functionality. Ditto for Mollom's 'report as spam' feature.

So, the menu system should have something like:

  1. CREATE_TASK: create tasks are displayed below the tabs and proceeded with a + sign. Currently called local action.
  2. UPDATE_TASK: update tasks are edit, delete or manage operations that could also be "edit in place" links. According to Drupal's UX team, they should not be displayed as tabs.
  3. LISTING: listing should be visualized as tabs.

Depending on how UPDATE_TASKs are displayed in the UI, it might make sense to make 'delete' and 'report as spam' UPDATE_TASKs.

But, it is still unclear how it would work for comments (i.e. where we have many delete links on a single page). Here, relying on the menu system's local tasks breaks down, IMO.

Anyway, this is a bit of a ramble, but I think it is clear why it feels like we might not want to "force" this into the menu system. Either way, the difference between local task and local action is a major WTF. Even if we choose not to do the above, we should rename local task to local tab. In fact, why local? Is there a global tab too? It could just be task versus tab.

Dries’s picture

By the way, having to re-implement a couple of links feels minor. There are only a handful of elements that would have 'edit in place' links (e.g. comments, nodes, blocks), and each of these elements would have a couple of links? That doesn't make for a lot of code. Yes, there are some other issues with that, like the ability and need to alter those links separately.

It's a difficult problem (if you try to merge it into the menu system), and it is not 100% clear what they right direction is. It might be the case that 'edit in place' is a new but important subsystem with each own APIs and rules.

Dries’s picture

As a hack, menu_context_links() could only return links that have 'edit', 'configure' or 'manage' in them? :P

Reality is that it would be _great_ to have CRUD-semantics in the menu system; it would be really valuable to understand if a link is a create-link, a read-link (e.g. listing), an update-link or a delete-link. We could then pull the update- and delete-links into an "edit in place" widget.

This might be a relatively easy change to make, especially if we fall-back onto the current visualization of those links; e.g. mixing them all (but create-links) in tabs. There could be an interim solution that gets the job done.

Even so, we need to figure out what to do with comments and blocks.

Mmm, am I getting off-topic? Sorry!

David_Rothstein’s picture

@sun: Could you explain why you think the performance aspects of your approach are better? If anything, I would think they are worse - you'd be adding more stuff into the menu system, and then forcing all these menu items to be loaded on the page, even if they wound up not being displayed.

@Dries: So, the only thing we seem to gain from using the menu system is a permission/access check? .... Actually, we don't even gain that, since again, it was already in there before, and in a slightly more flexible way (it was possible to use #access to set more restrictive access on a particular link than would occur on the page callback for that link, but if not set, the menu system's access was automatically used as a fallback).

I would say the possible things to be gained from @sun's approach are as follows:

1. No need for extra 'href' parameters in random places; the links would be defined in hook_menu(), so better code organization.
2. Use of hook_menu_alter() to be able to change them.

#1 is no doubt nice. But, node links have been around for a long time without that, and the world hasn't ended - it's not like we're introducing some crazy new pattern here.

#2 is good, but not really a complete solution to the problem. You'd still want to be able to alter the links differently in different contexts. In fact, at least in the latest patch, you still needed code like this wherever you wanted to add them -- '#attached_links' => menu_context_links('node/' . $node->nid). You'd then need to search through whatever the output of that function is and correctly change/remove things if you ever wanted to do the links differently in a particular context (and provide other modules a way to do that as well). And this is after you've already defined a bunch of extra things in hook_menu, including "special" versions of the menu item title and description that would be used for display on the links only?

I'm still not seeing how this would all work. It feels like we'd be categorizing things that just aren't as easy to categorize as we hope they are.

sun’s picture

Thanks, Dries, you successfully repeated the discussion the rest of us had in IRC (huh?). ;)

The solution is still dead simple, and as mentioned before, it's just a matter of will to accept that we can change it. But it is a paradigm shift, which needs constructive criticism, but also a sensible compromise to move forward.

The innards:

  1. Assigning links manually (the previous implementation): Here, we manually build a list of links (which includes to manually invoke many functions) for. every. single. box. on. the. page. During template rendering, we grab that custom list, check every single link whether the current user has access, and conditionally remove it. Additionally, there is no solid alter hook facility (because we cannot provide any context, which is kinda confusing, because these links are supposed to be contextual links...). Furthermore, we are linking to page callbacks, which need to exist in the menu system.
  2. Introducing a new sub-system: Here, we'll need to introduce a hook that is invoked for a certain object and returns a collection of links suitable for display as administrative links. That hook would look like:
    function node_context_links($node) {
      $links['edit'] = array('href' => 'node/' . $node->nid, 'title' => t('Edit'), ...);
      return $links;
    }
    

    Why $node? Because we have node_access() (which the menu system already accounts for). Little difference to the previous approach - we're still building link structures that already exist. And if we continue this example implementation, then we're almost back to an non-cacheable, custom implementation of hook_menu(). Why non-cacheable? Because the links are per-object.

  3. Leveraging the menu system: Here, we just take what's there. And, we categorize it. But we will filter out in a theme function, so themes can override the decision we take. We don't need new constants, because local tasks are still local tasks. All we need is a menu router item property that indicates
    • whether it is not a contextual link (a local task on a page; a "listing", unsuitable, whatever you want to call it)
    • whether it is a contextual link
    • whether it is both

    The example for comments is right here: http://api.drupal.org/api/function/comment_menu/7 - replace MENU_CALLBACK with MENU_LOCAL_TASK and add the property to indicate the nature of each task (or "priority", like webchick put it).

    The menu system only knows comment/%. We only retrieve those once. For all nodes/comments/foo/bar on the page.

    Usually, modules add contextual tasks as a local task, f.e. comment/%/report. If for any reason, a module really cannot do this (unlikely), then we still have hook_menu_local_tasks_alter(), which provides full context about the local tasks that are built. In there, you can equally assign the same property as in hook_menu() to control whether your local task is a contextual link or not.

    The example of edit and delete must be enhanced by load and render (Devel) for certain user roles. Oh -- aren't these registered already?

    If there is any meaning in "local", then displaying local tasks as contextual links gives it (finally) a meaning. And, given that the main content on a page is now a block... guess what? Do we render "local tasks" for the page... or... perhaps? For the system_main content block?

    Furthermore, we don't want to remove all "unsuitable" local tasks earlier. Because, we are talking an UI issue. The root of the problem we are concerned about is the sole amount of links. But. What if my theme is capable to display more links by using some nifty CSS? I can do that. Which means, I can do more, with a single click, from wherever I am currently.

Gábor Hojtsy’s picture

@Dries:

Either way, the difference between local task and local action is a major WTF. Even if we choose not to do the above, we should rename local task to local tab. In fact, why local? Is there a global tab too? It could just be task versus tab.

I think we've discussed this multiple times on other issues (like the local actions issue). Local tasks might not appear as tabs. They appear as subitems in admin_menu, they appear as navigation links in many Drupal themes (not tabs!). "Tab" is a display concept, and the API would ideally not name items after how they are going to displayed. If @sun's concept of using local tasks for in-place actions goes through, then they will be even less of tabs (and even more local as he explains).

Dries’s picture

- @sun wrote: Thanks, Dries, you successfully repeated the discussion the rest of us had in IRC (huh?). ;)

I typed my comments to webchick in a private Skype chat, so she must have passed these on into IRC on my behalf. I'm guessing though. I wasn't in IRC, nor did I read the IRC logs. Maybe I can read the telepathic channels?

- @sun, I'm still interested to see how far we get with the menu system approach (i.e. approach 3). Are you working on it still? It seems we all agree that it could potentially lead to some nice clean-ups and extensions to the menu system.

- @gabor in #146, yes, I was rambling last night. Let's not have that conversation in this issue. My wrong for getting it started.

David_Rothstein’s picture

I'd like to see if this approach bears fruit as well since I think categorizing menu items is useful, but with three days to go, it seems awfully scary to spend too much time on it. It's kind of a huge change. Let's try to summarize, though.

Here's a list of things that seem like they would need to be resolved for this plan to work:

  • Titles for the links. As discussed above, we can't just use the menu system titles here. As a concrete example, the title of 'node/%node/edit' is 'Edit'. For accessibility reasons, this isn't specific enough. In the patch, we've been using things like 'Edit article "the title of my article"'. How does that information get stored or added?
  • We need to rewrite block.module's URLs to follow the standard pattern. Probably not that hard (and probably a good thing to do in theory anyway), but still, it would (as far as I can tell) need to be done in order for this approach to work.
  • This is something that's the same for either approach, but it's important to point out - we'd still wind up with code like this in system.module:
    if (module_exists('menu')) {
     ... menu_context_links('admin/structure/menu/manage/' . $delta),
    }
    

    This dependency was deemed not good above, so if we need to solve it, we'd still need to solve it, and the only way would be through hook_block_view_alter(). So the point is that doing it all via the menu system doesn't magically solve the need for new alter hooks.

Performance:

I still don't understand the performance difference - maybe I'm missing something. You mention that the original approach needs to go through each box on the page and run each link through a bunch of functions. So does the new approach. You are calling things like menu_context_links('node/' . $node->nid) - i.e., it gets called for each node on the page. And then for each one, the corresponding menu item gets loaded, etc. I can see how there is a bit of a benefit since rather than loading each link separately, you are doing it one level up, so the database query gets a few tabs at once rather than one at a time. But still, once they are loaded from the database, they still need to be looped through and have access checked on each one, and any other functions that need to be called. I don't see how there's any other way to do it. You can't just store everything under node/% because it may be different. Just because a user has access to node/1/edit does not mean they have access to node/2/edit ... not to mention node/1/delete. So each one needs to be checked.

What that does convince me is that using the menu system at all for comments might be scary performance-wise (either way). With my approach, I know how to make comment.module's #attached_links implementation bypass the loading of any menu items entirely, with a single line of code. With your approach, I don't - I think you'd basically have to reimplement something like my approach for the #attached_links in the particular case of the comment module? (We aren't going to do that in this issue either way, I think, but we need to think about how that would work in a potential followup.)

Noyz’s picture

There's a general concern regarding the "Edit" shortcut link not being available for users that turn off or collapse the shortcut bar. Additionally there's a train of thought which suggests the "Edit" link stand on its own (not as a shortcut). As such, I propose this design change #602408: Adjust height of shortcut bar

xmacinfo’s picture

I think we should create a status bar, at the bottom of the screen, and use it to display the "Edit" shortcut link. This status bar could be made to look like the Localization Client bar, so that other module could put shortcuts on that bar.

I also think that the Admin Toolbar module should be responsible for displaying the status bar.

However this in another issue, though. We should first make sure that Edit links is comited and later we could choose where to display the "Edit" shortcut.

Gábor Hojtsy’s picture

@xmacinfo: that is why it was opened as a separate issue.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
31.16 KB

Since we only have two days to get this in, here's an updated patch going back to the approach left off in #126... most of the changes here need to be done no matter what anyway.

Changes:

  1. Links now have 'title' and 'description' and the 'title' shows up for screen-readers even when icons are being used.
  2. The icons now appear only via a dedicated theme function - thus, easier to override.
  3. Added a permission (called "view contextual links" for now) to control who has access to toggle the edit mode and see these links.
  4. Added alter hooks for viewing blocks (similar to as discussed in #435474: Addition of a hook_block_view_alter), which allows the menu module to correctly provide links for editing the system module's menus (these still need documentation).
  5. Slightly altered node_build_content() so that the node links could be added there (but still not wind up in the actual "content" that would go into e.g. search indexes), thereby allowing modules to attach or change links on nodes via existing hooks.
  6. Removed the code that made shortcut menu items non-editable, since it seems likely that the "edit mode" link is not going to live there in the end.
  7. Renamed edit_mode_toggle_page() to edit_mode_toggle() as per an earlier comment from @sun.
  8. Other small fixes (e.g., $node->title is now a field, etc).

Not done yet or remaining issues:

  1. Did not rename "attached links" to "operations"... upon further thought, that naming doesn't work because we often really do need to use the word "links" when referring to them, and "operations links" doesn't cut it for me. Maybe "contextual links" as per the terminology @sun is using? (I've already used that terminology in a place or two in the patch.) Let's not bikeshed it too much though :)
  2. Did not move the edit toggle out of the shortcut bar and into the header, since that is being discussed separately at #602408: Adjust height of shortcut bar.
  3. I think this probably could use a once-over in terms of defining the boundary between the pre-render function and the theme function... it's not totally clear to me where proper use of ['attached']['css'] is and where proper use of drupal_add_css() inside a theme function is? It would seem that the pure "theme-related" CSS (e.g., for the icons) might belong more in the latter.
  4. Responding to an earlier comment by Gábor in #113:

    Good to see support for separate rendering of the block and block content links - would it make sense to add provisions for similar functionality in nodes? so people can edit their attached image(s) and such via these links if exposed by the modules they use?

    I thought about that a bit, but it seems to me like blocks are a special case - for nodes, I don't think we want to pull any links out from inside the node content, because it would make the most sense for those links to still be displayed with the field that they are attached to (e.g., an image field). However, the field modules should definitely handle the possibility of displaying any attached links via their tpl.php files (and we should look into it in a followup issue).

Think that's it for now.

Gábor Hojtsy’s picture

Ok, maybe it does not make sense for nodes. I've tried and reviewed this patch again and would say that this half-baked shift to "contextual links" makes understanding what happens quite complicated. The permissions as well as some code level docs use "contextual links" while most of the patch / internal API / functionality uses attached_links. This clearly needs to be named consistently before the patch is committed.

Gábor Hojtsy’s picture

FileSize
194.9 KB

By the way, here is an updated screenshot. Not that the UI changed a bit, but to reassure those not applying every patch that this is how it looks now. Also need to add that the edit toggle not being in its down state constantly is confusing, but that again is solved by the cross-linked #602408: Adjust height of shortcut bar, so we should not obsess about that here.

Gábor Hojtsy’s picture

FileSize
35.39 KB

Now that the dashboard is in, we can also look at the interaction of this feature with the dashboard. As discussed there, this patch would give customization options for the blocks (and it does). Since the icons are also CSS recolorable and recolored in Seven, they blend in with the rest of the page. What's confusing though is that the dashboard has a customization feature, but the edit icons are independent. I can go into customization mode without seeing the edit icons and see the edit icons without otherwise customizing the blocks. Maybe not to be solved here but needs some thinking nonetheless.

Dries’s picture

Yes, we should have those icons on the dashboard too! Very cool, and very much inline with what other dashboard do.

dmitrig01’s picture

Mostly minor nitpicks - I think that given the timeframe, it's more productive to get this in and then put edit links on the dashboard.

+++ misc/attached_links.js	14 Oct 2009 02:51:27 -0000
@@ -0,0 +1,31 @@
+/**
+ * Highlights the region of the page that an attached link is associated with.
+ */

It would make more sense to me to highlight a region on mouseover of the region, not on mouseover of the link.

+++ misc/attached_links.js	14 Oct 2009 02:51:27 -0000
@@ -0,0 +1,31 @@
+
+    // Trigger the behavior when hovering over the link.
+    $('.attached-links-link').hover(addHighlight, removeHighlight);

Can't you just inliine addHighlight and removeHighlihgt?

+++ modules/block/block.module	14 Oct 2009 02:51:27 -0000
@@ -280,6 +280,26 @@ function _block_get_renderable_array($li
     unset($block->content);
+    // Attach links to the block. Skip this for empty blocks and also for the
+    // main system content block, which has nothing useful to configure.
+    if (!empty($build[$key]) && ($block->module != 'system' || $block->delta != 'main')) {

Isn't there a better way to figure out what has configuration and what doesn't?

+++ modules/system/system.module	14 Oct 2009 02:51:27 -0000
@@ -248,6 +254,10 @@ function system_permission() {
     ),
+    'view contextual links' => array(
+      'title' => t('View contextual links'),

Wouldn't it be more logical to call this "use edit more"?

This review is powered by Dreditor.

David_Rothstein’s picture

New patch - the only change is that I filled in the last @todo by adding documentation to the new block alter hooks. Responses to the above (I wrote the patch before seeing Dmitri's feedback):

1. To clarify, Gábor's screenshot in #155 is real - the links are already on the dashboard :) They just magically appeared when the dashboard module was committed and this patch was applied... very nice. In fact, with the particular case of the Seven theme used as an admin-only theme, they look so good that it might make sense to leave them showing all the time. For the other issues in #155, seems like we should leave the details to #601150: Improved UI for contextual links since it will depend on that. I'd suggest that part of the solution is that "Customize" is too generic of a word to use on the dashboard page since it is too open to interpretation...

2. Re #153, I'm happy to change it to "contextual links" everywhere once there is agreement to do that. Only want to do it once :)

3.

It would make more sense to me to highlight a region on mouseover of the region, not on mouseover of the link.

I think that might be right, although I feel like we tried that at one point above and there was something weird about it? One issue is if you have overlapping regions (e.g., a View that contains nodes), it can be hard to figure out what should be highlighted - but of course, it's hard to move your mouse to the tiny region of the page where the icon is also...

4.

Can't you just inliine addHighlight and removeHighlihgt?

Sorry, I'm JavaScript-impaired enough that I'm not sure exactly what you're suggesting there :)

5.

Isn't there a better way to figure out what has configuration and what doesn't?

Everything has configuration - the problem is that we want to hide links to the configuration page for this particular block because they are intrusive and get in the way, and the main system content block is so weird and special that we really don't ever want to encourage anyone to visit the configuration screen for it.

I think maybe the code comment there could use some improvement, though.

Another one we might want to consider special casing: The system help block. The links on that block also just get in the way and don't really serve any purpose.

6.

Wouldn't it be more logical to call this "use edit more"?

I think in #601150: Improved UI for contextual links we're hoping to deemphasize the "edit mode" as the only way in which these links will appear, so I was trying to name the permission in a way that will fit with that.

David_Rothstein’s picture

Tried to think in a bit more specific detail about what it would take to move this to the hook_menu() approach advocated by @sun. Not sure this could really happen in one day, but anyway, the remaining major tasks to move to that approach seem like:

  1. We would need to add a minimum (I think) of four new columns to the menu_router table:
    • 'is_contextual_link' (or whatever we would call the thing that determines whether the menu item is exposed that way). This would have to work with MENU_LOCAL_TASK, MENU_LOCAL_ACTION, or MENU_CALLBACK (I don't think there's any way to avoid having it be all three).
    • 'description callback': We could probably reuse the existing description field since I don't think it's really used for any other purpose for the above types of links - however, it needs to become dynamic to support the use cases of the patch, thus we need this.
    • 'description arguments': Ditto.
    • 'contextual link title': I think we can fall back on the menu item title for this sometimes, but not always. This is just the one word title (e.g., 'Configure' or 'Edit') so it doesn't need to support arbitrary title callbacks.
  2. Make the contextual link functionality actually load the correct items for MENU_LOCAL_TASK, MENU_LOCAL_ACTION, and MENU_CALLBACK (as above).
  3. Fix the block module's URLs to conform to the required standard.
  4. Either bypass the menu system altogether in cases where you need higher performance on these links, or do some serious performance improvements to menu_get_item(), or something - because to extend these to comments, it feels like loading and processing hundreds of menu items on a single page load isn't a great idea.
mcrittenden’s picture

@David_Rothstein: for #4, I think Dmitri's saying you don't have to make functions for addHighlight and removeHighlight, you can just put the code for them right inside that .hover() call.

Awesome work on this, btw. It's really coming together well.

David_Rothstein’s picture

Ah, thanks - I was thrown off by the word "inline" and started thinking about inline JavaScript (i.e., directly on the page) or something :)

We could do that, but seems to me like it might be a bit more readable to keep them as separate named functions.

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
34.34 KB

Rerolled to chase HEAD and with small changes as per #158.5 (improved code comments and excluding the system help block from displaying links).

I had no energy to rename "attached links" to "contextual links" just yet. We'll wait for a green light that that will be the final name.

BTW, as per IRC discussion @sun may be doing some work to pursue the hook_menu() approach, so we'll see what happens with that (miracles can happen!). But as for this approach, I think this patch ought to be basically committable.

yoroy’s picture

Issue tags: -Needs usability review

Played around with this a bit and it's really cool: so much more intuitive to move a shown block to another region for example. Working in context of what you are looking at instead of having to go birds-eye and navigate through toolbar hierarchies. It's great UX.

Also:
- It looks ugly as hell but we can fix that later. Needs more subtle :-)
- I also wondered if a node's 'Edit' local_task should be able to do the same hover styles. I'm hearing from tha_sun that's not feasible for now. Ok, we can probably fake it quite a bit with css I think. (http://img.skitch.com/20091015-mj8r7414suwkiw1cj51mbxsu7.png). Local tasks is a big topic in itself anyway.
- All in all even for blocks and menus alone a really big improvement.

UX-wise RTBC me thinks

mcrittenden’s picture

A quick extremely nitpicky review. None of these things should prevent a commit, IMO, but it's all I could find (and that's a good thing!).

+++ misc/attached_links.css	15 Oct 2009 22:19:41 -0000
@@ -0,0 +1,62 @@
+  -moz-border-radius: 3px;
+  -webkit-border-radius: 3px;

Can we add plain old border-radius here too? D7 will be around for quite awhile, long after browsers start supporting that property.

+++ modules/block/block.api.php	15 Oct 2009 22:19:41 -0000
@@ -151,6 +151,70 @@ function hook_block_view($delta = '') {
+  // Add a theme wrapper function defined by the current module to all blocks
+  // provided by the "somemodule" module.
+  if (is_array($data['content']) && $module == 'somemodule') {
+    $data['content']['#theme_wrappers'][] = 'mymodule_special_block';
+  }

What is somemodule? Is this some convention I don't know about? I'm confused as to how it can be hardcoded right in the condition.

+++ modules/system/system.module	15 Oct 2009 22:19:44 -0000
@@ -2883,6 +2918,20 @@ function system_timezone($abbreviation =
 /**
+ * Menu callback; Toggle the global edit mode and redirect.
+ */

Very nitpicky: could use proper English here.

+++ modules/system/system.module	15 Oct 2009 22:19:44 -0000
@@ -3444,6 +3493,133 @@ function system_date_format_delete($dfid
+    // Transform and add to the link.

"Transform" doesn't really tell me anything.

I'm on crack. Are you, too?

yoroy’s picture

Best spell out the suggested the proper English for the non-natives then please.

sun’s picture

Issue tags: +Needs usability review
FileSize
22.2 KB

So, yeeha, David's last changes contained some really good ones that allowed me to proceed further.

Just posting something in between so the world knows that something IS happening. ;)

....

And, yes, my secondary goal is to reduce the size of this patch. ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review

David's patch is still CNR for now.

sun’s picture

FileSize
22.22 KB

Quick notes on the current state: (also talked to David and yoroy in IRC)

- All logic for those links has been moved into system.module, which means less cluttering of API functions like theme().

- The edit-mode-toggle has been removed, and we go with a simple (system.module) permission for now. Displaying and/or hiding those links is a pure theming issue, and even the theme's JS is able to save/load a cookie on the client to toggle the edit mode. Optionally, this functionality could also live in Toolbar module - but still, we don't need backend code to support that.

- The icons are gone. That, again, is a pure theming issue, and we need to flesh out how to properly deal with icons in Garland and Seven theme, and, well, in all other contributed and custom themes. Turning text links into icons is a novice job for all real themers on this world, so we will not hold off this patch with that discussion. In addition, pure CSS/JS changes is what UX freeze phase is for.

This patch is "testable", but I'm still revamping some stuff and removing obsolete code.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
21.78 KB

This one should at least come back green, or skyrocket to mars.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
26.8 KB

8 exceptions? I can handle 8. ;)

This one also contains some proof-of-concept code for comment module. And, yes, the result is: Administrative tasks for comments *can* be auto-loaded and displayed contextually.

sun’s picture

FileSize
24.79 KB

Last patch with those proof of concept changes.

sun’s picture

FileSize
20.44 KB

More clean-ups, removed proof-of-concept code.

sun’s picture

FileSize
20.07 KB

A bit more less.

sun’s picture

I need a quick break, so I want to summarize the current state:

- This works very fine, and I hope the implementation is 99% clean.

- The text links are surely looking a bit strange, but that's a pure CSS/theming issue, and has to be dealt with separately per core theme. Nothing to deal with in here.

- Our local task titles are very suitable for this already... I am reading here: "Edit menu", "Configure block", etc. Hence, I don't see the need for separate titles or anything like that.

- What's currently missing is the discussed new property for menu router items that allows to indicate the "priority" or assumed "visibility" of a local task, i.e. whether it's assumed to show up only in the primary local tasks on a page, only in contextual links, or both. Should be easily doable by just adding a new column to menu_router and saving that info accordingly.

Well, if we'd skip the property, and just assume we didn't know, then a contextual "Revisions" task on nodes would count as a bug report. :P

Dries’s picture

I played around with the latest patch and it seems to work. Haven't looked at the cod yet.

It didn't work for comments yet, so I'm wondering if that might be do-able.

Let's get this sucker in today! Thanks for all the hard work so far @sun and @David_Rothstein.

Dries’s picture

Talked to sun about the comments problem. We agreed that it made sense to convert comment editing/deleting to local tasks. We also agreed that converting comments could happen in parallel by someone else. This should be fairly easy to do, so I suggest we create an issue for that.

sun’s picture

FileSize
25.48 KB

- Minimally more styling.

- More PHPDoc.

Note to anyone who's working on the comment path changes: The patch in #175 contained the first steps to accomplish it. You can download that, remove all hunks unrelated to modules/comment/* and just add comment/%comment/edit to the game (delete is already converted in there, and, is a very nice clean-up on its own if you have a look at the removed functions ;).

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
29.67 KB

- Added new hook_menu() property "tab_type" to control the placement of local tasks along with docs. Quick summary: Can be one of

- "view": Display in page context only. (This is the default, because we want to opt-in)

- "task": Display in all contexts, i.e. both a local task on a page and as contextual link/task.

- "context": Display as contextual link/task only.

sun’s picture

FileSize
29.66 KB

oops, copy/paste error.

effulgentsia’s picture

subscribing

David_Rothstein’s picture

OK, I took a very close look at this patch and it is really really good. It does seem like this is the most flexible method and the right way to go.

Here's an initial review focusing first on the API-level and accessibility things that we need to at least understand or have a plan for to get this committable as soon as possible.

API changes and accessibility

  1. We'll need a new menu router database column with three states. @sun added this while I was reviewing :)
  2. Accessibility question: Do links need descriptions? (If so, two new database columns.). This is a question for the accessibility folks; in the previous patch, the links had (hover) text associated with them along the lines of "Configure the XYZ block" or "Edit article XYZ". In the current patch, the only text is "Configure block" or "Edit". Is that OK? Maybe someone can try out the patch and give some feedback?

    @sun and I discussed this a bit in IRC yesterday but I'm not sure either of us are accessibility experts (I know I'm not). Seems to me like they need them. Looking at the history of this issue, it looks like @webchick in #51 was the first person to say this:

    (side-note: for accessibility, "Configure this block" should probably be "Configure the XXXX block" and so on for nodes)

    Everett Zufelt in #86 seemed to agree.

    If we need them, see part of my comment in #159 - it involves adding "description callback" and "description arguments" columns to the menu_router table. I actually think this is a good change, but it is another new API and the menu router table keeps getting bigger and bigger, so let's be prepared.

    It's also not a change that would be put in just for this one feature. I discovered this gem that is currently in node.module:

      // @todo Remove this loop when we have a 'description callback' property.
      // Reset internal static cache of _node_types_build(), forces to rebuild the
      // node type information.
      drupal_static_reset('_node_types_build');
      foreach (node_type_get_types() as $type) {
        $type_url_str = str_replace('_', '-', $type->type);
        $items['node/add/' . $type_url_str] = array(
          'title' => $type->name,
          'title callback' => 'check_plain',
          'page callback' => 'node_add',
          'page arguments' => array(2),
          'access callback' => 'node_access',
          'access arguments' => array('create', $type->type),
          'description' => $type->description,
          'file' => 'node.pages.inc',
        );
      }
    

    So this feature would be more broadly useful than just for this patch - which is good!

Other things (can be for followups, etc)

  1. Comments: Converting them to use local tasks does seem like a good cleanup for a separate issue, but before we try to attach the contextual links this way, we'll need to solve the performance problem mentioned in a previous comment.

    Looking at the code, it seems to me like there might be some performance improvements than can be made in menu_get_item() that would help here, and @sun already has a todo/bug about issues with menu_local_tasks() that are similar. Sounds like a post-freeze followup to improve the performance - I think it can be done. (And worst-case scenario, we can still attach the links manually in the particular case of comments.)

  2. +    // Always unset the default, because it is not a real task.
    +    if ($task['#link']['type'] == MENU_DEFAULT_LOCAL_TASK) {
    +      unset($tasks[$key]);
    +      continue;
    +    }
    

    Actually we'll need to remove this restriction - in the case of menus, we actually do want it to point to "list links" (a default local task) - the current patch points to a less-useful screen.

  3. +  // @todo This access check really doesn't belong into an API function. But as
    +  //   of now, there is no better location to entirely prevent the loading and
    +  //   building of contextual links in case they should not be rendered at all.
    +  if (!user_access('view contextual links')) {
    +    return array();
    +  }
    

    Seems like this can just go in a simple wrapper function?

  4. We seem to have lost the ability to have "block configuration" links and "block content" links rendered separately? (I think it is not terribly hard to add back though.)
  5. Deferring icons/theme stuff to a followup: Yes, at this point, of course .... However, there is no way we aren't going to use icons at least for Seven - they just look way too good on the dashboard :) But we have a good theme function for that in one of the earlier patches - so easy to add back in the correct place in a followup.

    Boy, though, this really makes me wish my comment #13 had gotten some more support, when I wrote: I purposely did not put any work into theming the links nicely yet - so if we could commit this patch without any theming that would be fine with me... We maybe could have been getting to the stuff we're doing now in July rather than October :)

  6. +Drupal.attachedLinks.hover = function () {
    +  $(this).addClass('attached-links-link-active')
    +    .parents('.attached-links-region').eq(0).addClass('attached-links-region-active');
    +};
    

    Wow, those four lines of code allowed about 25% of the PHP code to be removed, didn't they? Guess we needed a jQuery expert on this patch earlier :)

sun’s picture

Anyone with a free slot available, please help over in #606608: Use proper menu router paths for comment/* - thanks!

sun’s picture

FileSize
28.07 KB

And, yeeeeha! This patch solves all of the points David mentioned except 3), as far as I can see.

I want to do another round of optimization, but this really really really works very very very nice. 8)

David_Rothstein’s picture

I also created #606640: Use proper menu router paths for the block module (but no argument loader functions) as a similar issue for the block module.

BTW, quick (minor) note: The above patches are now missing the drupal_get_destination() that redirected you back to the place you came from when you finish doing your edits.

sun’s picture

FileSize
30.77 KB

- Fixed the destination.

- Fixed some PHPDoc.

So.... I am quite happy with this patch now. Time for final reviews - while we wait for #606608: Use proper menu router paths for comment/* and #606640: Use proper menu router paths for the block module (but no argument loader functions).

webchick’s picture

Code review:

+++ includes/menu.inc	16 Oct 2009 19:44:01 -0000
@@ -1756,6 +1757,86 @@ function menu_local_tasks($level = 0) {
+ * - node/%node/load with tab_type "view"

Can we use node/%node/revisions as the example here, since that comes from core?

+++ includes/menu.inc	16 Oct 2009 19:44:01 -0000
@@ -1756,6 +1757,86 @@ function menu_local_tasks($level = 0) {
+ * - node/%node/render with tab_type "context"

I'd prefer to use a more real-world example here. It's important that people understand the differences between these types.

How about node/%node/report_as_spam or something?

+++ includes/menu.inc	16 Oct 2009 19:44:01 -0000
@@ -1756,6 +1757,86 @@ function menu_local_tasks($level = 0) {
+function menu_contextual_links($path) {
+  // @todo This access check really doesn't belong into an API function. But as
+  //   of now, there is no better location to entirely prevent the loading and
+  //   building of contextual links in case they should not be rendered at all.
+  if (!user_access('access contextual links')) {
+    return array();
+  }

Hm. This is too bad. Can we create a wrapper function that checks access and calls menu_contextual_links(), similarly to how form submit functions will sometimes check access before calling node_save()?

+++ includes/menu.inc	16 Oct 2009 19:44:01 -0000
@@ -1756,6 +1757,86 @@ function menu_local_tasks($level = 0) {
+
+  // Allow modules to alter contextual links.
+  drupal_alter('menu_contextual_links', $tasks, $router_item, $root_path);

Need documentation for this hook.

+++ modules/menu/menu.api.php	16 Oct 2009 15:25:37 -0000
@@ -199,6 +199,18 @@
+ *   - "tab_type": (optional) Defines the type of a tab to control its placement
+ *     depending on the requested context. By default, all tabs are only
+ *     displayed as local tasks when being rendered in a page context. All tabs
+ *     that should be accessible as contextual links in page region containers
+ *     outside of the parent menu item's primary page context, should be
+ *     registered using one of the following types:
+ *     - view: (default) The tab is displayed as local task for the page context
+ *       only.
+ *     - task: The tab is displayed both in the page context and as contextual
+ *       link.
+ *     - context: The tab is displayed as contextual link outside of the primary
+ *       page context only.

We really need to work on these names, since they aren't clear at all. :\
task = view + context is about as obvious as mint ice cream = goose + purple.

Dries pointed out that we should probably base them off what a non-technical user would call them, based on their default visual rendering. Too much catering for semantics can lead to confusion.

Sun suggested:

tab_type => context
view => page
context => container
task => all

"container" is not perfect, but all in all I think this is the right approach.

Docs also need to be adjusted to point out that these are only for MENU_LOCAL_TASK items.

+++ modules/node/node.tpl.php	16 Oct 2009 17:56:11 -0000
@@ -18,6 +18,7 @@
+ * - $contextual_links (array): An array of contextual links for the node.
@@ -74,6 +75,10 @@
+  <?php if (!$page && $contextual_links): ?>
+    <?php print render($contextual_links); ?>
+  <?php endif; ?>

(I'm too tired to spot this, but sun pointed it out.)

We need to refactor this so that it doesn't require a tpl.php file to display contextual links.

+++ modules/system/system.module	16 Oct 2009 18:43:54 -0000
@@ -243,6 +243,10 @@ function system_permission() {
+    'access contextual links' => array(
+      'title' => t('Access contextual links'),
+      'description' => t('Access contextual links associated with items on the page.'),
+    ),

Maybe explicitly mention here that this will give you access only to the links you have access to. Except. You know. More clearly than that. ;P

This review is powered by Dreditor.

sun’s picture

FileSize
33.78 KB

So let's talk about performance. Go effulgentsia, go! :-D

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
32.76 KB

We love performance.

sun’s picture

FileSize
35.99 KB

Debugging the menu system.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
36.53 KB

New patch for performance benchmarking. Menu system issues are resolved (well, workaround'ed).

sun’s picture

FileSize
37.69 KB

And. Here. We. Go.

Thank you for participating in this nice effort.

I especially want to thank

- David_Rothstein for coming up with some nice code,
- yoroy for helping with UX decisions and finding the best way forward,
- effulgentsia for doing a bloat-load of performance benchmarks,
- agentrickard for diving into menu system innards for debugging,
- webchick for being tired and doing not-so-lengthy reviews :P, and
- Dries for listening. 8)

sun’s picture

FileSize
38.01 KB

Two minor fixes after in-depth review.

Thank you again, webchick! Awesome review! :)

webchick’s picture

Status: Needs review » Needs work
Issue tags: +Needs documentation

I gave my review in IRC, but basically the problems I noted were:

1. Comments have a delete link, so should nodes. If we want to talk about applying this pattern wholesale, we can look at #196758: Having delete as a button on the node edit form means certain users don't have access to it when they should. (fixed in this patch)

2. I was getting a notice on comment.tpl.php from Seven/Stark (fixed in this patch)

3. The approve link shows up on published comments, and also lacks a confirm form. (re-opened #606608: Use proper menu router paths for comment/*)

4. Visually, there are some issues with this. Those who looked at this patch much earlier probably would scream that what got committed was a huge regression, since icons are gone, all the links appear at all times, etc. Screenshot:

Edit in place

However, sun and yoroy discussed this, and came to the conclusion that the final interaction here will need some serious thought put into it, and splitting the API/enabling parts from the UI/interface parts makes a lot of sense since we're already at issue comment #200. I can get behind that.

5. Primary/Secondary links don't have edit links next to them. This will get into some discussion about how best to present an edit link on links ;P so this can also be moved to a follow-up.

Since I could find nothing left to hold this patch, I went ahead and committed it to HEAD!

Great work on this, sun and David, and everyone else who helped get it out the door. This is a huge patch for Drupal. Awesome.

Those API changes to the menu system need to be documented in the upgrade guide. Marking needs work. Also, I think we're missing a CHANGELOG.txt entry. Oopsie!

effulgentsia’s picture

Status: Needs work » Needs review

Here's some performance info: I did a fresh install of HEAD (default profile) before patch #199 was committed. Used devel generate to generate 50 nodes. Manually generated 5 comments (devel generate wasn't working with comments). Gave "anonymous" role "access comments". Went to block configuration page and for all disabled blocks, put them in either left or right sidebar. Then used "ab" (Apache Bench) for the home page:

Before patch: 98ms
After patch: 104ms

There is no visual difference between these two, because anonymous user can't do any of the actions for which a context link would show up. But, the code still has to check each of those potential links, and sun worked hard to optimize that. This is for a page with a lot of context links to evaluate access for (10 node teasers and 6 blocks), so 6% slow down isn't too bad, but I wonder if it can be made any better.

effulgentsia’s picture

Status: Needs review » Needs work

status change in #201 was unintentional

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.26 KB

Sorry for forgetting that.

Please back to work after committing.

sun’s picture

FileSize
3.47 KB

oh wow, sometimes, it doesn't hurt to read ...

catch’s picture

#204 is good.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed the follow-up patch.

chx’s picture

Status: Fixed » Active

6% slowdown is not bad???? Anything goes if it's usability??

sun’s picture

Status: Active » Needs work
Issue tags: -Needs usability review +Contextual links

Created the follow-up issue for tweaking performance: #607244: Decrease performance impact of contextual links

@all: Please tag all follow-up issues, regardless of whether DX or UX or whatever, with Contextual Links, thanks! (so no need to follow-up and link them here)

To provide some clues about the necessary follow-up issues:

- Decrease performance hit (see above)

- Figure out a proper interaction (JS/CSS) for contextual links (yoroy + Bojhan are working on this already)

- Add a new "description callback" menu router item property to allow to shorten visible link texts and move a longer contextual description into TITLE attribute (though the accessibility team proposed to use a hidden H2 element for the regular local tasks in the page context, so maybe that should be discussed first -- in a separate issue)

- Think about icons and styling of icons per-theme (CSS)

Back to needs work for documentation of menu API changes in the module upgrade guide.

JacobSingh’s picture

What type of benchmarks were done?

This is not 6% on production machines, just 6% on some benchmark revealing very little methodology. I'm not saying it is useless, perhaps it is a cause to do a real benchmark, but it is not a cause for premature optimization.

If I am not mistaken, this patch is for people with write access to content on the website and the permission to see the UI. Given the UI, I'm going to go out on a limb here and say 98% of them are "admins". IF their performance is decreased by 6%, but their productivity is increased by 50% or even 10%, it is a win, not a loss. Further, if an admin takes less clicks to get where they are going, the put less strain ultimately on an infra, if that is a concern.

again, I appreciate the work to bench it, and I don't feel it is useless, but before we panic, can we have to look at benchmarks against expected usage and somewhat real-world conditions?

Best,
j

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community

Jacob: please see http://drupal.org/node/607244#comment-2162936. I don't have production server numbers, but perhaps the info in that comment can be a guideline for how someone can generate those.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Damn it! I am so frustrated by Firefox retaining the state of "select"s when a page is refreshed!!! Setting back to NW.

jrabeemer’s picture

efful: that seems like a JS post-load form reset on selects.

Gábor Hojtsy’s picture

@JacobSingh, @sun, @chx: that comment actually talks about a 6% performance decrease **for anonymous**, which if reproducible is a major problem.

eigentor’s picture

er, er... I want to user-test this. But where the heck do I activate the edit mode? :) No Shortcut in Toolbar, no module to be activated...

effulgentsia’s picture

It's just on. If you have permission to configure a block, then when you're on a page with blocks, you have "Configure block" links at the top-right of each block. If you have permission to edit a node, then you have "Edit" links at the top-right of each node teaser. Same with menus and comments. It's possible some kind of setting will be added in the future to be able to turn it on/off, but at this time, it's always on.

catch’s picture

David_Rothstein’s picture

It's possible some kind of setting will be added in the future to be able to turn it on/off, but at this time, it's always on.

Just to point out for those not following #607244: Decrease performance impact of contextual links, the fact that the initial committed patch left the feature "always on for everybody" is the reason why it led to a large performance hit for anonymous users. So there are simple ways to deal with that problem.

I'm late commenting here, but just wanted to say that it's great this got in, and thanks to @sun and everyone for helping improve it and bring it home in the end :) There's definitely some followup work to do, and I'll try to file some additional "contextual links" followups in the next few days.

David_Rothstein’s picture

There's one followup that could get its own issue, but needs an answer soon, and since a bunch of people interested in accessibility are already subscribed to this issue, perhaps worth doing here....

People interested in accessibility:

Please re-read the first part of comment #186 and respond. As stated there, if this issue with the link descriptions needs fixing, it requires an API/schema change, so it's important to do soon :)

David_Rothstein’s picture

One more thing :)

- Figure out a proper interaction (JS/CSS) for contextual links (yoroy + Bojhan are working on this already)

Is there an issue where this is being worked on?

I just went and added the "Contextual links" tag to the issue at #601150: Improved UI for contextual links which was intended to be for the UI but has not exactly seen a whole lot of subscribers. Just wanted to make sure that duplicate work doesn't happen.

David_Rothstein’s picture

[accidental double post]

te-brian’s picture

Probably not the best place for this, but I thought you guys would be interested in seeing my quick implementation of this new "contextual links" api.

As a way to learn how this patch works, I whipped up a module that adds contextual links to the site_name and site_slogan (with some minor help from the theme).

Here's a screen cast: http://screenr.com/XcN

Edit: Oh yeah.. I also applied some CSS to bring back the icons.

David_Rothstein’s picture

Re #221, that screencast looks great! I think it would be wonderful if core allowed editing the site name and slogan via contextual links. Depending on how your code works (I wonder if it required a hack, given the way these page elements get placed in Drupal right now?), it might make sense to create a followup issue to add it to core; if so, please tag the issue with "Contextual links".

For icons and such, #601150: Improved UI for contextual links is the place to go.

Bojhan’s picture

@David - the follow up is turning them into blocks :) To be honest, heh

te-brian’s picture

Re #222, it did require a "hack" in that , as you said, the site_name and slogan are not really alterable in garland's implementation of page.tpl.php (which most themes follow). The way I pulled it off is I changed those two elements to be renderable arrays inside a $page['elements'] array. That way, my module could alter $page['elements']['site_name'] and $page['elements']['site_slogan'] and add the contextual links to them. The links themselves point to a /admin/config/editable_elements/%/edit callback. I am honestly not sure how core would best pull this off. After having the power to alter all the regions and page content, it does seem somewhat silly for phptemplate implementations of page.tpl.php to just print out other stuff like the site_name and menus as "hard coded" variables, so to speak. You could currently change the text of the title, but changing the markup that wraps it is more difficult.

mgifford’s picture

Missed this previously. Don't think we need a description field as such, but putting in more details so that "Configure this block" is listed as "Configure the XXXX block" and so on for nodes is important.

As per #49428: Include node title in "Read more" link for better accessibility and SEO - replicated links are a problem. Something like:

<?php
t('Configure')  . '<span class="element-invisible">' . t(' the content ') . strip_tags($node->title[FIELD_LANGUAGE_NONE][0]['value']) . '</span>'
?>
David_Rothstein’s picture

Don't think we need a description field as such, but putting in more details so that "Configure this block" is listed as "Configure the XXXX block" and so on for nodes is important.

Well, "configure the XXXX block" is actually exactly what I meant by a description field :) The "XXXX" part is what makes it complicated - as things now stand, we'd seem to need new columns in the database to store the info that allows us to provide that.

But: I actually had an email conversation with Cliff about this a while ago (I thought he had posted back here but I guess not). His opinion - as I remember it - was that just having "Configure this block" is actually OK as long as the link is placed correctly. For example, we usually have an <h2> that contains the title of the block (or of the node, whatever), and if we were to put the contextual links in the HTML immediately after that title, the theory was that that would provide enough context.

Right now, we are not doing this consistently - in most (all?) places we are putting the links before the <h2>. But changing this isn't hard. It could probably be done as part of #601150: Improved UI for contextual links. Assuming that's good enough, we could then defer any bigger changes to Drupal 8.

bowersox’s picture

+1 to the suggestion of saying "Configure this block" and putting that link immediately following the block title h2 heading.

joachim’s picture

Please can we rethink the little gear icon that's appeared when I updated from CVS today?
And in general, resist the urge to put little meaningless icons on everything?

mcrittenden’s picture

@joachim: for discussion on the icon, see #601150: Improved UI for contextual links.

Cliff’s picture

@David, sorry I haven't been able to stay up with the development of this issue (or most others, for that matter), but you're close to right about what I said about context. The only difference is that the "Configure this block" link doesn't have to go immediately after the <h2>. It just has to be the only "Configure this block" link between two headings of whatever level separates blocks. So either of these is OK:

First example:
<h2>Block 1</h2>
Blah de blah lots of text here.

<a href="url">Configure this block</a>

<h2>Block 2</h2>
A lot more text here.

<a href="url">Configure this block</a>

Second example:
<h2>Where We Talk About Blocks</h2>
<h3>Block 1</h3>
Blah de blah lots of text here.

<h4>More stuff about Block 1</h4>
text here
<h4>Still more stuff about Block 1</h4>
text here
<a href="url">Configure this block</a>

<h3>Block 2</h3>
A lot more text here.

<h4>More stuff about Block 1</h4>
text here
<h4>Still more stuff about Block 1</h4>
text here

<a href="url">Configure this block</a>

<h2>Where We Go on to Other Stuff</h2>

The whole point, as WCAG 2.0 recognizes, is context. Taken in context, the meaning of each "this block" is clear.

There is a problem, in that people using screen readers can't use the "List all links" feature to quickly find the link for configuring the Admin block, but that's similar to the experience of a sighted person. We can't look only at each "Configure this block" link and figure our where it goes — we have to read the surrounding information for that context. And anyone using a screen reader can quickly scan the headings for "Admin Block," go to that heading, and then skip from link to link in that section until they hear, "Configure this block." That's perfectly analogous to the experience of a sighted user.

Does that make sense to you?

sun’s picture

Status: Needs work » Fixed

I'm not sure which of the latest follow-ups are still relevant as of now, so I'm marking this issue as fixed and kindly ask you to create separate issues for any remaining things.

Please make sure you tag all related issues with "contextual links", see also http://drupal.org/project/issues/search/drupal?status[]=Open&issue_tags=...

David_Rothstein’s picture

Yeah, I think this is about done.

Cliff, thanks for the nice explanation - it does make sense. Note that as of now (due to the patch that was committed at #646874: Modules like Contextual links and Shortcut cannot add to "template regions"), all of the contextual links should be automatically appearing after the relevant <h2> or other heading, so that part is all fixed.

I did go ahead and create #687842: Add a "description callback" property to menu items as an issue against Drupal 8, since it seems like it might be useful to have that someday in order to make these links more descriptive anyway (as well as for other reasons).

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

JayKayAu’s picture

Hi all,

At the risk of sounding completely out of my depth (which would be true!), I'm wondering if it is possible to backport this to D6, perhaps as a module?

In our case, there are a bunch of legacy sites that occasionally come back to us for maintenance, and it'd be lovely to add this onto them.

But, I'm *really* looking forward to D7!

dman’s picture

@JayKayAu Short answer, for D6, try http://drupal.org/node/473268#comment-1645792

JayKayAu’s picture

Fantastic, thank you!

rfay’s picture