Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Potential use of article, header, footer, time with pubdate and possibly nav or menu for links.
Note that time and pubdate could be built using a new theme function in preprocess.
http://api.drupal.org/api/drupal/modules--comment--comment.tpl.php/7
Comment | File | Size | Author |
---|---|---|---|
#95 | html5ify_comment_module-1189816-95.patch | 4.95 KB | jessebeach |
#94 | html5ify_comment_module-1189816-94.patch | 4.95 KB | jessebeach |
#93 | 1189816-93.patch | 3.2 KB | Jacine |
#79 | 1189816-79.patch | 1.81 KB | Jacine |
#78 | 1189816-78.patch | 1.99 KB | Jacine |
Comments
Comment #1
stijnbe CreditAttribution: stijnbe commentedInitial patch for this issue. What would be the best solution to use here? Should we also patch the themes in core overriding this template?
Comment #2
stijnbe CreditAttribution: stijnbe commentedInitial patch for this issue. What would be the best solution to use here? Should we also patch the themes in core overriding this template?
Comment #3
JacineTagging!
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedWhitespace.
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.
Comment #5
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis 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.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedWhat 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.
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commentedWhat 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.Comment #8
stijnbe CreditAttribution: stijnbe commentedImproved my patch using your comments. Is there already a global function for theming publication time and date?
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commentedTheme 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?
I don't mean to be picky but shouldn't this be:
26 days to next Drupal core point release.
Comment #10
JacineGood 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)
andrender($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.Comment #12
JacineAdding to current sprint, plus RDFa tests are failing... http://qa.drupal.org/pifr/test/163124
I guess we need some help with this. :s
Comment #13
stijnbe CreditAttribution: stijnbe commentedJacine, I will update the RDFa tests so they work with the new markup and create a new patch. Is this the way to go?
Comment #14
Jeff Burnz CreditAttribution: Jeff Burnz commentedWill 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.
Comment #15
JacineI don't know the answer here. I'll see if I can find someone who knows.
Comment #16
sunJust replace the xpath selectors with the new ones. That RDF module test specifically is about Comment module integration - nothing generic too worry about.
Comment #17
Jeff Burnz CreditAttribution: Jeff Burnz commentedYeah, 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.
Comment #18
sunTests 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
Comment #19
Everett Zufelt CreditAttribution: Everett Zufelt commentedBrowsing 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.
Comment #20
Everett Zufelt CreditAttribution: Everett Zufelt commentedComment #21
Jeff Burnz CreditAttribution: Jeff Burnz commentedEverett, 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.
Comment #22
Everett Zufelt CreditAttribution: Everett Zufelt commentedCan 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?
Comment #23
scor CreditAttribution: scor commentedRDFa does not rely on the elements but only on the attributes, so changing
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.
Comment #24
Everett Zufelt CreditAttribution: Everett Zufelt commentedOpened #1272870: No semantics for nested comments / bad for screen-readers
Comment #25
franzI'm not sure if this should be included in this issue, but $title_attributes and $content_attributes are missing documentation.
Comment #26
scor CreditAttribution: scor commented@franz: separate issue.
Comment #27
franzGarland also makes use of comment.tpl.php, although Garland might not make it to Drupal 8 release
Comment #28
dcmouyard CreditAttribution: dcmouyard commentedShouldn't
$title_prefix
and$title_suffix
be rendered outside the<header>
element?Comment #29
Everett Zufelt CreditAttribution: Everett Zufelt commentedWhat 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.
Comment #30
scor CreditAttribution: scor commentedIt's used for contextual links for example. agree with @dcmouyard, I don't think it makes sense to have these rendered inside
<header>
.Comment #31
Jacine$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.
Comment #32
scor CreditAttribution: scor commentedThere 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.
Comment #33
scor CreditAttribution: scor commentedmoved $title_prefix and $title_suffix lines outside the $title condition, which should solve both issues.
Comment #34
dcmouyard CreditAttribution: dcmouyard commentedI usually don't use
<header>
if it only contains a single heading. Is this mentioned in the coding standards anywhere?Comment #35
droplet CreditAttribution: droplet commentedNAV on the links ??
5 days to next Drupal core point release.
Comment #36
stevetweeddale CreditAttribution: stevetweeddale commentedTagging for the current HTML5 sprint
Comment #37
JacineTagging for the current sprint.
Comment #38
dcmouyard CreditAttribution: dcmouyard commentedI 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.
Comment #39
dcmouyard CreditAttribution: dcmouyard commentedHere’s how I would approach the markup:
Comment #40
JacineChanging script tags.
Comment #41
Jeff Burnz CreditAttribution: Jeff Burnz commentedAssigning me myself, preparing new shiny patch for BADCamp.
Comment #42
Jeff Burnz CreditAttribution: Jeff Burnz commented#33: comment-html5-1189816-33.patch queued for re-testing.
Comment #44
ericduran CreditAttribution: ericduran commentedThe test needs to be re-written to account for the changes.
Comment #45
ericduran CreditAttribution: ericduran commentedThis 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.
Comment #46
ericduran CreditAttribution: ericduran commentedJust sending it to the testbot FYI.
Comment #47
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis 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 usingem
, 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
inp
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 refactoredi
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.
Comment #48
ericduran CreditAttribution: ericduran commentedI'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:
Everything after the <?php should be indented. At least thats the pattern that seems to be everywhere else.
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.
Comment #49
scor CreditAttribution: scor commentedI 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.
Comment #50
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #51
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, 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.
Comment #52
Jeff Burnz CreditAttribution: Jeff Burnz commentedOps...
Comment #53
scor CreditAttribution: scor commentedyou missed that one too...
these need to be reindented back too.
Comment #54
Jeff Burnz CreditAttribution: Jeff Burnz commentedThanks 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.
Comment #55
scor CreditAttribution: scor commentedAs 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.
Comment #56
scor CreditAttribution: scor commentedComment #57
JacineThanks 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!
Comment #58
sun- Fixed indentation
- Removed paragraph around permalink.
Comment #59
JacineThanks @sun! Looks good.
Comment #60
JacineI'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.Comment #61
JacineI asked Bruce Lawson about this. This was the Twitter convo:
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.
Comment #62
JacineSame patch without the whitespace. Also, forgot to mention in the previous comment, the patch updates the comment that hides the links.
Comment #63
JacineSigh. One more time?
Comment #64
JacineFound 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 .
Comment #65
rasskull CreditAttribution: rasskull commentedI would like to make a suggestion to consider removing the nav element from the $links var
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.
Comment #66
JohnAlbinYeah,
<nav>
is only for major navigation.Also, related to the mention of pubdate in the initial issue description, there's this today… :-p
@html5doctor:
Comment #67
JacineOk, 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.Comment #68
JacineAlright, here's an updated patch that removes that
<nav>
element.Comment #69
rasskull CreditAttribution: rasskull commentedI 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?
Comment #70
rasskull CreditAttribution: rasskull commentedHere is the bit I added if you don't want to open up the patch
There could be some indenting issues here :/
Comment #71
JacinePersonally, 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>
? ;)Comment #72
rasskull CreditAttribution: rasskull commentedre-rolled with indentation fixed, removed a new line after first php tag and added clearfix to content div.
Comment #73
rasskull CreditAttribution: rasskull commentedJacine, 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 :)
Comment #74
Jeff Burnz CreditAttribution: Jeff Burnz commentedComment #75
Anonymous (not verified) CreditAttribution: Anonymous commentedSo which patch is the one that needs review here? I'm assuming Jacine's in #68, let me know if that is correct.
Comment #76
Jacine@linclark Oh, sorry! Yes, #68 is still up for review. Thanks Lin!
Comment #77
rasskull CreditAttribution: rasskull commentedyeah, ignore my patches for #69 and #72
Comment #78
JacineRe-rolled #68 due to the core directory change.
Comment #79
JacineOh 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
Comment #80
Everett Zufelt CreditAttribution: Everett Zufelt commentedCan 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.
Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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).
Comment #82
Everett Zufelt CreditAttribution: Everett Zufelt commentedRespectfully disagree. I still prefer references to W3C spec. No need for further off topic debate in this issue though.
Comment #83
rasskull CreditAttribution: rasskull commentedJacine, #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 :)
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous commentedDo comments really represent self-contained compositions that are independently distributable? Would you ever distribute a comments section without the node?
Comment #85
rasskull CreditAttribution: rasskull commentedThe following sentences after your quote from the W3C spec also say this:
So without getting into this issue too deeply here, it seems they specifically say that a user-submitted comment can be considered an 'article'.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedwell will you look at that....
objection withdrawn ;)
Comment #87
Michsk CreditAttribution: Michsk commentedOk so, i also don't want to push this to deep but:
This just isn't a comment, i know what w3c states but that piece above exactly describes how i see this.
Comment #88
JacineAlright... So, in the interest of not bike-shedding and moving this issue forward, here's my take on this.
On
<article>
as a wrapper...<article>
. See @linclark's comment in #86 and the spec itself.<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:
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.Comment #89
webchickYeah, 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. :)
Comment #90
JacineThis should actually be consistent with the node template.
Comment #91
mortendk CreditAttribution: mortendk commentedhmm is it me or do we have to groups of people working on this comment stuff: #1216976: Clean up the CSS for Comment module
Comment #92
Jacine@mortendk We should not. This one is about markup and the other is about CSS.
Comment #93
JacineHere's an updated patch that brings this more in line with the
node.tpl.php
template. With the patch applied it looks like this:This patch also changes the name of the
$picture
variable to$user_picture
, also to be consistent with the node template.Comment #94
jessebeach CreditAttribution: jessebeach commentedThis 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.
Comment #95
jessebeach CreditAttribution: jessebeach commentedBlarg, my changes were present. Reposting with my changes. Disregard the patch in #94.
Comment #96
dcmouyard CreditAttribution: dcmouyard commentedThe patch in #95 looks good to me!
Comment #97
droplet CreditAttribution: droplet commenteddon't we allow more space be inserted to promote readability.
comment permalink or node, it's not clear ?
$user_signature ?
24 days to next Drupal core point release.
Comment #98
dcmouyard CreditAttribution: dcmouyard commentedAccording 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.Comment #99
droplet CreditAttribution: droplet commented@dcmouyard,
I wouldn’t be opposed to it but see Function Calls part (coding standards) and other modules are allowed.
Comment #100
JacineTBH, 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.
Comment #101
Dries CreditAttribution: Dries commentedThis looks great. Committed to 8.x. Thanks all!
Comment #102
JacineYAY!! Thanks :D
Comment #103.0
(not verified) CreditAttribution: commentedAdd message regarding time and pubdate.