No semantics for nested comments / bad for screen-readers

http://stackoverflow.com/questions/4825290/html5-semantic-threaded-comments exemplifies 2 approaches for this. I believe the 'B' approach is simpler and can be done on comment.tpl.php.

@Everett: How would you cover the case where nested comments are displayed in flat chrono order? Would you expect a screen reader to be able to somehow still be aware of the parent-child relationship even though it's not displayed as such in the page? (via some markup like in the stackoverflow example B).

But if we do implement B from the link above, wouln't be worth to make 2 use cases out of it, provided the choice for ordering is according to the setting, and screen reader would get the relationship as a bonus. =)

Is there a use case for not displaying parent-child relationships in comments that isn't based on design?

If you can reply to an individual comment, is there any reason why keeping that relationship in code would be a detriment to the the site? It could be easily over-ridden in a custom template by the sounds of it.

I'd suggest we provide the proper semantics in either case & only alter the display of it in the threaded/flat choice which is offered to administrators.

For flat comments see #81373: Allow option for comments to be completely flat (no threading in database). I don't care either way if we show the semantics but that is one of the oldest comment module bugs in the queue.

Suggested approaches in #1 are good choices for D8 + HTML5.

But how this can be done in a markup agnostic way? Can we replicate this with just text?

Maybe nested comments can have a (hidden?) text with something like "in reply to comment above", and the zero level ones can have a "in reply to article (or node? or post?)".

In terms of showing nesting using markup there are two options:

Firs we could actually nest each child comment inside the parent, there are some remarks on the stackoverflow page regarding the semantics of this however I personally need to re-read the spec, this is one idea:

<article class="comment">
  <article class="comment reply threaded depth-1">
    <article class="comment reply threaded depth-2">
       <article class="comment reply threaded depth-3">
      </article>
    </article>
  </article>
</article>

Problem with that approach is that right now screenreaders probably won't understand it and not recognize any actual relationship, so I would perhaps push this back to an accessibility discussion and want to know what can we do that will actually work? I have always like the embedded link idea, fwiw.

Not as much on this as I'd like for sure. Certainly blogs aren't a new idea, but somehow there doesn't yet seem to be a best practice out there. Nesting seems like the best approach, but not sure. Found this post here which also had an element on comments:
http://html5doctor.com/designing-a-blog-with-html5/

Um, you only need to wrap comment inside <article>, if you plan to use <address> or <time pubtimepubdate>.

Just one of my random comments. 0.0b

Edit: added more contexts

• Inclusion of pubdate html attribute in comment requires article html element, otherwise the pubdate's time element will mark the publish date of the entire page.

  Indicates that the date and time given by the element is the publication date and time of the nearest ancestor article element — or, if the element has no ancestor article element, of the document as a whole.

  Source is from pubdate specs in w3.org.

• While the inclusion of address html attribute in comment is fine with just the sectioning elements, I think that means either article or section.

  If an address element applies to a body element, then it represents contact information for the document as a whole. If an address element applies to a section of a document, then it represents contact information for that section only.

  Source is from address specs in w3.org.

HTML5 elements is mostly aimed at use-case, hardly the naming, for example footer can be printed above the section's content.

Disclaimer:

Those were my own interpretation of the specs, so, CMIIW.

Dunno why the links to dev.w3.org shows on the first page of google result XD. Was looking for the released version.

All the comments here are about HTML5, but this is marked as a major bug report. Is there an approach that's backportable to Drupal 7?

If not and it's won't fix for D7, I'd question the major priority here (although that might be my personal dislike of threaded comments biasing me against anything that has anything to do with them).

@Everett, when you say text strings, are you suggesting something like "in reply to" type strings? Sorry if I missed this in an earlier comment.

Threaded comments *are* confusing to most. You don't see them used often and reading this it looks like a, difficult feature to support in an accessible way. I would suggest to consider simplefying what core offers out of box here.

Would 'in reply to (linked comment number)' work?

Well, a case could be made for adding user-facing strings for backport (but not changing existing ones). The untranslated strings would be in an element hidden from most users, and the accessibility gain is probably more important. We've precedent for such a backport in #1008580: Fix image references in forum.css.

I am fairly certain there is no backportable markup fix for this. Even if we do use XHTML-compatible markup rather than HTML5, it's still a markup change that would wreak havoc on themes.

The two markup elements I can think of that properly indicate a hierarchical relationship in a way that AT supports are nested lists (<ul> and <ol>). Not sure if that is bending semantics too much or not.

@Everett I wonder if that would solve it though, because it requires the cognitive strain of remembering the title/author of the previous comment. Going down a big tree of nested comments this seems even more difficult.

@xjm It doesn't have to be invisisble, the comment title can be appended or even replaced by this - we can design it so it creates no additional clutter.

I do wonder what best-practices are in this, how do others solve this?

@Everett I wonder if that would solve it though, because it requires the cognitive strain of remembering the title/author of the previous comment. Going down a big tree of nested comments this seems even more difficult.

In typical forum software such things are hyper-linked to allow jumping to the replied-to comment. If this uses normal named anchors then once you are familiar with this system (almost all forum software is like this, so its common) you know to hit the "back button" to go back to the comment you were reading in the first place.

So, to clarify, we're thinking something like:

<div class="something">(In reply to <a href="#comment-1231231">comment 10</a>)</div>

Possibly with some more semantic markup? Unfortunately that wouldn't be backportable. =/

This is probably scope creep so I'm just tossing it out there as something to think about and, if there's any interest, it likely needs its own issue. Anyway, I recently saw a forum where the "in reply to" also included the user name that was the author of the post being replied to. I thought that was pretty handy as it makes it easy to see if someone is responding to you without having to remember (or jump to look) that you posted comment #42. Seems like that would be handy for blind folks as well as there's less jumping around to know when someone is talking to you.

Michelle

Michelle, Simple Machines forum software does something similar to that, except its on quotations. Its a nice feature.

xjm - I think so, how could/does this work with paginated comments?

Maybe we could have a BBCODE-like solution in the input format like [@user#15].

In the case the comment is not inserted by means of the Drupal GUI, or for contributed module to provide dropdown, something like facebook @ thingy.

I guess we'd have to always have the full link rather than just the fragment to account for pagination.

Can someone roll up a patch? It should have a full page link rather than a fragment I feel.

@Jeff any sample output from SimpleMachines to share?

@dozymoe - likewise with BBCODE?

What are the best practices out there to add semantics to comments?

Adding tag for sprint

Here's a patch which adds a bunch of parent comment information to the $variables array in template_preprocess_comment(), then prints a summary of this in core/modules/comment/comment.tpl.php. I also added it to bartik's comment.tpl.php (the only other one which ships with core).

This now works with all of core's themes. There may well be a prettier way to do this, but I'll leave that to a designer/ux person to improve. Now that this information is actually passed to the template people can present this however they like in their own themes.

gah, i was testing without E_ALL. let's try that again, but this time check that our variable exists...

EDIT: please ignore, i uploaded some other issue's patch by mistake.

oops -- i uploaded a patch for the wrong issue. too much code sprinting! trying again.

I'll just carry on my little chat with the test bot here -- I'm now defining all these variables as empty strings, like $variables['new'], to avoid warnings.

#36: comment-parent-context-1272870-36.patch queued for re-testing.

reroll

#39: comment-parent-context-1272870-39.patch queued for re-testing.

Adding patch

changing status so that it gets reviewed by testbot.

The error doesn't seem at all related (base table field_config doesn't exist). So let's try again.

#43: comment-No_semantics_for_nested_comments-1272870-43.patch queued for re-testing.

Patch applies nicely. Seems to work well in my testing. Attaching two screenshots.

Think we just need to bring in the UX folks to see if we can add it as is or if it needs to be made invisible for screen readers.

Great work though!

Yhea, although we try to avoid using invisible anywhere - it feels like this text is 99% of the time going to be only beneficial for screenreaders and adds a lot of visual clutter for non-screenreading-users. If you are all oke with doing element invisible here, that'd would be great as I have no idea how to fit this into Bartik/Stark.

This will probally also need a good documentation note somewhere, why its there - because I imagine this will puzzle most themers.

Ok, here's one with some inline docs & also with element-invisible attached.

I'm not sure this is the best inline docs, but it's a start:

// Screen reader users require semantic relationships to be meaningful.

There is some trailing whitespace in this last patch. About the message, how about:

"Screen reader users require meaningful semantic relationships in threaded comments, thus it is important to link this comment with it's parent."

Not so sure about it, but I think it explains a little more.

Agreed that this is better. I wanted to try to keep it under 80 characters so that we wouldn't have to wrap the line.

Would be pretty cool if you could just write a very short description and then use a tiny url to point folks to the full docs.

Let's try to settle on the language quickly and then get a new patch out.

I'll clean this up a bit and write a test.

Attached:

• Clarifies the inline comment in the comment templates.
• Removes trailing whitespace.
• Adds test coverage asserting that the parent links are correct for various comment threading.

Edit: Sorry, the interdiff below is wrong (against an intermediate commit).

Got so into the test I forgot about the rest of the code style errors:

+++ b/core/modules/comment/comment.moduleundefined
@@ -1657,6 +1657,29 @@ function template_preprocess_comment(&$variables) {
+  } else {
+    $variables['parent_comment'] = '';
+    $variables['parent_author'] = '';
+    $variables['parent_created'] = '';
+    $variables['parent_changed'] = '';
+    $variables['parent_title'] = '';
+    $variables['parent_permalink'] = '';

This else doesn't follow our coding standards.

+++ b/core/modules/comment/templates/comment.tpl.phpundefined
@@ -43,6 +43,19 @@
+ * These variables are provided to give context about the parent comment (if any):
+ * $comment_parent: Full parent comment object (if any).
+ * - $parent_author: Equivalent to $author for the parent comment.

This first line is over 80 chars. Also $comment_parent also needs a bullet.

Cleans up the points mentioned in #58 and corrects a typo.

Applies nicely & provides the the desired output.

            <p class="comment-parent element-invisible">
        In reply to <a href="/comment/13#comment-13" class="permalink" rel="bookmark">Child Comment</a> by <a href="/user/1" title="View user profile." class="username" lang="" about="/user/1" typeof="sioc:UserAccount" property="foaf:name">openconcept</a>      </p>

Thanks @xjm!

This looks ready to me.

Hm. A little nervous to put that sort of 4-line comment in the middle of a tpl.php file, but OTOH it's capturing pretty important information that would be difficult to discern otherwise. I guess we'll see if it survives the Twig conversion. :)

Committed and pushed to 8.x. Thanks!

This will need a change notice.

Hm. Note that #1810710: Remove copying of node properties wholesale into theme variables is taking the exact opposite approach , and removing one-off variables in favour of directly accessing them off of the object directly. Can folks here take a look? We should come up with one way that we standardize on across all templates.

@webchick, I thought of that too, but if you look closely it's actually a different issue. What node does is take the node properties and smoosh all of them into variables unilaterally. That's actually different from what this patch does, which is just making the parent comment object available like the main comment object already is.

I'll write this change notification.

http://drupal.org/node/1826444

The change notice is clear and thorough.

err, the hunk for Bartik's comment.tpl.php didn't get committed.

Thanks @scor. Good catch.

Oh actually we also need the documentation of the variables up top as well.

That's just $parent in comment.tpl.php, right?

I think the others already have inline documentation.

+++ b/core/modules/comment/templates/comment.tpl.phpundefined
@@ -43,6 +43,20 @@
+ * These variables are provided to give context about the parent comment (if
+ * any):
+ * - $comment_parent: Full parent comment object (if any).
+ * - $parent_author: Equivalent to $author for the parent comment.
+ * - $parent_created: Equivalent to $created for the parent comment.
+ * - $parent_changed: Equivalent to $changed for the parent comment.
+ * - $parent_title: Equivalent to $title for the parent comment.
+ * - $parent_permalink: Equivalent to $permalink for the parent comment.
+ * - $parent: A text string of parent comment submission information created
+ *   from $parent_author and $parent_created during
+ *   template_preprocess_comment(). This information is presented to help
+ *   screen readers follow lengthy discussion threads. You can hide this from

Sorry, just meant that this hunk needs to be added in bartik's template as well.

Ok, this time with inline docs.

Thanks for the clarification @xjm.

#73: 1272870_73_bartik_comment_gone.patch queued for re-testing.

Great, thanks @scor and @mgifford!

Rock, thanks!

Committed and pushed to 8.x.

That's great news, thanks!

Just saw this one:

+  if ($comment->pid > 0) {
+    // Fetch and store the parent comment information for use in templates.
+    $comment_parent = comment_load($comment->pid);
+    $variables['parent_comment'] = $comment_parent;
+    $variables['parent_author'] = theme('username', array('account' => $comment_parent));
+    $variables['parent_created'] = format_date($comment_parent->created);
+    $variables['parent_changed'] = format_date($comment_parent->changed);
+    $uri_parent = $comment_parent->uri();
+    $uri_parent['options'] += array('attributes' => array('class' => 'permalink', 'rel' => 'bookmark'));
+    $variables['parent_title'] = l($comment_parent->subject, $uri_parent['path'], $uri_parent['options']);
+    $variables['parent_permalink'] = l(t('Parent permalink'), $uri_parent['path'], $uri_parent['options']);
+    $variables['parent'] = t('In reply to !parent_title by !parent_username',
+     

This means that if you're using threaded comments, then we're going to duplicate a lot of the preparation of the comment variables (format_date() twice, theme_username() for example), that's already been done when rendering the actual parent comment, assuming it's done on the same page. That's likely to be a noticeable regression when you have a lot of threaded comments on a page, for example on groups.drupal.org. We should look at finding way to re-use the variables created for the parent comment I think.

Also there are variables created here that never get used? Can we just remove those please?

Downgrading for the followup.

Also there are variables created here that never get used? Can we just remove those please?

My assumption is that the extra variables were added for anyone who wanted to theme the relationship differently than "In reply to foo by bar." One could equally override the preprocessing to that end, but as a non-themer I'm not sure how important it is to provide the variables in the template by default.

This means that if you're using threaded comments, then we're going to duplicate a lot of the preparation of the comment variables (format_date() twice, theme_username() for example), that's already been done when rendering the actual parent comment, assuming it's done on the same page. That's likely to be a noticeable regression when you have a lot of threaded comments on a page, for example on groups.drupal.org. We should look at finding way to re-use the variables created for the parent comment I think.

How would we go about this short of a bunch of static caching? As far as I know comments are themed individually and there's no theme hook/template to theme the comment list.

Would be good to clean this up in the sprint this weekend if there is time.

There's also another performance regression here, see #1844146: comment_load() call in template_preprocess_comment() re-loads comment parent entities because of disabled static cache.

Any thoughts on how to deal with this? I guess we should follow up in the other patch.

Opened #1857946: Comment parent template variables are built twice as a major follow-up to fix the double variable preparation. Moving this to fixed but I've added it to #1744302: [meta] Resolve known performance regressions in Drupal 8 and am tagging 'revisit before release' since we may want to consider rolling this back if these aren't fixed.

Entity render caching is in and we still have #1857946: Comment parent template variables are built twice open. Untagging.

@catch we could consider using aria-owns to provide that relationship.

I agree @mgifford, the aria-owns approach seems like a good approach to this.

We need to figure out if it's too late to revert the patch. it was committed 2 years ago now.

It's also important to know if it works with screen readers or not. It's not clear to me that it is well supported with assistive technology:

• http://stackoverflow.com/questions/6501876/does-jaws-support-the-aria-at...
• https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Tes...
• http://accessibleculture.org/articles/2013/02/not-so-simple-aria-tree-vi...

@mgifford Maybe better to file a follow-up issue?

Really the follow-up issue is #1857946: Comment parent template variables are built twice - this was just an exploration from #84 to see if this could be done in any other way.

It really doesn't look like there is, so there's not much else to say at this point. I should have stated that in the previous post.

Sorry to enter the conversation so late. Was any consideration given to using heading level to communicate reply depth? Currently "Comments" is h2, and the subject of each comment and reply is h3, regardless of depth. I think one could argue that a reply to a comment might warrant an h4, and a reply to that reply might warrant an h5, and so on through h6. With this approach a screen reader user could jump from reply to reply, and would be able to understand their relationships via the heading level.

One problem with this approach is that the comment/reply heading comes after the comment header for each comment (which contains the user name and data/time posted), so if screen readers are hopping from heading to heading they would miss that meta data, or worse- they would mistakenly associate it with the preceding reply. That might be a solvable problem though using aria-describedby to associate the comment header with the subject heading.

Regarding #89, aria-owns is not currently well supported by screen readers. Here's a test page.

@terrill I just opened this issue #2290043: Threaded Comments Should Use Heading Levels to Communicate Reply Depth

I do think it's something worth exploring.

Thanks for for the link to the test page you set up. Quite useful!