Our regions aren't identified by predictable id attributes. In the core themes we have leftover ids like "sidebar-left". Other regions - e.g., 'header' - have no id attributes.

This presents theming issues and also barriers to dynamic content rendering; there is no way to dynamically assign content to a given region.

We should ensure that all regions have an enclosing element with an id like 'region-left', 'region-content', etc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nedjo’s picture

Another big reason to add these ids is theming. With them, we could e.g. theme menus differently in each region. For example, menus in a footer region could be themed horizontally.

LAsan’s picture

Version: x.y.z » 7.x-dev

Does this feature request still applies to cvs?

messenger’s picture

+1 for this, wherever it might fit. Horizontal theming of blocks for specific regions would be great.

JohnAlbin’s picture

I might have a solution for this. Let me discuss with EclipseGC in IRC.

JohnAlbin’s picture

Ok. So the idea EclipseGC had at Drupalcon was to create a region-wrapper.tpl.php.

The advantages of a having a wrapper tpl are that you don't have to tell all themes to include a <div id="region-NAME"> in their page.tpl and you could ensure that the IDs are the same across all themes.

Also, we'd be able to simply the HTML in the new modules/system/page.tpl.php.

JohnAlbin’s picture

Title: Include id attributes for all regions in themes » Add region-wrapper.tpl with id and class attributes for all regions in themes
Issue tags: +theme, +more theme features, +theming, +tpl-refresh
EclipseGc’s picture

So, to get in on this and make some comments:

JohnAlbin, jjeff and myself discussed this @ DCDC for a bit. The effort to improve the default page.tpl.php ultimately resulted in a div with a class="region" directly around any print $region statements. I believe all of these have an id associated with them as well. When I had suggested the class="region" I just felt it could be useful, I didn't have a usecase at the time, but jjeff pointed out we could potentially use that for drag-n-drop block/etc placement. Whatever the case, that mention got us to thinking that we should just encapsulate all regions in a default wrapper that forced a class="region" onto the blocks. This would also result in consistent id naming structures which would be a big ++ all around.

My one "cringe" in all of this is the whole "htmlzero" theme push to get a contrib theme that outputs markup w/ no ids or classes. We just need to be sensitive to that as we proceed. I don't foresee it being a problem, it's just an additional layer of markup obfuscation is all.

Eclipse

emmajane’s picture

Summarizing what I said to EclipseGC in IRC:
I have to admit that another layer of HTML wrappers sort of makes me cry inside. I wouldn't say that I'm part of the htmlzero camp, but I am in favour of only have markup where it's needed. What I am a little bit excited about though is this: having a very heavily documented, but empty, region wrapper available in core. This would be a themer toolkit/module developer win because you could use it if you wanted to; but also a keepyourcrapoutofcore^H^H htmlzero win because nothing extra would be added by default.

Plausible? or too fence-sitter-ish?

JohnAlbin’s picture

Actually, if you look at the page.tpl.php, all of the div wrappers for the regions would be removed. To be replaced by the div in this new region tpl. So there would be no net increase/decrease of divs in the markup.

EXCEPT, the left and right regions have a problematic "section" class in their "region" wrapper divs. I'm not sure how to prevent 3 wrapper divs around the left and right regions. :-\

eaton’s picture

Eclipse's concern about the bloating this could cause is valid, but that would only be the case if the region added its markup AND the page added additional markup. This is about ways to replace the page-level markup with region-specific markup.

John, It sounds like this has some great potential to streamline certain theme related issues; if our regions are consistently marked up, is there any way that a #classes or #attributes property in the region could be used to populate the css classes, allowing the templates to remain the same while certain regions get automatic classes via preprocess hooks? That might not be a useful approach, but I'd really love to see some of it happen...

In the future, it might also provide ways for modules to insert their own custom regions into the page; nothing would prevent a module like Panels from building the $content region out of multiple sub-elements of #type=region...

JohnAlbin’s picture

In #422738: Improve consistency in base region IDs (a dupe of this issue.), Gábor has a good analysis of the classes that would be cleaner if we added this feature.

Gábor Hojtsy’s picture

What about having a patch here, so we have something concrete to talk about? I'd think it would be a shame if my issue with a usable patch was marked dupe on an issue without a useful patch and this change would never get into Drupal 7.

dvessel’s picture

We already have a region wrapper. theme_blocks! Convert that into a tpl.php after we manage to get in #306358: Add $classes to templates with new theme_process_hook functions, the region class would output as "blocks" and you can setup an ID attribute specifically for it in a preprocess function.

JohnAlbin’s picture

Status: Active » Needs review
FileSize
7.29 KB

theme_blocks() has been removed from D7, so here's a patch that use HOOK_alter_page(), etc to implement it.

Sorry it took so long to get a patch for this, Gábor. :-)

JohnAlbin’s picture

woot! tests pass!

I should point out that my patch added a special case to drupal_render() so that it doesn't render an empty region.tpl. We should probably generalize that idea so that some theme wrappers only render if there are children, but I couldn't think of a good general purpose flag for that. #dont_theme_wrap_if_children_empty? :-p #print_if_children? Or instead of #theme_wrapper, a new #children_theme_wrapper?

Gábor Hojtsy’s picture

Well, this patch pushes for regions to have unique classes and they would not have IDs to identify them anymore, right? It looks so from the patch.

JohnAlbin’s picture

Title: Add region-wrapper.tpl with id and class attributes for all regions in themes » Add region-wrapper.tpl for all regions in themes
Issue tags: -theming

@Gábor, yep. We could easily have IDs, but I've seen a lot of IDs getting converted to classes, so I went that way. I'm open to debate.

Fixing issue title so it doesn't mention IDs.

eaton’s picture

The lack of universally available identifying snippets to call out regions was one of the reasons that we weren't able to make the Buzzr-style 'drag blocks into regions' code work with anything other than Zen-based themes.

Of course, it moves us yet farther away from the ideal of a page template 'wrapping' pre-rendered content -- if people continued to do THAT instead of coding their CSS against the region classes generated by this template, we'd see even more bloat. Is that bad? Acceptable? It's the only way to make "features" like drag and drop work reliably across themes, IMO. Any designers willing to weigh in on that tradeoff? I know that I'd love it.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
6.43 KB

Updated patch.

nedjo’s picture

Looks basically good.


+  $variables['blocks'] = $variables['elements']['#children'];

Better to call this variable e.g. 'content' rather than 'blocks', since it can contain non-block content that has been assigned to a region.

Presumably we need to edit existing core themes' templates in the same way as modules/system/page.tpl.php ?

JohnAlbin’s picture

FileSize
8.98 KB

Thanks for the review, Nedjo! :-)

The new patch changes the variable to $content from $blocks.

Stark has no tpls. I looked through the Seven theme's page.tpl.php and there weren't any region wrappers to simplify. But Garland’s header region could be simplified. So I've added that to this new patch as well.

JohnAlbin’s picture

Issue tags: +Needs design review

Adding "needs design review" tag per eaton's request.

JohnAlbin’s picture

FileSize
9.71 KB

Whoops. Missed a necessary clearfix class on Garland’s header region div.

Status: Needs review » Needs work

The last submitted patch failed testing.

zzolo’s picture

Status: Needs work » Needs review

If you go to the block admin page (admin/structure/block) for Garland, there is a CSS issue because there is a td with the header-region class which puts things inline. There are a number of ways to fix this, but I am not sure which one is the best.

Zarabadoo’s picture

I like the idea of a region wrapper. I actually made such a thing in the Studio theme pack.

The only thing I don't like is the double-wrapping of the sidebars. It is not huge since I can easily override it, and I understand the reasoning for it since it fits with the current state of the system. This is one of the reasons I am in favor of passing all attributes down to templates and theme functions.

Like I said though, the complaint is minor and more along the lines of personal taste. This will allow me to remove one more extension from my base theme in the upgrade.

zzolo’s picture

Status: Needs review » Needs work

I did not purposefully put to "needs review". Back to "needs work". See my comment above.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
15.02 KB

Thanks, zzolo!

This new patch deals with that issue. Specifically, it renames the way-too-generically named "region" and "region-NAME" classes used on the block-admin-display-form.tpl.php. That tpl now uses region-title and region-title-NAME classes.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
15.09 KB

Heh. Looks like poormanscron-in-core already added a system_page_alter(). :-D

Re-rolled.

mfer’s picture

Status: Needs review » Reviewed & tested by the community

Worked for me.

JohnAlbin’s picture

FileSize
15.09 KB

Because of some changing FUZZ, the patch doesn't apply without failed HUNKs.

Re-rolled.

JohnAlbin’s picture

Title: Add region-wrapper.tpl for all regions in themes » Add region.tpl.php for all regions in themes

Hmm… just noticed the issue title still says "region-wrapper".

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

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

Re-rolled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

alonpeer’s picture

Status: Needs work » Needs review
FileSize
15.23 KB

The seven theme is broken because it's missing the seven_preprocess_region() in template.php as added to garland. Without this function, template_preprocess_region() in theme.inc isn't called, and the $content variable isn't populated and so the page's body is empty.

I've added seven_preprocess_region(), but I'm not sure this is the right way to go. How do you make template_preprocess_region() to always get called? Or should each theme implement themename_preprocess_region()?

JohnAlbin’s picture

FileSize
14.98 KB

I've added seven_preprocess_region(), but I'm not sure this is the right way to go. How do you make template_preprocess_region() to always get called? Or should each theme implement themename_preprocess_region()?

That's not needed at all. You're encountering that error because you've installed D7 without that functionality and then applied the patch and the theme registry needs to be rebuilt. Try temporarily adding drupal_flush_all_caches(); to index.php to fix that.

I've re-rolled the patch in #36.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Updating status

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

mattyoung’s picture

subscribe

JohnAlbin’s picture

Status: Needs work » Reviewed & tested by the community

Re-rolled patch to work with page.tpl/html.tpl split.

JohnAlbin’s picture

FileSize
13.29 KB

And… here’s the patch. :-D

webchick’s picture

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

I reviewed this patch and it looks like a good overall clean-up, as well as providing some interesting opportunities for working with blocks later.

Committed to HEAD. Needs documenting in the theme guide.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
@@ -2802,6 +2801,18 @@ function system_page_build(&$page) {
       '#markup' => theme('system_run_cron_image', 'system/run-cron-image'),
     );
   }
+
+  // Find all block regions so they can be rendered.
+  $regions = system_region_list($GLOBALS['theme']);
+
+  // Load all region content assigned via blocks.
+  foreach (array_keys($regions) as $region) {
+    // Don't render empty regions.
+    if (!empty($page[$region])) {
+      $page[$region]['#theme_wrappers'][] = 'region';
+      $page[$region]['#region'] = $region;
+    }
+  }

Doing this in hook_page_build() is not correct because all elements of the page may not have been added yet. For example, this doesn't work correctly with the 'page_top' region (containing the toolbar), since the toolbar module hook gets called after system module. It needs to be done in hook_page_alter() instead.

I needed this fix for the latest overlay patch at #610234: Overlay implementation, but I'm pulling it out here as well since it's an independent bug. The attached patch should fix it.

By the way, the "don't render empty regions" check shown in the above code will not always work - an element of the $page array can be populated, but still not show any content when it is rendered (for example, if #access is set to FALSE). This is the case with the toolbar module; thus, the result is that the 'page_top' region will get output with a wrapper div even if the current user does not have access to see the toolbar. I haven't fixed this in the attached patch because I'm not sure how we would.

effulgentsia’s picture

Status: Needs review » Needs work

#46 seems to now be in HEAD, perhaps from the overlay work. Back to "needs work" for docs as per #45.

droplet’s picture

region.tpl.php templates override broken

region-[name].tpl.php don't work

effulgentsia’s picture

droplet’s picture

region--[name].tpl.php not work yet.

effulgentsia’s picture

Leaving as "needs work" as per #47. I don't know if docs have been completed or not. If they have, please mark this issue fixed.

Re #48/#50: that's being taken up in a separate issue: #821446: Need tests for region-specific overrides of region.tpl.php.

jhodgdon’s picture

I think this needs documenting in both the Theme Guide in the Handbook and the Theme 6/7 Update Guide? Adding tag specific to update guides, accordingly.

jhodgdon’s picture

Status: Needs work » Fixed

Added to theme update guide
http://drupal.org/update/themes/6/7#region-tpl
The updates to the Theme handbook are done based on that page, so OK to mark this one fixed.

droplet’s picture

Status: Fixed » Needs work

a better example for D6:

<div id="header-region" class="clear-block"><?php print $header; ?></div>
jhodgdon’s picture

Status: Needs work » Fixed

Thanks! Duh. I took that example from the patch, and it was d7 -> d7 not d6 -> d7. :( Sorry! Should be better now.

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