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:
- The theming of the edit links is pretty ugly in some places.
- 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.
- 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.
- 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 :)
Comment | File | Size | Author |
---|---|---|---|
#204 | drupal.edit_.204.patch | 3.47 KB | sun |
#203 | drupal.edit-follow-up.patch | 2.26 KB | sun |
#199 | drupal-edit.199.patch | 38.01 KB | sun |
#198 | drupal-edit.198.patch | 37.69 KB | sun |
#197 | drupal-edit.196.patch | 36.53 KB | sun |
Comments
Comment #1
mcrittenden CreditAttribution: mcrittenden commentedMarking 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.
Comment #2
Dries CreditAttribution: Dries commentedTracking it!
Comment #4
ksenzee@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.
Comment #5
Gábor HojtsyThinking 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.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedGá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.)
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedRegarding 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).
Comment #8
catch$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.
Comment #9
Gábor HojtsyDepending 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 :)
Comment #10
yoroy CreditAttribution: yoroy commentedSome 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.
Comment #11
Dries CreditAttribution: Dries commented+1 for what yoroy said in #10. :)
Comment #12
dvessel CreditAttribution: dvessel commentedsubscribing
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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):
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 :)
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedInteresting 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.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedI'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 :)
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedOn 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.)
Comment #17
univate CreditAttribution: univate commentedThis 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.
Comment #18
mcrittenden CreditAttribution: mcrittenden commentedRe: #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.
Comment #19
Dries CreditAttribution: Dries commentedThe 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.
Comment #20
Noyz CreditAttribution: Noyz commentedsubscribing. 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.
Comment #21
dman CreditAttribution: dman commentedSubscribe - 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.
Comment #22
jp.stacey CreditAttribution: jp.stacey commentedSubscribing. 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.)
Comment #23
michaelfavia CreditAttribution: michaelfavia commentedSubscribing.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedHere is an updated patch. Some changes:
Otherwise, this patch is still held up on going much further based on the discussion about the API, as mentioned in #13...
Comment #25
tstoecklerDidn't apply cleanly -> rerolled.
Noticed that the links in your screenshot didn't appear for me. Is there anything that needs to be done?
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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).
Comment #27
Bojhan CreditAttribution: Bojhan commentedIs 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.
Comment #29
sunsubscribing
Comment #30
cwgordon7 CreditAttribution: cwgordon7 commentedHere'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.
Comment #31
cwgordon7 CreditAttribution: cwgordon7 commentedAlso an annotated screenshot.
Comment #32
webchickSubscribe
Comment #33
Gábor HojtsyA 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).
Comment #35
Dries CreditAttribution: Dries commentedWhile 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.
Comment #36
mthart CreditAttribution: mthart commentedsubscribing
Comment #37
JeremyFrench CreditAttribution: JeremyFrench commentedSubscribe
Comment #38
cwgordon7 CreditAttribution: cwgordon7 commentedHere'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?
Comment #39
Bojhan CreditAttribution: Bojhan commentedI 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?
Comment #40
seutje CreditAttribution: seutje commentedpatch seems to have went stale, rerolled for current head
seems to work as expected
Comment #41
seutje CreditAttribution: seutje commentedtest 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
Comment #42
seutje CreditAttribution: seutje commentedturns out IE6 didn't like this for some reason:
so I changed it into this:
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)
Comment #43
seutje CreditAttribution: seutje commentedforgot status
Comment #44
seutje CreditAttribution: seutje commenteddoh, "class" is reserved
changed it back to
Comment #45
Bojhan CreditAttribution: Bojhan commentedStill not working, SVN repository that is.
Comment #46
Bojhan CreditAttribution: Bojhan commented-
Comment #47
Bojhan CreditAttribution: Bojhan commentedMy first look at this.
Obviously, we won't have floating icons ass the final design.
Comment #48
Gábor Hojtsy@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).
Comment #49
stella CreditAttribution: stella commentedThe 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.
Comment #50
Bojhan CreditAttribution: Bojhan commented1. 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.
Comment #51
webchickThis 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:
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:
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.
Comment #52
Dries CreditAttribution: Dries commented- @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.
Comment #53
sunThis 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.
I don't like the term "actions". These are "admin links", and that we should name them.
This change to use drupal_render() makes little sense, because only the top-level uses a drupal_render-style array.
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.
- $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?
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.
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.
Comment #54
Damien Tournoud CreditAttribution: Damien Tournoud commentedI do agree with most of what sun is saying:
I do disagree on one point:
I do believe that the session is perfectly right to do that.
Comment #55
sun@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?
Comment #56
catchI 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.
Comment #57
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedNice 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...
Comment #58
Everett Zufelt CreditAttribution: Everett Zufelt commentedWondering 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?
Comment #59
Dries CreditAttribution: Dries commentedThis patch is holding up (part of) #376103: Make /admin into a REAL dashboard..
Comment #60
cwgordon7 CreditAttribution: cwgordon7 commentedFirst 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. :)
Comment #61
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #62
mcrittenden CreditAttribution: mcrittenden commentedEverett 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.
Comment #63
Dries CreditAttribution: Dries commentedLast patch fails to apply for me. Asking the test-bot to test it again, but we might need a reroll. Call 911!
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedGreat 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.
Comment #66
seutje CreditAttribution: seutje commented@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
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
Comment #67
seutje CreditAttribution: seutje commentedforgot status again
Comment #68
sunThese 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.
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.
I'm also not sure how this change relates to this patch.
Please re-roll with diff -upN
The second foreach is superfluous, can be merged into the first.
3rd + 4th time we iterate over the very same array...
Please declare this once upfront, not multiple times for every not passing condition.
Use @todo here. Following lines should be indented with 2 spaces, to make clear which other lines of this comment belong to the @todo.
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.
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.
There are obsolete now, no?
11 days to code freeze. Better review yourself.
Comment #70
sign CreditAttribution: sign commentedRerolled to work with head and renamed actions to admin_links
Comment #71
yoroy CreditAttribution: yoroy commentedbot?
Comment #72
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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 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.
Comment #73
xmacinfoAfter code freeze I'll try my hand at creating better looking admin links icons.
Awesome!
Comment #74
David_Rothstein CreditAttribution: David_Rothstein commentedHere are some comments on the current state of the patch:
+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?
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).
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?$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.)Comment #75
Gábor Hojtsy@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.
Comment #77
lilou CreditAttribution: lilou commentedHEAD is broken.
Comment #78
mgiffordI 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).
Comment #79
int CreditAttribution: int commentedadd tag
Comment #80
Everett Zufelt CreditAttribution: Everett Zufelt commentedI can confirm the error @mgifford is getting in comment #78 on clean head.
Comment #81
Everett Zufelt CreditAttribution: Everett Zufelt commentedForgot to set to needs work in comment #80
Comment #82
Manuel Garcia CreditAttribution: Manuel Garcia commentedResults from applying the patch:
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.
Comment #83
my-family CreditAttribution: my-family commentedFirst, 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)
Comment #84
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK, 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:
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.
Comment #85
sign CreditAttribution: sign commentedre-rolled patch,
fixed the error above (
Notice: Undefined index: #block in menu_page_alter() (line 317 of modules/menu/menu.module).
)Comment #86
Everett Zufelt CreditAttribution: Everett Zufelt commentedApplied 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?
Comment #87
sign CreditAttribution: sign commentedAdjusted 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
Comment #88
xmacinfo@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.
Comment #89
Everett Zufelt CreditAttribution: Everett Zufelt commented@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?
Comment #90
Gábor HojtsyHere 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.
Comment #91
mgiffordI 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.
Comment #92
Cliff CreditAttribution: Cliff commentedSubscribe
Comment #93
seutje CreditAttribution: seutje commented@Gábor: You forgot admin_links.css and admin_links.js
didn't review yet
Comment #94
seutje CreditAttribution: seutje commentedre-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)
Comment #95
heather CreditAttribution: heather commentedTo 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?
Comment #96
Nick Lewis CreditAttribution: Nick Lewis commentedI 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.
Comment #97
Gábor HojtsyRecent 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).
Comment #99
Gábor HojtsyGosh, 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.
Comment #100
Dries CreditAttribution: Dries commentedLooking 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.
Comment #101
markboulton CreditAttribution: markboulton commentedI 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.
Comment #102
Gábor HojtsyAll 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:
Comment #103
David_Rothstein CreditAttribution: David_Rothstein commented1. 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.
Comment #104
yoroy CreditAttribution: yoroy commentedcreated #585776: Make 'edit mode' toggle available even when toolbar module is disabled. Will try and test this tonight.
Comment #105
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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 :)
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.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.
Comment #106
Gábor HojtsyI'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.
Comment #107
int CreditAttribution: int commentedgo bot
Comment #109
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.Comment #110
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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.
Comment #112
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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.
Comment #113
Gábor HojtsyLooks 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
Comment #114
Gábor HojtsyHere is the actual patch, see my comments above.
Comment #115
sunWe 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).
This should adhere to CSSDoc standard -- basically following the same rules as PHPDoc.
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.
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.
Comment #117
Dries CreditAttribution: Dries commentedTalked 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).
Comment #118
webchickDarn. I tried to take this for a spin but it no longer applies. :(
Comment #119
JacobSingh CreditAttribution: JacobSingh commentedIt is pre-theme functions losing their signatures. I imagine that is why.
Comment #120
sun(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.
Comment #122
sunComment #123
David_Rothstein CreditAttribution: David_Rothstein commented@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!
Comment #124
David_Rothstein CreditAttribution: David_Rothstein commentedOops, crosspost... looks like you already started :)
Comment #126
sunPass again.
Comment #127
sunShort 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.
Comment #128
sunNow 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. ;)
Comment #130
sunDon't be scared by this sucker. Also contains the patch from #599706: Allow to alter local tasks/actions, so we can move forward here.
Comment #132
sunBack to business.
Comment #133
sunIn progress, need a quick break.
Comment #135
Dries CreditAttribution: Dries commentedSome 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.
Comment #136
Dries CreditAttribution: Dries commented1) 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 thinkmenu_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.
Comment #137
David_Rothstein CreditAttribution: David_Rothstein commentedSure it did.
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.
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.
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...
Comment #138
David_Rothstein CreditAttribution: David_Rothstein commented@Dries:
Makes sense to me.
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....
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.
Comment #139
sunWhat we need to understand:
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 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.
Comment #140
Dries CreditAttribution: Dries commented@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:
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?
Comment #141
Dries CreditAttribution: Dries commentedThe 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:
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.
Comment #142
Dries CreditAttribution: Dries commentedBy 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.
Comment #143
Dries CreditAttribution: Dries commentedAs 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!
Comment #144
David_Rothstein CreditAttribution: David_Rothstein commented@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:
.... 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.
Comment #145
sunThanks, 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:
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.
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.
Comment #146
Gábor Hojtsy@Dries:
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).
Comment #147
Dries CreditAttribution: Dries commented- @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.
Comment #148
David_Rothstein CreditAttribution: David_Rothstein commentedI'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:
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.)
Comment #149
Noyz CreditAttribution: Noyz commentedThere'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
Comment #150
xmacinfoI 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.
Comment #151
Gábor Hojtsy@xmacinfo: that is why it was opened as a separate issue.
Comment #152
David_Rothstein CreditAttribution: David_Rothstein commentedSince 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:
Not done yet or remaining issues:
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.
Comment #153
Gábor HojtsyOk, 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.
Comment #154
Gábor HojtsyBy 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.
Comment #155
Gábor HojtsyNow 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.
Comment #156
Dries CreditAttribution: Dries commentedYes, we should have those icons on the dashboard too! Very cool, and very much inline with what other dashboard do.
Comment #157
dmitrig01 CreditAttribution: dmitrig01 commentedMostly minor nitpicks - I think that given the timeframe, it's more productive to get this in and then put edit links on the dashboard.
It would make more sense to me to highlight a region on mouseover of the region, not on mouseover of the link.
Can't you just inliine addHighlight and removeHighlihgt?
Isn't there a better way to figure out what has configuration and what doesn't?
Wouldn't it be more logical to call this "use edit more"?
This review is powered by Dreditor.
Comment #158
David_Rothstein CreditAttribution: David_Rothstein commentedNew 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.
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.
Sorry, I'm JavaScript-impaired enough that I'm not sure exactly what you're suggesting there :)
5.
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.
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.
Comment #159
David_Rothstein CreditAttribution: David_Rothstein commentedTried 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:
Comment #160
mcrittenden CreditAttribution: mcrittenden commented@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.
Comment #161
David_Rothstein CreditAttribution: David_Rothstein commentedAh, 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.
Comment #163
David_Rothstein CreditAttribution: David_Rothstein commentedRerolled 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.
Comment #164
yoroy CreditAttribution: yoroy commentedPlayed 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
Comment #165
mcrittenden CreditAttribution: mcrittenden commentedA 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!).
Can we add plain old border-radius here too? D7 will be around for quite awhile, long after browsers start supporting that property.
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.
Very nitpicky: could use proper English here.
"Transform" doesn't really tell me anything.
I'm on crack. Are you, too?
Comment #166
yoroy CreditAttribution: yoroy commentedBest spell out the suggested the proper English for the non-natives then please.
Comment #167
sunSo, 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. ;)
Comment #169
catchDavid's patch is still CNR for now.
Comment #170
sunQuick 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.
Comment #172
sunThis one should at least come back green, or skyrocket to mars.
Comment #174
sun8 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.
Comment #175
sunLast patch with those proof of concept changes.
Comment #176
sunMore clean-ups, removed proof-of-concept code.
Comment #177
sunA bit more less.
Comment #178
sunI 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
Comment #179
Dries CreditAttribution: Dries commentedI 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.
Comment #180
Dries CreditAttribution: Dries commentedTalked 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.
Comment #181
sun- 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 ;).
Comment #183
sun- 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.
Comment #184
sunoops, copy/paste error.
Comment #185
effulgentsia CreditAttribution: effulgentsia commentedsubscribing
Comment #186
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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
We'll need a new menu router database column with three states.@sun added this while I was reviewing :)@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:
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:
So this feature would be more broadly useful than just for this patch - which is good!
Other things (can be for followups, etc)
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.)
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.
Seems like this can just go in a simple wrapper function?
Boy, though, this really makes me wish my comment #13 had gotten some more support, when I wrote: We maybe could have been getting to the stuff we're doing now in July rather than October :)
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 :)
Comment #187
sunAnyone with a free slot available, please help over in #606608: Use proper menu router paths for comment/* - thanks!
Comment #188
sunAnd, 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)
Comment #189
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #190
sun- 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).
Comment #191
webchickCode review:
Can we use node/%node/revisions as the example here, since that comes from core?
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?
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()?
Need documentation for this hook.
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.
(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.
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.
Comment #192
sunSo let's talk about performance. Go effulgentsia, go! :-D
Comment #194
sunWe love performance.
Comment #195
sunDebugging the menu system.
Comment #197
sunNew patch for performance benchmarking. Menu system issues are resolved (well, workaround'ed).
Comment #198
sunAnd. 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)
Comment #199
sunTwo minor fixes after in-depth review.
Thank you again, webchick! Awesome review! :)
Comment #200
webchickI 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:
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!
Comment #201
effulgentsia CreditAttribution: effulgentsia commentedHere'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.
Comment #202
effulgentsia CreditAttribution: effulgentsia commentedstatus change in #201 was unintentional
Comment #203
sunSorry for forgetting that.
Please back to work after committing.
Comment #204
sunoh wow, sometimes, it doesn't hurt to read ...
Comment #205
catch#204 is good.
Comment #206
Dries CreditAttribution: Dries commentedCommitted the follow-up patch.
Comment #207
chx CreditAttribution: chx commented6% slowdown is not bad???? Anything goes if it's usability??
Comment #208
sunCreated 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.
Comment #209
JacobSingh CreditAttribution: JacobSingh commentedWhat 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
Comment #210
effulgentsia CreditAttribution: effulgentsia commentedJacob: 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.
Comment #211
effulgentsia CreditAttribution: effulgentsia commentedDamn it! I am so frustrated by Firefox retaining the state of "select"s when a page is refreshed!!! Setting back to NW.
Comment #212
jrabeemer CreditAttribution: jrabeemer commentedefful: that seems like a JS post-load form reset on selects.
Comment #213
Gábor Hojtsy@JacobSingh, @sun, @chx: that comment actually talks about a 6% performance decrease **for anonymous**, which if reproducible is a major problem.
Comment #214
eigentor CreditAttribution: eigentor commenteder, 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...
Comment #215
effulgentsia CreditAttribution: effulgentsia commentedIt'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.
Comment #216
catch#611642: Roll back comment contextual links (temporarily)
Comment #217
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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.
Comment #218
David_Rothstein CreditAttribution: David_Rothstein commentedThere'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 :)
Comment #219
David_Rothstein CreditAttribution: David_Rothstein commentedOne more thing :)
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.
Comment #220
David_Rothstein CreditAttribution: David_Rothstein commented[accidental double post]
Comment #221
te-brian CreditAttribution: te-brian commentedProbably 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.
Comment #222
David_Rothstein CreditAttribution: David_Rothstein commentedRe #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.
Comment #223
Bojhan CreditAttribution: Bojhan commented@David - the follow up is turning them into blocks :) To be honest, heh
Comment #224
te-brian CreditAttribution: te-brian commentedRe #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.
Comment #225
mgiffordMissed 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:
Comment #226
David_Rothstein CreditAttribution: David_Rothstein commentedWell, "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.
Comment #227
bowersox CreditAttribution: bowersox commented+1 to the suggestion of saying "Configure this block" and putting that link immediately following the block title h2 heading.
Comment #228
joachim CreditAttribution: joachim commentedPlease 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?
Comment #229
mcrittenden CreditAttribution: mcrittenden commented@joachim: for discussion on the icon, see #601150: Improved UI for contextual links.
Comment #230
Cliff CreditAttribution: Cliff commented@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?
Comment #232
sunI'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=...
Comment #233
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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).
Comment #235
JayKayAu CreditAttribution: JayKayAu commentedHi 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!
Comment #236
dman CreditAttribution: dman commented@JayKayAu Short answer, for D6, try http://drupal.org/node/473268#comment-1645792
Comment #237
JayKayAu CreditAttribution: JayKayAu commentedFantastic, thank you!
Comment #238
rfayTiny followup: #765830: E_NOTICE if block is themed without an argument