Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

Looking at this issue. Adding article, header and footer elements not really tricky.

As for time, this could be added in the template, we would have to redo how the author / date is printed, or it could be added to template_preprocess_node() by altering $submitted directly.

Although adding it to the template is a bit more work, it would mean that all of the html5 markup is in the template file. Adding it to the preprocess function means that there would be two places where contrib authors would have to look to remove the html5 elements.

Everett Zufelt’s picture

Also, $content['links'] is really the only thing that I think we have in the footer of a node. Since there may not be content in $content['links'] then I think it would be a good idea to do a test before printing the footer, thoughts?

<?php if $content['links']: ?>
<footer>
<?php print render($content['links']); ?>
</footer>
<?php endif; ?>
Jeff Burnz’s picture

In node (and comment) it makes more semantic sense for <footer> element to contain meta info - such as date and author. There is a use case for links, e.g. when there is a link to comments (comments are related documents). Its probably worth looking at how <footer> is being used in the wild. I'm not sure we really need to have two footers in node tpl, just get our biggest semantic bang for your bucks which to me is date and author info.

I think we should invent a new theme function for dates/times see: #1094324: Add new theme_functions for consistency - Discussion

Everett Zufelt’s picture

@Jeff

I was thinking that header would be most appropriate for title / author / date. See http://html5doctor.com/the-header-element/

Where I believe on that page title / author / date is in the <header> of the article. And also explained in one of the examples.

cleaver’s picture

I think that makes sense as a default. Anything else in that space seems a special case.

The spec says: "A footer typically contains information about its section such as who wrote it, links to related documents, copyright data, and the like." I don't think putting author in the footer would be useful to most people.

I originally thought of the footer as being in a page context, rather than node, but this use seems to make sense to me after looking closer at the spec and some examples.

Jeff Burnz’s picture

Its still got to make sense as a styling hook - right?

For example we don't currently do this:

<div class="header">
  <h2><?php print $title; ?></h2>
  <?php print $submitted; ?>
</div>

But we do have this:

<?php if ($display_submitted): ?>
  <div class="submitted">
    <?php print $submitted; ?>
  </div>
<?php endif; ?>

Which would become:

<?php if ($display_submitted): ?>
  <footer class="submitted">
    <?php print $submitted; ?>
  </footer>
<?php endif; ?>

I discussed the "links" problem in #html5 some time ago and outside of Drupal in other places and some feedback I got was that the redefined <menu> element could be a good fit for wrapping links.

Currently we don't use any additional wrappers for the title or links - which raises the question - is this markup purely for the sake of semantics or are we going to make sure they are useful as well (as styling hooks).

Everett Zufelt’s picture

@Jeff

If the links aren't footer than we don't have a footer in node.tpl.php, that I can tell. That's alright, if someone wants to add footer for some other reason they can do it in contrib.

IMO header should be used once at the beginning of the node for title / author / date / etc. Not as a wrapper for each. The three together make up the "header" of the node / article.

As to whether these classes are only for semantics or not, I cannot really address this. What I can't say is that Ideally they can be used for additional style hooks, but not at the expense of unnecessary / inaccurate semantics.

casey’s picture

http://www.w3.org/TR/html5/sections.html#article-example uses <header> for post info in a first example, but <footer> for author/date information for comments in a second example.

The first example also contains a link "Show comments" in <footer>.

Everett Zufelt’s picture

In the spec it states:

Notice the use of footer to give the information for each comment (such as who wrote it and when): the footer element can appear at the start of its section when appropriate, such as in this case. (Using header in this case wouldn't be wrong either; it's mostly a matter of authoring preference.)

I would rather not leave this up to authoring preference, let's pick an implementation and standardize it.

Jeff Burnz’s picture

@Everett, yes I know what you mean, however that is the problem - wrapping the title, author and date in one header element doesn't make a whole lot of sense from a designers perspective - it's much more flexible if the title and submitted information is separate. In my example I show the current approach and there are good reasons why title and submitted are separate - because its flexible for themers. There are no ideal semantics here, HTML5 gives us leeway, we can use that to our advantage and build templates that are not only semantically rich, but also great for themers. That is the "ideal" we should be striving for.

@casey - yes, great isn't it. I think this is one of the best things about HTML5, its very flexible and its up to us to define the semantics, we're not locked into a single, heavy handed specification.

cosmicdreams’s picture

So, I'm confused. What should the focus of this issue be?

1. Supplying the bare minimum of html formatting so that a node can be rendered?
2. Providing a recommended reference implementation for how a themer should render a node?
3. Providing the best most implementation we can think of for rendering a node?

If 1 and 2) we should simply aspire to create a node.tpl.php that "works" and implements whatever it needs to in order to use the semantic markup so that search engines can gobble up all the stuff nodes spit out.

Jacine’s picture

Ok, so we are not just looking to throw together any implementation that just "works" and search engines should have zero to do with this.

The goal here is to make the best, most semantic implementation we can. It needs to be a sensible default and if we do really well, many themes wont bother overriding it. This means it also has to make sense from a design perspective for the majority of use cases.

I think we should do some research into how others are marking posts up. There's a slew of sites that are all nicely categorized here: http://html5gallery.com. We can pick some good examples, post them here as "templates" and discuss what's best for our own implementation.

Does that sound okay?

Jeff Burnz’s picture

OK, here is one from http://bit.ly/o3ve3p - this is actually a Drupal site.

Thing to note about this example is that the h1 is inside the article - this is the full view, not a teaser view.

<article>

  <header>  
    <h1>...</h1>
    <div class="itemMeta">
      Posted in <a href="/">taxonomy term</a> on <span class="...">date time</span>
    </div>
  </header>

  <img src="..." alt="" title=""  class="..." width="..." height="..." />	
    
  <p>Content in paragraphs etc...<p>
	
  <footer>
    <div class="itemMeta">
      Posted in <a href="...">taxonomy term</a> 			 
    </div>
    
    <div class="addthis_toolbox addthis_pill_combo share">       
      <a class="addthis_button_compact">Share</a> 
    </div>
  </footer>

</article>
Jeff Burnz’s picture

This next one is from http://openconcept.ca also a drupal site and built using one of my theme (full disclosure of course).

The first code block is the teaser, the second is the full node view - on the teaser we have the node title in the article but on the full view its using the standard page title found in most Drupal themes - this is weak IMO because it leaves an untitled section - this is one thing I would really like to see "fixed" in D8.

With additional wrappers and field markup there is a lot more markup in this Drupal 7 example.

The menu element was chosen after consultation with accessibility experts who thought this would provide superior semantics in the long run and not hurt anything now (because it wraps a UL).

Note the use of WAI ARIA roles and microformats. datetime attribute has a proper valid date string also.

Teaser view:

<article id="node-2514" class="node node-blog node-teaser" role="article" about="/path/to/node">
   <div class="node-inner clearfix">

    <h2 class="node-title">
      <a href="/" rel="bookmark">Node title</a>
    </h2>
        
    <footer class="submitted" role="contentinfo">
      Submitted by Jason on <time datetime="2011-07-14T14:27:56-04:00" pubdate="pubdate">Thu, 2011-07-14 14:27</time>
    </footer>

    <div class="node-content">
      <div class="field field-name-body field-type-text-with-summary field-label-hidden">
        <div class="field-item even">
          <p>content...</p>
        </div>
      </div>
    </div>

    <menu class="node-links">
      <ul class="links inline">
        <li class="..."><a href="...">....</a></li>
      </ul>
    </menu>

  </div>
</article>

Full node view:

<article id="node-2514" class="node node-blog node-view" role="article" about="/path/to/node">
  <div class="node-inner clearfix">

    <footer class="submitted" role="contentinfo">
      Submitted by Jason on <time datetime="2011-07-14T14:27:56-04:00" pubdate="pubdate">Thu, 2011-07-14 14:27</time>
    </footer>
    
    <div class="node-content">

      <div class="field field-name-body field-type-text-with-summary field-label-hidden">
        <div class="field-item even">
          <p>field content...</p>
        </div>
      </div>

    </div>

    <menu class="node-links">
      <ul class="links inline">
        <li class="..."><a href="...">....</a></li>
      </ul>
    </menu>
    
    <div id="comments" class="comment-wrapper">
      comment form and comments...
    </div>

  </div>
</article>
Jacine’s picture

Here's one from Bruce Lawson's blog:

<article role="article">
  <header>
    <h2><a href="#" rel="bookmark" title="Permanent link to $title">$title</a></h2>
    <time pubdate="" datetime="2011-07-12">$date</time>
  </header>
  $content
  <footer>
    Posted in <a href="#" title="View all posts in $tag_1 $tag_2" rel="category tag">$tag_1 $tag_2</a>.
    <a href="#comments" title="Comment on $title">Leave a comment</a>
  </footer>
</article>
Jacine’s picture

I much prefer the structure of 13 as opposed to 14.

The menu element was chosen after consultation with accessibility experts who thought this would provide superior semantics in the long run and not hurt anything now (because it wraps a UL).

The usage of <menu> seems off to me for post links. I don't think post links are like "commands" at all, except for maybe
"add comment." I think <menu> makes more sense for the Toolbar, Contextual links and Action links than in this case. Also, this hasn't been implemented yet, but according to the spec once it is, this could easily bite us in the ass. See the pic below the code. Here's some more on this from HTML5 Doctor:

The difference of nav from menu

If you aren’t aware there is another element that can confuse the issue in the HTML 5 specification – menu. I’ve noticed that some developers are using the element for navigation rather than the element. We thought it best to clarify that is to be used for a list of commands and is an interactive element and more likely to be used exclusively in Web Applications. We will be covering more about the element in a later post.

Another thing I don't like about 14 is the use of <footer> for the submitted information at the top of the post. I know that technically this is just as correct as using <header>, but it's a WTF as far as I'm concerned nonetheless.

Some combination of all 3 would probably be best.

Jeff Burnz’s picture

Do we even need header and footer elements? I know lots of people are using them, but do we really "need" them?

Also about h2 - in the first example above the heading is in the article, even on full node view, this creates a properly titled section, without this the outline broken because page title is always served from page.tpl.php (untitled section) - we need to rethink this I believe, and have the title always in the article, and shift the logic to page template.

Jacine’s picture

We probably don't need the header and footer elements, but I think they do make sense semantically. It's a good question. And yes, I agree that we need to move the title into the node template permanently to solve sectioning issues.

skottler’s picture

Status: Active » Needs work
FileSize
1.79 KB

Here is a first go-round. It requires some tweaking of the attribute, though. Its loosely based on #13.

skottler’s picture

Apparently the indentation in Vim is totally broken. I will update with a less-borked patch.

Everett Zufelt’s picture

Looks good.

I like the use of header and footer. I think that the links should be in the footer, but the comments likely should not. IMO comments aren't part of the footer of te article, but are a separate sub section of the article.

skottler’s picture

At second pass, do you think it makes sense to make the primary content area of the node use the section tag?

skottler’s picture

FileSize
1.78 KB

Here is the corrected patch using the article tag instead of a div for the primary content area.

skottler’s picture

Status: Needs work » Needs review
sanduhrs’s picture

#22 I don't think so, per definition everything outside of a sectioning tag is the main content.
So if you have content in an article tag everything in there that is not inside another sectioning tag is the content.

Since article is a sectioning element, we can use h1 for the title here.
See http://dev.w3.org/html5/spec/Overview.html#the-article-element
The link also contains a nice example of how to include the comments.

Also the header element should probably be conditional otherwise it could be empty when all of the contained variables are empty. Same for the footer

Please use spaces instead of tabs to indent.

matglas86’s picture

FileSize
1.84 KB

I reviewed the patch and found 2 minor things. So here is a new patch.

1. Indent on line 97 & 98 was incorrect.
2. The first article tag was not closed.

Besides that I think the structure looks good. One thought I had was that it might be a good idea to implement the summary tag for when a teaser is build. It would need to be researched to see whats the best option to do this.

Everett Zufelt’s picture

Took a quick look at the patch.

1. Shouldn't the title be an h1, as this is likely the main title on the page, unless it is a summary, in which case it should be an h2. Note, I don't personally believe that AT / UAs are ready for the headings / sectioning to work by spec in html5

2. Is there a reason that links were taken out of the footer, and that comments are in. Semantically are comments a footer? I thought that links would be good in the footer, and comments should be their own sub-section.

Thanks

Jacine’s picture

Status: Needs review » Needs work

Re: #26:

One thought I had was that it might be a good idea to implement the summary tag for when a teaser is build. It would need to be researched to see whats the best option to do this.

<details> or <summary> tags are interactive elements with and open and closed states. They will behave exactly like collapsible fieldsets in Drupal, but natively. This has only been implemented in Chrome thus far and definitely isn't right for this. See this HTML5 Doctor article for more on this.

Re: #27:

1. Shouldn't the title be an h1, as this is likely the main title on the page, unless it is a summary, in which case it should be an h2. Note, I don't personally believe that AT / UAs are ready for the headings / sectioning to work by spec in html5

Yes, I agree with this. The current recommendation is to use standard heading levels because it will pretty much destroy the accessibility of web pages and no browser has actually implemented HTML5 sectioning yet. There was an article about this just 11 days ago on Smashing Magazine. It states the following, emphasis mine:

I would strongly advise that until browsers — and, more critically, screen readers — understand that sectioning content introduces a sub-section, using multiple h1 elements is less safe than using a heading structure that reflects the level of each heading in the document, as shown in figure 6 below.

This means that user agents that haven’t implemented the outlining algorithm can use implicit sectioning, and those that have implemented it can effectively ignore the heading levels and use sectioning content to create the outline.

At the time of this writing, no browsers or screen readers have implemented the outlining algorithm, which is why we need third-party testing tools such as the outliner. The latest versions of Chrome and Firefox style h1 elements in nested sections differently, but that is very different from actually implementing the algorithm.

Bottom line it should be an <h2> for teasers.

2. Is there a reason that links were taken out of the footer, and that comments are in. Semantically are comments a footer? I thought that links would be good in the footer, and comments should be their own sub-section.

I agree with this too. The links should be in the <footer> as I see it, and the comments belong in a <section> (inside the article tag).

Jeff Burnz’s picture

Comment wrapper uses the section element so no need for any extra markup around comments in node tpl. Agreed that footer should only contain the links.

+++ b/modules/node/node.tpl.php
@@ -78,33 +78,32 @@
+    <article class="content"<?php print $content_attributes; ?>>

This is strange, article is a sectioning element that wraps the whole article, this should just be a div - its purely a styling hook in this position.

I don't see a closing article element, and there is no last line.

3 days to next Drupal core point release.

cosmicdreams’s picture

FileSize
1.82 KB

Here is a patch with the changes requested in #29

cosmicdreams’s picture

Status: Needs work » Needs review
Jacine’s picture

Issue tags: +D8H5
Jacine’s picture

Whoops. It's not September. LOL.

dcmouyard’s picture

Status: Needs review » Needs work

I don't think the <header> element is necessary here, and it definitely shouldn't include $user_picture, $title_prefix, or $title_suffix.

I also don't think the <footer> element should be used for comments. As Jeff mentioned before, the comments already have a sectioning element in comment-wrapper.tpl.php. I think the article metadata should go in <footer>, such as $user_picture and $submitted.

For the node links, I lean towards using <nav> and including a heading for accessibility.

dcmouyard’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

Here's a patch that incorporates my ideas from #34.

This is my first Drupal core patch, so I'm crossing my fingers...

dcmouyard’s picture

FileSize
1.45 KB

Whoops! Misplaced a " on line 96.

Jacine’s picture

Issue tags: -markupmarines, -D8H5 +sprint

@dcmouyard's patch in #36 is growing on me... Seems like a good mix of all the other examples. What do you guys think?

Jacine’s picture

Assigned: Unassigned » Jacine
jasonsavino’s picture

FileSize
2.78 KB

I believe that the choice between section and article needs to be addressed. IMHO I would use section for full view_mode and article for teaser view_mode. Here is my take on node.tpl.php

Everett Zufelt’s picture

@jasonrsavino

Can you please explain why you would use section / article as you have stated?

klausi’s picture

Status: Needs review » Needs work
+++ b/modules/node/node.tpl.php
@@ -78,21 +80,23 @@
-  <?php endif; ?>
+	<header>	¶
+	  <?php print $user_picture; ?>
+	
+	  <?php print render($title_prefix); ?>
+	  <?php if (!$page): ?>
+	    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h2>
+	  <?php endif; ?>
+	  <?php print render($title_suffix); ?>
+	  ¶
+	  <?php if ($display_submitted): ?>
+	    <div class="submitted">
+	      <?php print $submitted; ?>
+	    </div>
+	  <?php endif; ?> ¶
+	</header>
 
   <div class="content"<?php print $content_attributes; ?>>
     <?php
@@ -102,9 +106,13 @@

@@ -102,9 +106,13 @@
       print render($content);
     ?>
   </div>
-
-  <?php print render($content['links']); ?>
-
+  ¶

use spaces instead of tabs, remove trailing white spaces.

jasonsavino’s picture

FileSize
2.79 KB

@Everett Zufelt oops! i see the confusion, I inadvertently reversed the section and article tags in my explanation and code. I included a new patch file with the update. Maybe it will help clarify but the main idea of the code, however, was to allow for a developer/themer to choose which option to use rather than restricting to one option.

scor’s picture

@@ -102,9 +106,13 @@
+  ¶
+  <?php if($content['links']): ?>
+  <footer>
+    <?php print render($content['links']); ?>
+  </footer>
+  <?php endif; ?>
+  ¶

why not indenting here? the snippet above <div class="submitted"> is indented inside the

There is also whitespaces issue which dreditor will help you find.

Jacine’s picture

Status: Needs work » Needs review
FileSize
2.87 KB

I worked on this yesterday after assigning myself earlier this week, so here's my patch.

Status: Needs review » Needs work

The last submitted patch, node-html5.patch, failed testing.

Jacine’s picture

Status: Needs work » Needs review
FileSize
2.83 KB

Oops, use this one.

Jacine’s picture

FileSize
2.86 KB

Talked about this with @scor & @Jeff Burnz - Here's a revised and easier to grok version ;)

Status: Needs review » Needs work

The last submitted patch, node-html5-1077602-44.patch, failed testing.

scor’s picture

+++ b/modules/node/node.module
@@ -1424,6 +1424,21 @@ function node_is_page($node) {
+function node_preprocess_page(&$variables) {
+  // Prevent the title from being printed in both the node and page template
+  // when a full node is being viewed. In order to have properly sectioned HTML5
+  // markup for nodes, we must ensure the title is printed inside the node
+  // template, as opposed to the page.tpl.php template.
+  if (!empty($variables['node']) && $variables['page']) {
+    $variables['title'] = '';
+  }
+}

@jacine: the tests are failing because of this. The translation page is expecting a title, and fails otherwise.

-19 days to next Drupal core point release.

scor’s picture

Looked at this with @Jacine and others this afternoon. The challenge here is to find a way to get the context of the page: this title removal should only be done on the node view page (node/1) and not on any other tab (node/1/edit). In other words: !arg(2). Of course we want a better way than arg() for doing this, but that's the idea...

Jacine’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
3.4 KB

@webchick gave us a decent solution here. We'll also be back porting this part to Drupal 7.

+  // Make the view mode available in the node object.
+  $node->view_mode = $view_mode;
+

Please review ;)

Jacine’s picture

FileSize
3.24 KB

Man, I am completely useless at sprints. New patch uses preprocess (where this should have been in the first place - I just forgot to switch back when testing), simplifies the code and comment in there and fixes the comment at the top of the node template.

skottler’s picture

This is really great. I love the simplicity of the changes. Going to let the RDF team/others RTBC, but this is RTBC'ed from me.

scor’s picture

Status: Needs review » Needs work
+++ b/modules/node/node.tpl.php
@@ -77,34 +77,36 @@
+  <?php if ($links = render($content['links'])): ?>

Not meaning to offend themers, but aren't we trying to keep the PHP level in tpls as low as possible? It seems we might be introducing a new PHP pattern in this tpl. No other tpl from core has this if construct where a variable is set and checked if it's TRUE at the same time. In fact, I could not find any other tpl in core with the regular if '==' pattern: if ($var == 'value'). The following is what tpl usually look like:

<?php $links = render($content['links']);
      if ($links): ?>

Will test this patch in the meantime and check the markup from an RDFa standpoint.

skottler’s picture

+++ b/modules/node/node.tpl.phpundefined
@@ -77,34 +77,36 @@
+  <?php if ($links = render($content['links'])): ?>

This line introduces an accidental assignment bug.

+++ b/modules/node/node.tpl.phpundefined
@@ -77,34 +77,36 @@
+  <?php if ($links = render($content['links'])): ?>
+    <nav><?php print $links; ?></nav>
+  <?php endif; ?>
 
   <?php print render($content['comments']); ?>
 
-</div>
+</article>

Can't we just print render($content['links']);

-25 days to next Drupal core point release.

webchick’s picture

Well, no, because you don't want empty <nav> tags if there are no links. What does 'accidental assignment' bug mean? It's a deliberate assignment.

webchick’s picture

Oh. Although we could move <nav> to theme_links() I guess.

Jacine’s picture

This is actually very much needed:

+  <?php if ($links = render($content['links'])): ?>
+    <nav><?php print $links; ?></nav>
+  <?php endif; ?>

Without it, you'll ALWAYS get the <nav> tag whether you've got links or not. This is related to #953034: [meta] Themes improperly check renderable arrays when determining visibility. This issue applies to any render arrays that need a wrapper tag. I had to do the same thing in the comment issue. Basically core is very buggy right now.

This line introduces an accidental assignment bug.

Can you please clarify what you mean by this? ;) Thanks!

Jacine’s picture

Ha! Crosspost ;)

Oh. Although we could move to theme_links() I guess.

This is a possibility, but I'd rather leave it here in node.tpl.php. It's not a required wrapper, so if people want to change it, it's a lot easier to change it in here than it is to override theme_links(). We'll also probably have a configurable wrapper tag in theme_links() which will make it that much more complicated for people, so I don't know.

webchick’s picture

Ok, fair enough. :)

skottler’s picture

@webchick, @jacine: assigning a variable inside of a conditional can lead to issues.

If other people are okay with it, then I'm fine with it.

scor’s picture

RDFa markup works great. I've also created #1323830: Place title RDFa metadata inside entity HTML element since this patch will allow us to remove some code from rdf.module :)

We might want to update Bartik at some point up, as a follow up patch or separate issue.

JohnAlbin’s picture

I'm pretty sure that skottler assumed that we meant to do a == equality check, which is why he called our = an “accidental assignment” which would have been a bug. But, no, we really did mean to do a = assignment in the if statement as Jacine and webchick pointed out.

scor’s picture

Status: Needs work » Reviewed & tested by the community

Right, so @skottler, @Jacine and I discussed this on IRC, and agreed it was better to leave the conditional assignment issue as a follow up for #953034: [meta] Themes improperly check renderable arrays when determining visibility. That solves the concerns I had in #54. and re Bartik, this is handled separately in #1179764: Convert Bartik to HTML5. all set for RTBC.

Everett Zufelt’s picture

I'm curious about:

+<article id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?>"<?php print $attributes; ?>>

I know this is in the current tpl. DOesn't this allow for duplicate ids if the same node is rendered twice on a page? Does this ever happen? If so, even as an edge case, it is bad.

Jacine’s picture

@Everett it does allow for duplicate ids in that case, and it's not great. It can be worked around by removing the ID and adding it to the class attribute. This has happened to me in practice only one time, so I'd definitely categorize it as an edge case. It may be possible to use drupal_html_id() in the template, but I'm not positive, and in that case we are better off not providing ID's at all IMO because they'd be unreliable style hooks. So, basically I'd leave it be.

catch’s picture

Version: 8.x-dev » 7.x-dev
+function node_preprocess_page(&$variables) {
+  // In order to have properly sectioned HTML5 markup for nodes, the title is
+  // always printed inside node.tpl.php. This code prevents it from printing in
+  // page.tpl.php as well.
+  if (!empty($variables['node']->view_mode) && $variables['node']->view_mode == 'full' && node_is_page($variables['node'])) {
+    $variables['title'] = '';
+  }
+}

This makes me uncomfortable, since it's leaking assumptions about node rendering into the page as a whole. However while we still have drupal_set_title() and side effects like that I don't see much we can do about it for now.

Committed/pushed to 8.x, looks like this needs porting to 7.x.

catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
yched’s picture

Status: Patch (to be ported) » Needs work

The node_preprocess_page() code pointed by @catch also makes me tickle a little. That's very node-specific, and any other entity that has a "title" (or a similar "label") will need to implement a similar trick. Not that I have a better suggestion for now, though...

+++ b/modules/node/node.tpl.php
@@ -77,34 +77,36 @@
+  <?php if ($display_submitted || $user_picture || $title): ?>
+    <header>
+      <?php print $user_picture; ?>
+      <?php if ($page): ?>
+        <h1<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h1>
+      <?php else: ?>
+        <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h2>
+      <?php endif; ?>
+      <p class="submitted"><?php print $submitted; ?></p>
+    </header>
   <?php endif; ?>

The logic looks odd here :
- if $display_submitted is TRUE and $title is FALSE, an empty <h2><a></a></h2> gets printed ?
- if $display_submitted is FALSE and $title is not empty, an empty <p class="submitted"></p> gets printed ?

-27 days to next Drupal core point release.

catch’s picture

Version: 7.x-dev » 8.x-dev

Hmm that's a good point.

I've rolled this back for now. We should also open a follow-up issue for the title issue as well.

Jacine’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

Ugh, sorry about that @catch and @yched. Duh. Hopefully this patch is better.

Also, this patch removes the <nav> around $links because people are arguing against it in the comment issue and these should be consistent. And one more thing.... Should we add the ARIA role=article in this patch?

Jacine’s picture

FileSize
3.55 KB

Here's one with the ARIA role added.

Jacine’s picture

FileSize
3.55 KB

Oy, same patch as #72 without the whitespace. Thanks @aspilicious!

tstoeckler’s picture

#69 yched:

That's very node-specific, and any other entity that has a "title" (or a similar "label") will need to implement a similar trick.

It's somehow related, so wanted to point people there: #1067126: Support rendering entities in page mode

Jacine’s picture

FileSize
3.58 KB

I've attached a re-roll of this patch due to the core directory commit.

Also, I opened a follow up for the page title issue: #1328048: Page title location needs to be flexible.

rasskull’s picture

#75 patch applied cleanly for me

dcmouyard’s picture

Assigned: Jacine » dcmouyard

I'll review the patch in #75.

dcmouyard’s picture

Assigned: dcmouyard » Unassigned

I think it’s odd that $user_picture gets displayed before the title. I can imagine screen reader users getting confused about hearing “username’s picture” before the title is read.

I would prefer to group $user_picture and $submitted together in a <footer> because it makes more semantic sense to me.

I assume we’ll tackle the <time> and pubdate stuff later…

Everything else looks good. If nobody else agrees with my comments above, then ignore my bikeshedding and mark this RTBC.

rasskull’s picture

I think it’s odd that $user_picture gets displayed before the title. I can imagine screen reader users getting confused about hearing “username’s picture” before the title is read.

I would prefer to group $user_picture and $submitted together in a because it makes more semantic sense to me.

Having done quite a bit of theme work, this idea scares me a little bit as it sounds like it would complicate things from a presentation standpoint. I imagine countless hours of floating divs, setting left margins along with all sorts of other css hackery to get the image on the left of the post haha.

With that being said, dcmouyard you have a good point about the screen reader, so perhaps someone has another solution that would address both? If this something that was deemed not important, please disregard and move on. :)

Jacine’s picture

Having done quite a bit of theme work, this idea scares me a little bit as it sounds like it would complicate things from a presentation standpoint. I imagine countless hours of floating divs, setting left margins along with all sorts of other css hackery to get the image on the left of the post haha.

Yep, this is the precise reason it's at the very top, so that it makes sense from a CSS perspective as well. If we set this up in a way that most people need to override the template just to apply basic styling, IMO we will have failed. There needs to be a happy medium here. I've played with placing the parts of this template in numerous ways, and while none of them are perfect, the proposed patch was the best I could come up with. The image was also previously at the top of the template, likely for the same reason.

If we didn't have to place an image, or the image was always set at a specific small size, this would be a lot easier and more semantic, but we do have to deal with it and need to go with something that makes sense for both styling and semantics.

Everett Zufelt’s picture

Can I suggest that we just fix the issue pointed out in #69, and then do follow-ups for the rest?

This would make it a lot easier to deal with small points of contention.

RobLoach’s picture

FileSize
3.63 KB
20.7 KB
+++ b/core/modules/node/node.moduleundefined
@@ -1424,6 +1427,20 @@ function node_is_page($node) {
+function node_preprocess_page(&$variables) {
+  // In order to have properly sectioned HTML5 markup for nodes, the title is
+  // always printed inside node.tpl.php. This code prevents it from printing in
+  // page.tpl.php as well.
+  if (!empty($variables['node']->view_mode) && $variables['node']->view_mode == 'full' && node_is_page($variables['node'])) {
+    $variables['title'] = '';
+  }

So, the title appears semantically where we want it, but it looks a bit weird underneath the operation tabs on the full page. Not a big problem, just something I'm not quite use to yet. See the attached screenshot.

+++ b/core/modules/node/node.tpl.phpundefined
@@ -77,34 +77,36 @@
+// Hide the comments and links so they can be rendered apart from $content
+// afterwards.
+hide($content['comments']);
+hide($content['links']);

If we have to do this, we should probably do it where it becomes relevant, down below.

+++ b/core/modules/node/node.tpl.phpundefined
@@ -77,34 +77,36 @@
+      <?php if ($page && $title): ?>
+        <h1<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h1>
+      <?php elseif ($title): ?>
+        <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h2>
+      <?php endif; ?>

Could probably have a nest of ifs here to just clean it up a bit.

+++ b/core/modules/node/node.tpl.phpundefined
@@ -77,34 +77,36 @@
+    <?php print render($content); ?>
   </div>
 
   <?php print render($content['links']); ?>
 
   <?php print render($content['comments']); ?>

I'm not sure how I feel about segregating out the links and the comments here. I feel like we should just do render($content), and then provide an appropriate wrapper around each theme function within the system itself. The weighting of the links and comments is already done for us.

For example, for the links, stick a #theme_wrapper on it in node_build_content. For the comments, let's try to decouple them from Node and use comment-wrapper.tpl.php.

28 days to next Drupal core point release.

dcmouyard’s picture

Status: Needs review » Needs work

The Bartik theme overrides node.tpl.php and combines the user picture and submitted info below the title in a <div class="meta submitted">.

Also, the Bartik theme no longer displays the title on individual node pages. So we need to update it’s node template to include the new if statement for displaying the title.

dcmouyard’s picture

For the patch in #82, shouldn't the title prefix and suffix be rendered even if there’s no title?

Also, I think the comments should be rendered outside the content div. I'm on the fence about whether the node links should be in the content div, but I think we should render it separately to add a heading to the node links.

dcmouyard’s picture

First, I agree with Everett in working on the problem pointed out in #69 separately. Perhaps in a separate issue? We should probably fix that issue before continuing with this one

In any case, here’s a summary of changes made in this patch:

  • Displays title prefix & suffix even if there’s no title.

  • Removed link around the title when displayed on an individual node page.

  • Added rel="bookmark" to title link.

  • Moved user picture below the title.

  • Displays user picture and submitted info in a <footer class="submitted">.

  • Added a header with class="element-invisible" to the node links for accessibility. The heading level changes depending on whether it’s on a page.

  • Added title if/else statement to Bartik’s node template.

Finally, I don’t think these changes should be backported to D7 because existing themes won’t display the title on individual node pages. But this should be hammered out when dealing with the issue in #69.

dcmouyard’s picture

Status: Needs work » Needs review
rasskull’s picture

What's the procedure on updating the Bartik theme? Either we hit those tpl's at the same time as the core ones, or wait till all the core ones are finalized would be my suggestion. If we start updating those in the same patch as core files, it could become a sticky situation :)

dcmouyard’s picture

I did a quick check to see how the most popular D7 themes handle the user picture.

The following themes output the user picture after the node title:

  • AdaptiveTheme
  • Corolla
  • Genesis

The following themes output the user picture before the node title:

  • Framework
  • Fusion
  • Omega
  • Zen

And Tao doesn't even display the user picture.

rasskull’s picture

Bartik also seems to show title at the top, then groups user photo inside the submitted area. This might be foolish of me to say, but when was the last time anyone saw a user photo next to a main post? I've been looking around and haven't found one instance that wasn't a comment.

sun’s picture

Jacine’s picture

Status: Needs review » Needs work

A few things...

  1. Bartik (and Seven) are out of scope here. There is a separate issue for that. We are working on core module templates here and that's it, so the Bartik changes should be removed from the patch.
  2. The only part of this patch that is backport-able to D7 is detailed in comment #51.
+++ b/core/modules/node/node.moduleundefined
@@ -1476,6 +1493,12 @@ function template_preprocess_node(&$variables) {
+  // Provide the node ID as an ID attribute. This protects from duplicate IDs.

This is a change that would need to be discussed elsewhere, and should come out of this patch. There's already an issue to make all attributes happen in one variable in preprocess.

+++ b/core/modules/node/node.tpl.phpundefined
@@ -103,8 +106,13 @@
+  <?php if ($node_links = render($content['links'])): ?>
+    <div class="node-links clearfix">
+      <?php $tag = $page ? 'h2' : 'h3'; ?>
+      <<?php print $tag; ?> class="element-invisible"><?php print t('Article links'); ?></<?php print $tag; ?>>
+      <?php print $node_links; ?>
+    </div>

This doesn't fly with me. First of all, I don't think we need a div or an invisible heading here. This title will not make sense in the majority of cases and the code is a tad too complex for the template. I also don't like naming this $node_links, over links when the variable is called 'links' the $content.

28 days to next Drupal core point release.

scor’s picture

Status: Needs work » Needs review
FileSize
3.35 KB

@rasskull: Bartik will be taken care of in #1179764: Convert Bartik to HTML5. I removed the Bartik hunk from the patch.

+++ b/core/modules/node/node.module
@@ -1476,6 +1493,12 @@ function template_preprocess_node(&$variables) {
+  // Provide the node ID as an ID attribute. This protects from duplicate IDs.
+  $variables['attributes_array']['id'] = drupal_html_id('node-' . $node->nid);

That does not solve the issue where the same node teaser is displayed multiple times on the same page. I agree with Jacine it's off topic for this particular issue. #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess() is the issue for the code above, though as I said it doesn't solve all the problems.

+++ b/core/modules/node/node.tpl.php
@@ -103,8 +106,13 @@
+  <?php if ($node_links = render($content['links'])): ?>
+    <div class="node-links clearfix">
+      <?php $tag = $page ? 'h2' : 'h3'; ?>
+      <<?php print $tag; ?> class="element-invisible"><?php print t('Article links'); ?></<?php print $tag; ?>>
+      <?php print $node_links; ?>
+    </div>
+  <?php endif; ?>

I've kept the 'Article links' invisible title for now until we get feedback from more people, and made it a bit less complex.

sun’s picture

Status: Needs review » Needs work

I've tried to implement the suggested "Move page title into node page template" idea back in Drupal 5 already, and all I can say out of that experience is that it blows up and doesn't work out at all. It was horrible for theming.

Especially if it's going to be limited to nodes..... but I guess that's just supposed to be a temporary glitch here, right...?

Anyway, I think we need to rethink the main page content at a larger scope first.

There's much more stuff between the page title and the node content. Say hello to breadcrumb, tabs, help, and whatever other arbitrary stuff in content_top.

Whether there is anything or nothing in between depends on your actual theme, and only on that.

We might be able to solve this challenge by introducing a new content.tpl.php -- a "container" that acts as a wrapper for the main page content output on all pages in Drupal.

In fugly other words, kinda of an improved system_main block. Treating the actual primary page content similar to other blocks on the page (hence, forming the <article>). But without comments. And without a node listing on node listing pages. Just the title, tabs(, help), and the main content output (fields, a form, whatever). [Note that tabs are just a different representation of contextual links -- Contextual module basically had that design idea right from the start.]

That's one way to approach this. Another one would be:

Turn the page title, tabs, and main content into blocks, and introduce a main Article region (that kinda lives within the Content region), so modules and site builders can populate that Article region as they wish, but clearly differentiating it from anything else (node listing, comments, etc) in that $content container to make up the primary <article>.

I guess what all of this boils down to is that we currently have only one notion of $content - but no way at all to allow modules to differ between the main article and "add-on" stuff on and below it. Everything returned from the page callback is stuffed into $content, and the page callback has no chance to say: "Hey, this is right there on the page, but it's NOT the main article piece..."

Now, I totally know that all of this doesn't belong into this issue and discussion. But in order to properly implement this here, we absolutely need to have that discussion and find a proper solution. If anyone can make any sense of what I've written above, I'd appreciate it if you could take that on. :)

However, the approach of the current patch doesn't fly at all for me.

To not block progress here, I'd suggest to simply leave everything alone, but output the node title twice and add an .element-hidden to the duplicate title in node.tpl.php.

This means there's a well-formed and self-contained <article> on the page.

(Still leaves us with moving out comments out of that, but as mentioned before, that's a separate issue anyway already.)

sun’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

Like this.

Jacine’s picture

Status: Needs review » Needs work

I don't think this discussion should happen here. It's much bigger and far reaching than the node template and at this point, this issue is a complete and utter bike-shedding disaster already. I'd like to see #1328048: Page title location needs to be flexible be the issue where we attempt to address that.

+++ b/core/modules/node/node.moduleundefined
@@ -1452,6 +1455,13 @@ function template_preprocess_node(&$variables) {
+  // In order to have properly sectioned HTML5 markup for nodes, the title is
+  // always printed inside node.tpl.php. This code hides it when the title is
+  // printed as page title in page.tpl.php already.
+  if ($variables['page']) {
+    $variables['title_attributes_array']['class'][] = 'element-hidden';

Um, no way. That is no solution, not even a temporary one.

The rest of my feedback in #91 still stands:

+++ b/core/modules/node/node.tpl.phpundefined
@@ -103,8 +106,13 @@
+  <?php if ($node_links = render($content['links'])): ?>
+    <div class="node-links clearfix">
+      <?php $tag = $page ? 'h2' : 'h3'; ?>
+      <<?php print $tag; ?> class="element-invisible"><?php print t('Article links'); ?></<?php print $tag; ?>>
+      <?php print $node_links; ?>
+    </div>

This doesn't fly with me. First of all, I don't think we need a div or an invisible heading here. This title will not make sense in the majority of cases and the code is a tad too complex for the template. I also don't like naming this $node_links, over links when the variable is called 'links' the $content.

24 days to next Drupal core point release.

nimbupani’s picture

+1ing what Jacine said. I dont understand why there needs to be an element-invisible class and a heading for what are essentially node links. This also has nothing to do with HTML5.

Jacine’s picture

Status: Needs work » Needs review
FileSize
2.24 KB

Here's a new patch. It does the following:

  1. Removes the <h1> from the template per @sun's comment in #93. We can deal with that in #1328048: Page title location needs to be flexible and it wont be the end of the world.
  2. Leaves the user picture in the <footer> element. I'm compromising on this because it actually fixes a bug. The user picture should not print if $display_submitted is false.
  3. Puts the $submitted line in a <p>.
  4. Removes all the wrapper markup and invisible heading from the node links. This is not needed, and I will argue to prevent this for as long as I have to. I think that a heading is completely unnecessary, and that it will be more of an annoyance to screen reader users than an aid. In addition, a node can be anything, and the text we put in here could end up completely useless or worse confusing and many people will not even realize it's there.

The entire template with the patch applied, looks like this:

<article id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

  <?php print render($title_prefix); ?>
  <?php if (!$page): ?>
    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>" rel="bookmark"><?php print $title; ?></a></h2>
  <?php endif; ?>
  <?php print render($title_suffix); ?>

  <?php if ($display_submitted): ?>
    <footer>
      <?php print $user_picture; ?>
      <p class="submitted"><?php print $submitted; ?></p>
    </footer>
  <?php endif; ?>

  <div class="content"<?php print $content_attributes; ?>>
    <?php
      // We hide the comments and links now so that we can render them later.
      hide($content['comments']);
      hide($content['links']);
      print render($content);
    ?>
  </div>

  <?php print render($content['links']); ?>
  <?php print render($content['comments']); ?>

</article>

It would be great to get this RTBC before 100 comments, because it's getting ridiculous at this point.

Everett Zufelt’s picture

Looks great.

1. I would prefer header be used rather than footer, but can form consensus w/ footer.
2. Agreed that there is no need for a heading prior to links in this context.

Jacine’s picture

Thanks @Everett! I appreciate the feedback on that.

Regarding <header> vs. <footer>, both are okay. I personally prefer <header>, as I've said a few times in this issue, but the spec description of <footer> is arguably more appropriate. Here's the description of both:

<footer> definition from the spec:

When the footer element contains entire sections, they represent appendices, indexes, long colophons, verbose license agreements, and other such content.

Note: Contact information for the author or editor of a section belongs in an address element, possibly itself inside a footer.

Footers don't necessarily have to appear at the end of a section, though they usually do.

Note: When the nearest ancestor sectioning content or sectioning root element is the body element, then it applies to the whole page.

The footer element is not sectioning content; it doesn't introduce a new section:

<header> definition from the spec:

The header element represents a group of introductory or navigational aids.

Note: A header element is intended to usually contain the section's heading (an h1–h6 element or an hgroup element), but this is not required. The header element can also be used to wrap a section's table of contents, a search form, or any relevant logos.

Everett Zufelt’s picture

+1

dcmouyard’s picture

Status: Needs review » Reviewed & tested by the community

Jacine’s patch in #97 looks good to me.

Jeff Burnz’s picture

Looks awesome +1 rtbc.

rasskull’s picture

Looks great, and yay post 101 rtbc =]

cosmicdreams’s picture

I'll test this patch this weekend of the act of manually testing it will help it get committed

webchick’s picture

Issue tags: -Needs backport to D7

This definitely does not need backport to D7. :) Untagging.

catch’s picture

If the tag goes, we need a separate issue for backporting the fix from #51 - can someone open that so it doesn't get lost? I'm not sure why that in particular needs to be backported tbh so not sure what to call that issue.

webchick’s picture

Priority: Normal » Major

Roger that. Created #1339870: Make the current view mode available on the node object, postponed on this issue.

Also bumping this to "major" instead of "normal". This issue is actually essential to the efforts of the HTML 5 initiative.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Also $node->view_mode is set in the patch, but then never used anywhere. Nor does the comment explain why it's being set.

sun’s picture

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

Removed that hunk.

This is needs to be done properly in #1339870: Make the current view mode available on the node object.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense to do it in that issue, which I've bumped to 8.x, so I've committed/pushed this one to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Component: node.module » node system
Issue summary: View changes