As identified in #626286-48: Make contextual links a module, templates need to contain awkward code like

-<?php if ($contextual_links): ?>
+<?php if (!empty($contextual_links)): ?>

for modules like Contextual links and others that want to output something at the beginning or end of a template.

Files: 
CommentFileSizeAuthor
#35 646874-followup-35.patch987 bytesDavid_Rothstein
Passed on all environments.
[ View ]
#24 drupal.template-title-prefix-suffix.24.patch18.14 KBsun
Passed on all environments.
[ View ]
#21 drupal.template-title-prefix-suffix.21.patch18.31 KBsun
Unable to apply patch drupal.template-title-prefix-suffix.21.patch
[ View ]
#19 shortcuts-garland.png59.49 KBDavid_Rothstein
#19 drupal.template-title-prefix-suffix-19.patch18.31 KBDavid_Rothstein
Passed on all environments.
[ View ]
#18 drupal.template-title-prefix-suffix.18.patch17.4 KBsun
Passed on all environments.
[ View ]
#15 drupal.template-title-prefix-suffix.15.patch17.38 KBsun
Passed on all environments.
[ View ]
#12 title-prefix-suffix-646874-12.patch17.25 KBDavid_Rothstein
Passed on all environments.
[ View ]
drupal.template-prefix-suffix.0.patch11.27 KBsun
Passed on all environments.
[ View ]

Comments

sun’s picture

Status:Active» Needs review
dawehner’s picture

+++ modules/block/block.tpl.php 30 Nov 2009 17:05:10 -0000
@@ -37,11 +40,8 @@
<?php print render($prefix); ?>

Any reason, why not check for content in $prefix/$suffix? I think it would save some calls to render()

+++ modules/block/block.tpl.php 30 Nov 2009 17:05:10 -0000
@@ -37,11 +40,8 @@
<?php print render($prefix); ?>

Any reason, why not check for content in prefix/suffix? This saves a call to render

This review is powered by Dreditor.

sun’s picture

heh. Sure. :) But let's agree on the design first, before talking about performance.

dawehner’s picture

There could be possibilty to use theme() and support also theme functions. But i don't like this idea much.

sun’s picture

Well, those template variables go through the regular template rendering process. So you have all kind of entry points where you can hook into.

dawehner’s picture

I just wondered why not to support theme functions, for example theme_table, without change the theme function itself

sun’s picture

Could you please clarify, because I have troubles to follow your consideration (or concerns?) here. Or if you already dropped your idea, then also please state so.

You can use #theme like in any other renderable array:

<?php
$variables
['prefix']['somemodule']['#theme'] = 'mymodule_foo';
?>
moshe weitzman’s picture

I'm having a little trouble reconcilining the proposed $prefix and $suffix in preprocess with #prefix and #suffix in render arrays. When to use each? I agree that the code that this replaces is a bit awkward.

sun’s picture

Similar thoughts crossed my mind when I wrote that patch. It's not only the name and when to use which, #prefix and #suffix also work entirely different - they add content entirely before or after something.

Reminds me of jQuery. #prefix and #suffix is basically .before() and .after().

So this would be the equivalent of .prepend() and .append().

Hence?

$variables['prepend'] = array();
$variables['append'] = array();
David_Rothstein’s picture

This is a great patch. But yeah, it's not really clear that "prefix" and "suffix" (or even "prepend" and "append") are what we want to call them, because I'm not sure we have a use case where the top of the template is actually where we want to put them:

1. Contextual links are at the top now, but for accessibility reasons we need to move them underneath the <h2> (see discussions at the end of #473268: D7UX: Put edit links on everything).

2. The $add_or_remove_shortcut template variable (see http://api.drupal.org/api/drupal/themes--seven--page.tpl.php/7/source) is another really ugly one that we should get rid of via this method, and that already appears after the <h1>.

So our immediate need here is actually for a variable more like $post_heading or something like that.

sun’s picture

So this means $title_prefix and $title_suffix ?

David_Rothstein’s picture

Title:Modules like Contextual links cannot add to "template regions"» Modules like Contextual links and Shortcut cannot add to "template regions"
StatusFileSize
new17.25 KB
Passed on all environments.
[ View ]

Sounds about right.

So here's a stab at doing it that way. The patch now removes twice as much ugly code as the previous patch did, plus solves the contextual links accessibility problem as well. Good deal :)

A side effect of this patch is that the "add to shortcuts" link now appears by default in all themes, rather than just Seven like it did before. However, that makes absolute sense to me (and was even a feature that people requested during the shortcut implementation).... I'm not sure I understand the rationale for why it was intentionally put only in Seven.

I did not do any theme adjustments to the "add to shortcuts" link, though, so it looks really messed up in Garland at the moment - that part would still need work.

David_Rothstein’s picture

Hm. Although. All our examples in core attach contextual links to things that have "titles", but there may be cases where it would be attached to something that does not.

So we may want to continue massaging these variable names a bit more.

sun’s picture

+++ includes/theme.inc 3 Dec 2009 04:13:00 -0000
@@ -2246,6 +2246,10 @@ function template_preprocess(&$variables
   $variables['title_attributes_array'] = array();
   $variables['content_attributes_array'] = array();

+  // Initialize 'title_prefix' and 'title_suffix' renderable arrays.
+  $variables['title_prefix'] = array();
+  $variables['title_suffix'] = array();

Well, as can be seen from diff context, we also setup title and content "attribute arrays" for all templates, regardless of whether they may have a title or not. So I think this is fine.

+++ modules/block/block.tpl.php 3 Dec 2009 04:13:00 -0000
@@ -37,13 +42,11 @@
<?php if ($block->subject): ?>
<?php print render($title_prefix); ?>
   <h2<?php print $title_attributes; ?>><?php print $block->subject ?></h2>
<?php print render($title_suffix); ?>
<?php endif;?>

(and elsewhere) oh, we need to move prefix and suffix outside of this condition - otherwise, blocks without titles won't have contextual links.

I'm on crack. Are you, too?

sun’s picture

StatusFileSize
new17.38 KB
Passed on all environments.
[ View ]

Fixed that.

Ready to fly?

David_Rothstein’s picture

Re #14, yeah, functionality-wise it's fine as is, I'm just a bit worried that the names might be confusing. For example, they confused me enough that I introduced the bug that you found :) On the other hand, I don't have a better suggestion at the moment.

It's mostly ready to go, but the one issue is that the "add to shortcuts" link theming is totally messed up in Garland. Might be a bit of a pain to fix correctly, especially with the crazy things the overlay does to style that link - but in the end it would be a good cleanup.

Possibly we can just turn the link off in Garland for now to get this patch in more quickly (after all, it doesn't display at all in Garland before this patch either) and then work on cleaning that up as a followup.

sun’s picture

+1 on that last suggestion :) Do you happen to know how? I know little about shortcuts/toolbar, just love to bash on them. :-D

sun’s picture

StatusFileSize
new17.4 KB
Passed on all environments.
[ View ]

Re-rolled against HEAD.

David_Rothstein’s picture

StatusFileSize
new18.31 KB
Passed on all environments.
[ View ]
new59.49 KB

+1 on that last suggestion :) Do you happen to know how? I know little about shortcuts/toolbar, just love to bash on them. :-D

Ah :)

Well, I actually was just thinking of a theme preprocess function in Garland that simply removed the link, for now. The attached screenshot shows what it currently looks like in Garland (and hovering on the plus sign causes the flyout text to push the local tasks to the right, ech).

The screenshot also shows a bug in which the contextual links show up even on the full-page node view - this is because there were node.module changes in the original patch that I removed but now need to come back due to the fix in #15.

The attached patch puts those back (no other changes), although I put it back in a slightly different way. I did this:

-  // Add contextual links for this node.
+  // Add contextual links for this node, when it's not being displayed on its
+  // own page.
   // @todo Make this configurable per build mode.
-  $build['#contextual_links']['node'] = array('node', array($node->nid));
+  if (!menu_get_object('node')) {
+    $build['#contextual_links']['node'] = array('node', array($node->nid));
+  }

as opposed to this from the original patch:

   // Add contextual links for this node.
   // @todo Make this configurable per build mode.
-  $build['#contextual_links']['node'] = array('node', array($node->nid));
+  if ($build_mode == 'teaser') {
+    $build['#contextual_links']['node'] = array('node', array($node->nid));
+  }

The reasoning being that especially if we don't have configurable build modes, hardcoding it to only a single one might not be the best default behavior? Not positive about this though.

sun’s picture

+++ modules/node/node.module 9 Dec 2009 04:29:55 -0000
@@ -1183,9 +1183,12 @@ function node_build($node, $build_mode =
-  // Add contextual links for this node.
+  // Add contextual links for this node, when it's not being displayed on its
+  // own page.
   // @todo Make this configurable per build mode.
-  $build['#contextual_links']['node'] = array('node', array($node->nid));
+  if (!menu_get_object('node')) {
+    $build['#contextual_links']['node'] = array('node', array($node->nid));
+  }

We need to use the $build_mode as condition, as I did in my earlier patch. See #445902: (bool) menu_get_object() is *not* an equivalent of $page.

Just because the current path may contain 'node/%', this doesn't mean we are rendering a node in full build mode (page view).

This review is powered by Dreditor.

sun’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new18.31 KB
Unable to apply patch drupal.template-title-prefix-suffix.21.patch
[ View ]

Fixed that. Since that is the only change, I'm RTBC'ing this patch.

Status:Reviewed & tested by the community» Needs review
Issue tags:-Contextual links

Re-test of drupal.template-title-prefix-suffix.21.patch from comment #21 was requested by webchick.

Status:Needs review» Needs work
Issue tags:+Contextual links

The last submitted patch, drupal.template-title-prefix-suffix.21.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new18.14 KB
Passed on all environments.
[ View ]

Re-rolled against HEAD.

sun’s picture

Status:Needs review» Reviewed & tested by the community
webchick’s picture

Status:Reviewed & tested by the community» Needs review

Hm. I too found $title_prefix and $title_suffix confusing. My initial assumption was that they were talking about <title>, which of course wouldn't make sense. Then again, I'm not a themer, and it's possible that since these variables are all called $title, themers would immediately make the connection. I asked around in #drupal for better name suggestions but nothing came up...

OTOH, I can see why this patch is needed; we're hard-coding assumptions about an optional module into all of our templates, and contrib doesn't have this luxury. It does feel pretty arbitrary though to allow this prepend/append stuff here and not on other variables, such as $content.

There's no possible way we could make render API support a syntax like:

$variables['#block']['title']['#prefix'] = 'before title';
$variables['#block']['title']['#suffix'] = 'after title';

...is there? That would at least be consistent with what other places in the render API do, and not special-case title in any way.

sun’s picture

The problem with

$variables['#block']['title']['#prefix'] = 'before title';
$variables['#block']['title']['#suffix'] = 'after title';

is that it's only going to be rendered if there is a

$variables['#block']['title']

at all. In the case of contextual links, we basically want to output something in the "title region" (regardless of template), before or after the "title region" content, and regardless of whether there is any content in that "title region" content.

All of this is food for thought for D8. For D7, we can only go with this approach, and just do what's required to fulfill the use-case we have. I think we need to see this action and work with it for a couple of months/years to come up with an idea of how this could work out in a more general sense.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

I read this over again and I can't think of anything better. I too can't think of a name better than #title_prefix and such. English has no words for our clever tricks :)

This does clean up shortcut_page_build() which puts in a nasty render array at the top of $page which is not a region. Kudos for ditching that.

RTBC.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

Yeah, agreed that there isn't really another suitable workaround. I guess in D8 we can work on making this kind of wrapping more generic.

Committed to HEAD. These new variables need to be documented in the theme upgrade guide.

David_Rothstein’s picture

A little late coming back to this, but I was going to suggest that a single $title_supplement (or similar) variable might be more clear?

That's because in retrospect, I am not sure we have much of a use case for "prefix". Themes that want separate prefixes and suffixes could always do print render($title_supplement['some element']) to pull out the part they want to display separately. Ultimately it's the theme's job to decide what content gets placed where, more than it is the module's job to try to hint at the order the HTML should be placed in.

****

We also should use the new node_is_page() API function here (which did not exist when this patch was originally written) for the contextual links visibility.

And do something to fix or remove the "add to shortcuts" button in Garland, although that can be a separate bug report (I'm thinking it needs to be a theme setting, which for now is only implemented by Seven and ignored entirely by the other core themes).

sun’s picture

Hm - we added the additional variables, because modules have no chance to output something below a certain template variable, but connected to that template variable. In this case, this is the $title variable in all templates. And Contextual module wants to specifically output something below the title, so that screen-readers have a context and can figure out to which item on the page the additional output (here: links) belongs. This was discussed in the original contextual links issue (or in IRC?) and it was specifically mentioned that the links should go below the title in the output to provide the context.

This means that there actually is an expected regional location for the variable in the template. In the same way, another module might need/want to output something explicitly before the title, or even wrap the title. Therefore, the most logical way forward was to introduce $title_prefix and $title_suffix variables.

All of this can likely be seen as a step towards implementing something like Node Displays into core for D8. For now, and this late in the D7 cycle, we need to go with a limited approach, live with it, and most importantly, think about how we can solve this more intelligently in D8 or beyond.

Please also note that we implemented a couple of similar new variables for full RDF support in core. Now that I recall this, it might even be possible to a few of those into $title_prefix or $title_suffix. For some tags, RDF module basically has the same requirement; it needs to output something in between template variables, but not within the regular ones. We may want to ping scor and effulgentsia about that.

While it's true that it's the theme's job to control what goes where, we still require all themes to support some kind of template regions. After all, if you entirely remove the structural markup from a template, then none of a module's default CSS styles and JavaScript will work. The same is true for contextual links - if you remove the surrounding markup, the links won't appear. The reality is that we expect and require themes to output certain things - if the theme wants to support default styles and behaviors that come with all modules.

Regarding node_is_page(): No, we already discussed that, reasoning in #20.

The only thing I agree with is the "add shortcuts" bugfix for Garland, but that should be moved into a separate issue, of course.

moshe weitzman’s picture

A single $title_supplement variable is not really that much clearer than what we have now.

Whatever we do for add_remove_shortcuts, lets please not put another non-region render array at top level of $page. On that note, why do we have empty elements at top level of every $page called dashboard_main and dashboard_sidebar?

aaron’s picture

aaron’s picture

sorry for the noise; please disregard #33.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new987 bytes
Passed on all environments.
[ View ]

Regarding node_is_page(): No, we already discussed that, reasoning in #20.

#20 is out of date; it assumed that all node page checks would be removed from core in favor of view modes. But what happened instead was that they were left in place.

The reason we shouldn't use the 'full' view mode here is that it assumes a particular use case for that mode which will not always be valid. For example, if someone uses the Views module to create a listing of full nodes, the default behavior should still be to display contextual links with them. The fact that they aren't is a regression introduced by the code that was committed here.

The attached patch switches to node_is_page().

David_Rothstein’s picture

Contextual module wants to specifically output something below the title, so that screen-readers have a context and can figure out to which item on the page the additional output (here: links) belongs. This was discussed in the original contextual links issue (or in IRC?) and it was specifically mentioned that the links should go below the title in the output to provide the context.

Yup, but the same is true of almost any item - for example, it applies to the "add to shortcuts" link as well, even though that is not strictly speaking a contextual link. That's why I think the use cases for $title_prefix are probably very limited; everything I can think of belongs in $title_suffix.

Also, it's not even limited to these variables. For proper accessibility, I would venture to say that the general rule is that themes should always put the title at the top of the template (for example, I suppose the "submitted by..." text on nodes would also be bad for accessibility if it is placed before the title). So this is a general decision that only the theme layer can make.

So, is there a legitimate use case for $title_prefix that I'm not thinking of, or not?

A single $title_supplement variable is not really that much clearer than what we have now.

The thing I am having trouble with is that $title_prefix and $title_suffix need to be output even when the title is not. The idea that something that isn't there still has a prefix and suffix is blowing my mind a bit :)

I agree $title_supplement ain't perfect either, but I can much more easily get my head around the idea that you need to supplement something because of its absence, than I can the above.

Anyone else?

David_Rothstein’s picture

The only thing I agree with is the "add shortcuts" bugfix for Garland, but that should be moved into a separate issue, of course.

I created #674394: The add/remove shortcut buttons look bad and don't appear in most themes besides Seven.

On that note, why do we have empty elements at top level of every $page called dashboard_main and dashboard_sidebar?

And that one should definitely be moved into a separate issue :)

I tracked it down and filed #674406: Block module adds empty theme regions to the $page array with what is hopefully a trivial patch.

sun’s picture

Status:Needs review» Reviewed & tested by the community
Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD.

webchick’s picture

If someone from this issue could please help out with the follow-up bug this created at #674394: The add/remove shortcut buttons look bad and don't appear in most themes besides Seven in the next day or two, I'd really appreciate it. It's totally marring first-time user experience, and we have an alpha to roll on Friday. :(

effulgentsia’s picture

Status:Fixed» Needs work

Docs still needed as per #29

David_Rothstein’s picture

Status:Needs work» Fixed
Issue tags:-Needs Documentation

Documented in the theme upgrade guide here:
http://drupal.org/update/theme/6/7#title-prefix-suffix

Didn't document it in the module upgrade guide - I could sort of imagine doing that too, but it's probably overkill, and that guide is getting gigantic as it is :)

Status:Fixed» Closed (fixed)
Issue tags:-Contextual links

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