Modules like Contextual links and Shortcut cannot add to "template regions"

+++ 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.

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

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

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.

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.

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.

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.

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.

+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.

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.

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.

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.

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).

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?

Did this create the issue at #673984: Notice: Undefined variable: contextual_links...?

sorry for the noise; please disregard #33.

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().

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?

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.

Committed to CVS HEAD.

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. :(

Docs still needed as per #29

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 :)