Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stijnbe’s picture

Status: Active » Needs work
FileSize
8.8 KB

Initial patch for this issue. What would be the best solution to use here? Should we also patch the themes in core overriding this template?

stijnbe’s picture

Initial patch for this issue. What would be the best solution to use here? Should we also patch the themes in core overriding this template?

Jacine’s picture

Issue tags: +HTML5 Sprint: August 2011 - 1

Tagging!

Jeff Burnz’s picture

+++ b/modules/comment/comment.tpl.php
@@ -57,16 +57,18 @@
+  ¶

Whitespace.

+++ b/modules/comment/comment.tpl.php
@@ -86,5 +88,5 @@
+  <nav><?php print render($content['links']) ?></nav>

No class on nav element, I am not sure what the plan is regarding elements with no classes.

28 days to next Drupal core point release.

Jeff Burnz’s picture

+++ b/modules/comment/comment.tpl.php
@@ -57,16 +57,18 @@
+  <header>
   <?php print render($title_prefix); ?>
   <h3<?php print $title_attributes; ?>><?php print $title ?></h3>
   <?php print render($title_suffix); ?>
+  </header>

This will print the header element even it there is no title, also no class on header element and no indentation.

28 days to next Drupal core point release.

Lars Toomre’s picture

What is the standard on the use of ; in *.tpl.php files? This line is inconsistent:

<h3><?php print $title_attributes; ?>><?php print $title ?>

There also is an extra > after the first print statement.

Jeff Burnz’s picture

What you pasted above is not whats in the patch, you have inserted an extra > after the opening h3, the title_attributes print on the title, not adjacent to it.

Good spotting with the title, should be <?php print $title; ?> for consistency.

stijnbe’s picture

Improved my patch using your comments. Is there already a global function for theming publication time and date?

Jeff Burnz’s picture

Theme function for date time is in the works #1183250: Add a theme_datetime() function to consistently theme dates and datetimes

This patch raises something I hadn't thought of previously, the xpath for RDFa tests uses an element, I don't know if this is going to be a problem or not?

+++ b/modules/comment/comment.tpl.php
@@ -57,16 +56,19 @@
+  <?php if ($title): ?>
+    <header>
+      <?php print render($title_prefix); ?>
+      <h3<?php print $title_attributes; ?>><?php print $title; ?></h3>
+      <?php print render($title_suffix); ?>
+    </header>
   <?php endif; ?>

I don't mean to be picky but shouldn't this be:

<?php print render($title_prefix); ?>
<?php if ($title): ?>
  <header>
    <h3<?php print $title_attributes; ?>><?php print $title; ?></h3>
  </header>
<?php endif; ?>
<?php print render($title_suffix); ?>

26 days to next Drupal core point release.

Jacine’s picture

Component: theme system » comment.module
Status: Needs work » Needs review

This patch raises something I hadn't thought of previously, the xpath for RDFa tests uses an element, I don't know if this is going to be a problem or not?

Good point. Seems like it might be. Setting to needs review so we can see what the testbot says. Also, this belongs in the comment module component, so I changed that.

And yes, the render($title_prefix) and render($title_suffix) lines definitely need to be outside of the <header>. They're not really part of the title and they need to print regardless of whether a title exists or not.

Status: Needs review » Needs work

The last submitted patch, comment-html5-1189816-7.patch, failed testing.

Jacine’s picture

Issue tags: +HTML5 Sprint: August 2011 - 2

Adding to current sprint, plus RDFa tests are failing... http://qa.drupal.org/pifr/test/163124

I guess we need some help with this. :s

stijnbe’s picture

Jacine, I will update the RDFa tests so they work with the new markup and create a new patch. Is this the way to go?

Jeff Burnz’s picture

Will tests still work if we use a wildcard instead of div/article etc - that is what I was thinking, although I am not sure if we actually need to do that, for example do tests typically run against core themes or just core templates, if against core themes (e.g. Seven) then we need to make everything HTML5 or wildcard the element. What I am saying is do we have tests that run on any theme, or only ones specifically for what is in core.

Jacine’s picture

I don't know the answer here. I'll see if I can find someone who knows.

sun’s picture

Just replace the xpath selectors with the new ones. That RDF module test specifically is about Comment module integration - nothing generic too worry about.

Jeff Burnz’s picture

Yeah, I tend to agree with sun here, since all core themes etc are to be html5 the generic approach will be to just update xpath selectors as required. My worry was that if someone is testing with another theme the test won't pass, but this is probably not our problem.

sun’s picture

Tests are always executed in an isolated environment; you have to hack the test code to change the theme. Module tests should be and have to be theme-independent. Note that we'll soon switch the default theme for testing to Stark: #1181776: Change theme_default variable to Stark

Everett Zufelt’s picture

Browsing through open issues I found #1272504: Display of nested comments is baffling and confusing. This reminded me that one of the major issues with comments is that there is no semantic parent-child relationship between nested comments.

If you look through a list of nested comments with CSS disabled, or using a screen-reader, there is nothing to signify if any particular comment is "in reply" to another.

I'm not sure if this is able to be dealt with in comment.tpl.php, or if it needs to be addressed in the wrapper, or somewhere else.

Everett Zufelt’s picture

Issue tags: +Accessibility
Jeff Burnz’s picture

Everett, to do that semantically would mean comments in a threaded discussion would have to be nested, so articles nest inside articles - that could make sense. I also think there is a comment format that is missing from Drupal - the stack overflow type, which was not common years ago when this stuff was first designed, but now is much more common.

Everett Zufelt’s picture

Can you explain the stack overflow type?

Articles within articles would be more semantic, though I am not sure if those semantics alone would solve the problem for screen-reader users.

IMO this is a major problem, should we address it here, or in a separate issue?

scor’s picture

Status: Needs work » Needs review
FileSize
11.25 KB

This patch raises something I hadn't thought of previously, the xpath for RDFa tests uses an element, I don't know if this is going to be a problem or not?

RDFa does not rely on the elements but only on the attributes, so changing

into will not cause any trouble in RDFa. The tests are running with the bartik theme which needs its comment.tpl.php to be fixed too (patch attached).

This reminded me that one of the major issues with comments is that there is no semantic parent-child relationship between nested comments.

Yep, and we had to implement some work around for RDFa in Drupal 7. so I'm in favor in fixing that, though IMO this should live in a separate issue. I've got other questions/suggestion but I'll keep them for the separate issue.

Everett Zufelt’s picture

franz’s picture

I'm not sure if this should be included in this issue, but $title_attributes and $content_attributes are missing documentation.

scor’s picture

@franz: separate issue.

franz’s picture

Garland also makes use of comment.tpl.php, although Garland might not make it to Drupal 8 release

dcmouyard’s picture

Shouldn't $title_prefix and $title_suffix be rendered outside the <header> element?

Everett Zufelt’s picture

What is the use case for $title_prefix and $title_suffix? I.e. where / how are they usually used? This will help to decide if they are semantically part of the heading or not.

scor’s picture

It's used for contextual links for example. agree with @dcmouyard, I don't think it makes sense to have these rendered inside <header>.

Jacine’s picture

Status: Needs review » Needs work

$title_prefix should definitely be outside the header IMO. $title_prefix is used for contextual links output. I don't know of anything else that uses $title_prefix, not anything that uses the $title_suffix yet, but I'd argue $title_suffix should be out of the header as well. Regardless of where they go, they must be rendered whether there is a $title or not.

Off topic, but no one understands those variables until explicitly being told what they are used for, so we should really open up a separate issue to get them renamed renamed or at least better documented.

scor’s picture

+++ b/modules/comment/comment.tpl.php
@@ -57,16 +57,19 @@
+  <?php if ($title): ?>
+    <header>
+      <?php print render($title_prefix); ?>
+      <h3<?php print $title_attributes; ?>><?php print $title; ?></h3>
+      <?php print render($title_suffix); ?>
+    </header>

There is also another potential problem: $title_prefix and $title_suffix are dependent on $title being not empty. It's a change from the previous logic where $title_prefix and $title_suffix would be displayed even if title is empty.

15 days to next Drupal core point release.

scor’s picture

Status: Needs work » Needs review
FileSize
10.98 KB

moved $title_prefix and $title_suffix lines outside the $title condition, which should solve both issues.

dcmouyard’s picture

I usually don't use <header> if it only contains a single heading. Is this mentioned in the coding standards anywhere?

droplet’s picture

+++ b/themes/bartik/templates/comment.tpl.phpundefined
@@ -100,6 +104,6 @@
+    <nav><?php print render($content['links']); ?></nav>

NAV on the links ??

5 days to next Drupal core point release.

stevetweeddale’s picture

Tagging for the current HTML5 sprint

Jacine’s picture

Issue tags: -HTML5 Sprint: August 2011 - 1, -HTML5 Sprint: August 2011 - 2 +D8H5

Tagging for the current sprint.

dcmouyard’s picture

Status: Needs review » Needs work

I think we need to wait until this issue is fixed: #1183250: Add a theme_datetime() function to consistently theme dates and datetimes

That way we can use the <time> element for the submitted date.

dcmouyard’s picture

Here’s how I would approach the markup:

<article>
  $title_prefix
  <header>
    <h3>$title</h3>
    <em class="new">$new</em>
  </header>
  $title_suffix

  <footer>
    $picture
    $submitted
    $permalink
  </footer>

  <div>
    hide($content['links']);
    print render($content);
    <div class="user-signature">
      $signature
    </div>
  </div>

  <nav class="comment-links clearfix">
    <h4 class="element-invisible">Comment links</h4>
    $comment_links
  </nav>

</article>
Jacine’s picture

Issue tags: -D8H5 +sprint

Changing script tags.

Jeff Burnz’s picture

Assigned: Unassigned » Jeff Burnz

Assigning me myself, preparing new shiny patch for BADCamp.

Jeff Burnz’s picture

Status: Needs work » Needs review
Issue tags: -html5, -sprint

#33: comment-html5-1189816-33.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +html5, +sprint

The last submitted patch, comment-html5-1189816-33.patch, failed testing.

ericduran’s picture

The test needs to be re-written to account for the changes.

ericduran’s picture

FileSize
13.49 KB

This was pretty easy, I think.

This one should pass the test. I didn't change anything in the markup, just fixed the test. FYI.

I didn't actually run the test locally, so it might still fail lol.

ericduran’s picture

Status: Needs work » Needs review

Just sending it to the testbot FYI.

Jeff Burnz’s picture

FileSize
13.73 KB

This is the approach I am working at the moment, its a little different in terms of markup and structure, so lets see what everyone thinks about this.

I moved the $new to below the title, not fussed on where this goes, but I tend to think header should be the first thing in the template. It uses the refactored b tag which implies no additional emphasis and is appropriate for marking up something that is normally going to look different. I tend to think emphasis, say using em, is not entirely appropriate in this context, i.e. emphasizing this one otherwise isolated, often repetitive word seems out of place in the normal usage of emphasis - to highlight some word or phrase within phrasing content, not a single, dynamic word in flow content.

I really want to use footer because its an appropriate tag for this set of elements, in essence creating this "footer element" - user picture, user name, publishing time and permalink. It makes sense to group user picture and submitted, they are closely related, so group them. I wrapped $submitted and $permalink in p tags, not fussed if we do this or not, its easier for theming if they have wrappers and classes, but not crucial.

The other change is $signature which I have used the refactored i tag, which again does not add emphasis but is suitable for phrasing content that might be offset from the normal text and have some other meaning, I actually think i works quite well for a signature type element.

I said my approach would be a little radical and it probably is, I want to get a bit deeper on the text level semantics and think more about how we are grouping related elements.

I haven't written a patch for core for ages, and like 1 using git, it might fail, and Bartik is not changed, its as per the most recent previous patch.

ericduran’s picture

Status: Needs review » Needs work

I'll leave the semantic aspect for someone else to review. I personally don't see anything wrong with @Jeff Burnz approach.

Now for the few code review:

+++ b/modules/comment/comment.tpl.phpundefined
@@ -57,34 +57,37 @@
     <?php
-      // We hide the comments and links now so that we can render them later.
-      hide($content['links']);
-      print render($content);
+    // We hide the comments and links now so that we can render them later.
+    hide($content['links']);
+    print render($content);

Everything after the <?php should be indented. At least thats the pattern that seems to be everywhere else.

+++ b/themes/bartik/templates/comment.tpl.phpundefined
@@ -57,7 +57,7 @@
+<article class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

Are we keeping the cleafix out of the classes array on purpose? I know we do this in a lot of the tpl's so I'm just wondering if this is on purpose.

-19 days to next Drupal core point release.

scor’s picture

+++ b/themes/bartik/templates/comment.tpl.php
@@ -84,14 +84,18 @@
       <?php
-        // We hide the comments and links now so that we can render them later.
-        hide($content['links']);
-        print render($content);
+      // We hide the comments and links now so that we can render them later.
+      hide($content['links']);
+      print render($content);
       ?>

I think this should remain indented the way it was before. Asked Jacine and Jeff Burnz here at the BADCamp code sprint, and they both agree. Even if the goal is to have them indented properly in HTML, we feel it's best to have it indented properly in the PHP tpl file.

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
13.71 KB

I have moved the hide() bit to above the template and just render the $content, so the indentation issue is solved by default, sorry about that, slip of the keyboard maybe.

Also nuking footer and moving picture, submitted and permalink into the header re discussion with Jacine, this will bring some consistency with node.

Jeff Burnz’s picture

OK, missed the if title which doesnt need to be there, changing b to mark after discussion with Jacine, lets give mark a go in this context, it does add a little emphasis but maybe thats OK after all, in terms of how machines will read this.

Jeff Burnz’s picture

FileSize
13.67 KB

Ops...

scor’s picture

Status: Needs review » Needs work
+++ b/themes/bartik/templates/comment.tpl.php
@@ -84,14 +84,18 @@
-    <h3<?php print $title_attributes; ?>><?php print $title; ?></h3>
+    <?php if ($title): ?>
+      <header>
+        <h3<?php print $title_attributes; ?>><?php print $title; ?></h3>
+      </header>
+    <?php endif; ?>

you missed that one too...

+++ b/themes/bartik/templates/comment.tpl.php
@@ -84,14 +84,18 @@
-        // We hide the comments and links now so that we can render them later.
-        hide($content['links']);
-        print render($content);
+      // We hide the comments and links now so that we can render them later.
+      hide($content['links']);
+      print render($content);

these need to be reindented back too.

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
14.17 KB

Thanks scor, here's some minimal effort, mostly to satisfy the test bot. I would prefer to tackle all of Bartik (and Seven) separately - after most of these HTML5 core template and CSS changes are in, however due to tests we have to make these changes. I say this because we should understand the changes to Bartik for tests sake should not be considered a final, since we can't really nail that at this stage due to future changes for other things like responsiveness.

scor’s picture

As discussed with @Jacine, @webchick and @boombatower today, we need to a way to properly test with the stark theme (to test the tpl files coming with core modules) and Bartik (to test what a typical Drupal user gets when installing Drupal). Right now all tests are run and are written for the Bartik markup. See the related issue #1181776: Change theme_default variable to Stark.

One solution which came up at the code sprint was to force stark as default theme in the setUp() of the test. I tried that for rdf.test. By extending CommentHelperCase(), which is the best practice when writing tests for comments, testCommentNewCommentsIndicator() gets automatically run as well. It fails on stark. We could fix this function for stark, but then it would fail for all the other tests which are run on Bartik. I tried to fix both xpath expressions of testCommentNewCommentsIndicator() (it's #1 and #3) to make them more lax and work on both stark and bartik: while I was able to fix the first one, the second one is impossible to fix because of the different logic in these themes. (They expect and test a different number of node links for a given test scenario).

Bottom line is:
- we can't get the testbot to test the modules tpls that Bartik overrides until something like #1181776: Change theme_default variable to Stark is fixed.
- let's focus on upgrading all module tpl file to HTML5 without worrying about Bartik and the tests (bear in mind these modules tpl.php files cannot be tested currently)
- when we upgrade Bartik to HTML5, then fix the tests too.

I'm uploading a reroll of #54 without the Bartik and test changes.

scor’s picture

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for all the work and detailed explanation on that @scor.

I really don't like our options here, but after talking to @webchick, it seems like this is our best option right now.

Sooo, RTBC!

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.77 KB

- Fixed indentation
- Removed paragraph around permalink.

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @sun! Looks good.

Jacine’s picture

Status: Reviewed & tested by the community » Needs review
+      <i class="user-signature clearfix">
+        <?php print $signature; ?>
+      </i>

I'm actually kinda on the fence about the <i> element here. I've asked around about this a little, so setting back to needs review for a short while longer until I hear back.

Jacine’s picture

I asked Bruce Lawson about this. This was the Twitter convo:

@jacine:
How would you markup a user's signature in a post comment? #drupal #html5 /cc @brucel @divya
@brucel:
@jacine nothing special required, although you could use <address> around their contact details (email/ twitter id etc)
@jacine:
@brucel It's user-specified, so there isn't necessarily contact info. Could just be a quote or something random. <i> & <aside> were proposed.
@brucel:
@jacine I don't think either of those are right. Just use <span> or <b>. or nothing at all. (I use <cite> but that's against spec)

Given that conversation, and the fact that our user signature can accept filtered content, we need to use a <div> or nothing here. I say we just go with <div class="user-signature">, drop the clearfix and call it a day.

I've attached a revised patch for that for review.

Jacine’s picture

Same patch without the whitespace. Also, forgot to mention in the previous comment, the patch updates the comment that hides the links.

Jacine’s picture

Sigh. One more time?

Jacine’s picture

Found another issue. We need to render to check for $content['links'] or we'll always get an empty <nav>.

FYI, just found another bug in comment.module that relates to this: #1321248: Comment forbidden link causes links to render when user has no permissions for comments .

rasskull’s picture

I would like to make a suggestion to consider removing the nav element from the $links var

+    <nav><?php print $links; ?></nav>

Whatwg.org spec on nav says that it should be used for "major navigation blocks". Removing it would also fix the issue of an empty element if $content['links'] is empty.

JohnAlbin’s picture

Status: Needs review » Needs work

Yeah, <nav> is only for major navigation.

Also, related to the mention of pubdate in the initial issue description, there's this today… :-p

@html5doctor:

<time> and @pubdate are gone http://www.w3.org/Bugs/Public/show_bug.cgi?id=13240
hello <data> http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-s...
We’ll write about it soon… #html5

Jacine’s picture

Ok, ok... :P I will remove the <nav> from node patch as well, because I'd like them to be consistent with each other.

BTW, <time> discussion is going on over here: #1183250: Add a theme_datetime() function to consistently theme dates and datetimes.

Jacine’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

Alright, here's an updated patch that removes that <nav> element.

rasskull’s picture

I wanted to see what it would be like if the links were wrapped in a div with a class and clearfix. I'm not sure how best to check if the links array exists, but I figured that checking the rendered output would give an accurate reading.

Should clearfix also be on content div?

rasskull’s picture

Here is the bit I added if you don't want to open up the patch

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

There could be some indenting issues here :/

Jacine’s picture

Personally, I'm against wrapping the links in a <div>, because there is really no need. I'm also against the blatant abuse of the .clearfix classes that are currently littered all over our codebase. I'd actually like to remove it from the wrapper as well, but that's a separate issue that I'm sure certain people will disagree with me on. We're not actually floating anything that we need to clear here, and personally I like to use markup-free clearing techniques. You should have heard the Fronteers crew trashing our markup, and making fun of this very problem. It's a classic case of Drupal making too many assumptions, and adding more than is needed. It'd prefer to leave this up to themes. They'll know if they need it or not, and will hopefully appreciate the fact that Drupal isn't imposing excess by default.

@raskull As far as checking if there is content from a render array before printing a wrapper, see my previous patch. That's the current accepted way of doing it. Of course you can also render the variable at the top of the template and do a standard <?php if ($links) { ... print $links; ... ?>, but that's kinda off-topic unless you want to argue for the <div> in the core implementation. So, I guess the question is, why do you want the <div>? ;)

rasskull’s picture

re-rolled with indentation fixed, removed a new line after first php tag and added clearfix to content div.

rasskull’s picture

Jacine, sounds good - I was just testing the waters here. I'm a fan of keeping things uncluttered as well so I will defer to your expertise here :)

Jeff Burnz’s picture

Assigned: Jeff Burnz » Unassigned
Anonymous’s picture

So which patch is the one that needs review here? I'm assuming Jacine's in #68, let me know if that is correct.

Jacine’s picture

@linclark Oh, sorry! Yes, #68 is still up for review. Thanks Lin!

rasskull’s picture

yeah, ignore my patches for #69 and #72

Jacine’s picture

FileSize
1.99 KB

Re-rolled #68 due to the core directory change.

Jacine’s picture

FileSize
1.81 KB

Oh crap. Whoops, wrong patch... That was #72.

THIS is a re-roll of #68. Sorry about that. It's probably time for me to go to sleep now. :P

Everett Zufelt’s picture

Can someone please reference the actual authoritative spec for <nav>? I haven't looked in a while so not sure about removing from links, but happy about consistency w/ node.tpl.php if it is removed.

Note: WHATWG is * not * authoritative about HTML5, as it is not part of W3C nor is it consensus driven.

Anonymous’s picture

I think we should keep the politics of WHATWG vs W3C out of our dev process, the Drupal community is better than all that. We should discuss the reasoning behind following either's version of the spec on a case by case basis (as was suggested by Crell I believe in the kickoff meeting).

Everett Zufelt’s picture

Respectfully disagree. I still prefer references to W3C spec. No need for further off topic debate in this issue though.

rasskull’s picture

Jacine, #79 is looking good to me regarding the nav element removal to match up to comment.tpl as well as the header logic changes. I'm too much of a noob to mark this as RTBC though :)

Anonymous’s picture

The article element represents a self-contained composition in a document, page, application, or site and that is, in principle, independently distributable or reusable

Do comments really represent self-contained compositions that are independently distributable? Would you ever distribute a comments section without the node?

rasskull’s picture

The following sentences after your quote from the W3C spec also say this:

This could be a forum post, a magazine or newspaper article, a blog entry, a user-submitted comment, an interactive widget or gadget, or any other independent item of content.

So without getting into this issue too deeply here, it seems they specifically say that a user-submitted comment can be considered an 'article'.

Anonymous’s picture

well will you look at that....

When article elements are nested, the inner article elements represent articles that are in principle related to the contents of the outer article. For instance, a blog entry on a site that accepts user-submitted comments could represent the comments as article elements nested within the article element for the blog entry.

objection withdrawn ;)

Michsk’s picture

Ok so, i also don't want to push this to deep but:

The smell test for going independent
An independent piece of content, one suitable for putting in an element, is content that makes sense on its own. This yardstick is up to your interpretation, but an easy smell test is would this make sense in an RSS feed? Of course weblog articles and static pages would make sense in a feed reader, and some sites have weblog comment feeds. On the other hand, a feed with each paragraph of this article as a separate post wouldn’t be very useful. The key point here is that the content has to make sense independent of its context, i.e. when all the surrounding content is stripped away.

This just isn't a comment, i know what w3c states but that piece above exactly describes how i see this.

Jacine’s picture

Alright... So, in the interest of not bike-shedding and moving this issue forward, here's my take on this.

On <article> as a wrapper...

  1. The spec specifically mentions user-submitted comments should use <article>. See @linclark's comment in #86 and the spec itself.
  2. This is how people are marking up HTML5 sites. For examples, see Bruce Lawson's blog, and the default WordPress theme. And there are plenty more examples of this in action.
  3. <section> was suggested in IRC, but makes no sense to me given the what the spec clearly states... Also, comments don't always have titles, so using <section> would be abusive and at that point we'd be better off with a <div> IMO.

Basically, this is a case of the spec being crystal clear. If we have a problem with the spec that's an issue to take up with spec writers. We are just implementors. ;)

On <nav> as a wrapper for links...

The spec notes the following:

Not all groups of links on a page need to be in a nav element — the element is primarily intended for sections that consist of major navigation blocks. In particular, it is common for footers to have a short list of links to various pages of a site, such as the terms of service, the home page, and a copyright page. The footer element alone is sufficient for such cases; while a nav element can be used in such cases, it is usually unnecessary.

However, in "Introducing HTML5" by Bruce Lawson and Remy Sharp, this exact use case was mentioned and challenged the spec as being an exception arguing there were benefits associated with using it for assistive technology. In the second edition though, they've changed their stance because in some cases it could be clutter and therefore less helpful for AT.

Personally, I still think <nav> is okay when viewing a full node, because in the context of a full page article the links could be argued as major navigation and helpful for AT. However, in a "river of news," I agree that it ends up being clutter so I'm completely on board with not using it at all in this template.

webchick’s picture

Yeah, additionally, the part of the spec quoted at #87 indicates that the piece of content needs to make sense on its own, outside of the context within which it was posted.

This..

"OMG! I can't believe you said that! Your mother was a hamster and your father smelt of elderberries!!!"

...doesn't make any sense outside of the context of the surrounding comments and the article to which it belongs. :)

Jacine’s picture

Assigned: Unassigned » Jacine
Status: Needs review » Needs work

This should actually be consistent with the node template.

mortendk’s picture

hmm is it me or do we have to groups of people working on this comment stuff: #1216976: Clean up the CSS for Comment module

Jacine’s picture

@mortendk We should not. This one is about markup and the other is about CSS.

Jacine’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

Here's an updated patch that brings this more in line with the node.tpl.php template. With the patch applied it looks like this:

<article class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

  <?php print render($title_prefix); ?>
  <?php if ($new): ?>
    <mark class="new"><?php print $new; ?></mark>
  <?php endif; ?>
  <h3<?php print $title_attributes; ?>><?php print $title; ?></h3>
  <?php print render($title_suffix); ?>

  <footer>
    <?php print $user_picture; ?>
    <p class="submitted"><?php print $submitted; ?></p>
    <?php print $permalink; ?>
  </footer>

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

  <?php print render($content['links']) ?>
</article>

This patch also changes the name of the $picture variable to $user_picture, also to be consistent with the node template.

jessebeach’s picture

This is the same patch as #93 except I removed the left-alignment of the equal signs for the variable assignment in template_preprocess_comment. I'm under the impression that the Drupal Coding Standard discourages this type of formatting (given that I've had others flag it for correction in my patches).

Otherwise I changed nothing about functionality.

Eventually, the <h3> around the title of the comment should be made an <h1> once the page outlining algorithms for parsers catches up.

I say commit it.

jessebeach’s picture

Blarg, my changes were present. Reposting with my changes. Disregard the patch in #94.

dcmouyard’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #95 looks good to me!

droplet’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/comment.moduleundefined
@@ -2263,20 +2263,20 @@ function comment_form_submit($form, &$form_state) {
-  $variables['comment']   = $comment;
-  $variables['node']      = $node;
-  $variables['author']    = theme('username', array('account' => $comment));
-  $variables['created']   = format_date($comment->created);
-  $variables['changed']   = format_date($comment->changed);
...
-  $variables['new']       = !empty($comment->new) ? t('new') : '';
-  $variables['picture']   = theme_get_setting('toggle_comment_user_picture') ? theme('user_picture', array('account' => $comment)) : '';
+  $variables['comment'] = $comment;
+  $variables['node'] = $node;
+  $variables['author'] = theme('username', array('account' => $comment));
+  $variables['created'] = format_date($comment->created);
+  $variables['changed'] = format_date($comment->changed);

don't we allow more space be inserted to promote readability.

+++ b/core/modules/comment/comment.tpl.phpundefined
@@ -57,34 +57,33 @@
     <?php print $permalink; ?>

comment permalink or node, it's not clear ?

+++ b/core/modules/comment/comment.tpl.phpundefined
@@ -57,34 +57,33 @@
+      <?php print $signature; ?>

$user_signature ?

24 days to next Drupal core point release.

dcmouyard’s picture

Status: Needs work » Reviewed & tested by the community

According to the coding standards operators should have a single space before and after them, so jessebeach’s changes are correct.

The permalink is for the comment, not the node. It even mentions that in the docblock.

I wouldn’t be opposed to renaming $signature to $user_signature, but I don’t think it’s necessary.

droplet’s picture

@dcmouyard,
I wouldn’t be opposed to it but see Function Calls part (coding standards) and other modules are allowed.

Jacine’s picture

TBH, the coding style issues (if they are an issue) have zero to do with this patch. I changed the name of one variable, and left the formatting the way it was. I don't like it either, but it exists like that in quite a few other places. Some people would call that kitten killing, as there may be other patches out there that deal with this. :s

But anyway, if we could please keep the comments focused on what this patch is actually affecting that'd be great. We're averaging a very high number of comments per issue is and the bikeshedding is becoming a serious problem.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks great. Committed to 8.x. Thanks all!

Jacine’s picture

YAY!! Thanks :D

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

Anonymous’s picture

Issue summary: View changes

Add message regarding time and pubdate.