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.

Related: #1189816: Convert comment.tpl.php to HTML5

Files: 
CommentFileSizeAuthor
#73 1272870_73_bartik_comment_gone.patch1.8 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 46,452 pass(es).
[ View ]
#68 1272870_68_bartik_comment_gone.patch734 bytesscor
PASSED: [[SimpleTest]]: [MySQL] 46,382 pass(es).
[ View ]
#60 Semantic Relationship.png206.13 KBmgifford
#59 comment-1272870-59.patch13.83 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 46,244 pass(es).
[ View ]
#59 interdiff-59.txt3.19 KBxjm
#55 comment-1272870-55-tests.patch9.43 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 46,236 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#55 comment-1272870-55.patch13.8 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 46,239 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#55 interdiff.txt9.79 KBxjm
#50 comment-No_semantics_for_nested_comments-1272870-49.patch4.17 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 42,814 pass(es).
[ View ]
#48 ParentChildBartik.png418.56 KBmgifford
#48 StarkParentChild.png426.38 KBmgifford
#43 comment-No_semantics_for_nested_comments-1272870-43.patch3.94 KBDrupali
PASSED: [[SimpleTest]]: [MySQL] 42,812 pass(es).
[ View ]
#39 comment-parent-context-1272870-39.patch5.66 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-parent-context-1272870-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 language-link-1164682-36.patch3.06 KBmvc
PASSED: [[SimpleTest]]: [MySQL] 37,003 pass(es).
[ View ]
#36 comment-parent-context-1272870-36.patch5.64 KBmvc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-parent-context-1272870-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 comment-parent-context-1272870-34.patch8.32 KBmvc
FAILED: [[SimpleTest]]: [MySQL] 36,996 pass(es), 0 fail(s), and 328 exception(s).
[ View ]
#31 comment-parent-context-1272870-31.patch5.21 KBmvc
FAILED: [[SimpleTest]]: [MySQL] 36,988 pass(es), 0 fail(s), and 332 exception(s).
[ View ]

Comments

franz’s picture

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.

scor’s picture

@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).

Everett Zufelt’s picture

I have no preference if parent-child relationships are exposed in flat comment mode (IMO the reply link should be gone in that case). If parent-child is exposed visually then it should be exposed to screen-readers as well.

franz’s picture

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. =)

mgifford’s picture

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.

catch’s picture

Component:markup» comment.module

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.

danillonunes’s picture

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?)".

Everett Zufelt’s picture

Marked #1316648: Threaded comments should be nested as a duplicate of this issue.

Jeff Burnz’s picture

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.

mgifford’s picture

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/

dozymoe’s picture

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.

catch’s picture

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 Zufelt’s picture

@catch

I think that where there is a semantic relationship between comments, and that where we are exposing this visually to some users, but not textually to others, that we have a major issue.

I agree with you on threaded comments. That being said, perhaps I hate them because they are so incredibly difficult (impossible) for me to understand. Perhaps if I could know that X is in reply to Y I would love threaded comments (likely not, but I hope that helps to make the point).

I agree that there isn't anything to be backported, but that is because the changes will almost certainly be user facing string additions.

Note, that while nesting <article> may solve the semantic issue, it will certainly not solve the accessibility issue. Text strings will need to be used for that.

Jeff Burnz’s picture

@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.

yoroy’s picture

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?

xjm’s picture

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 Zufelt’s picture

@yoroy

Yes, something like "In reply to ..."

@xjm

I don't think the "in reply to" should be invisible. 1. I think almost nothing should be invisible and 2. Screen-reader users are not the only group that might find the string to increase accessibility / usability.

Bojhan’s picture

@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?

Jeff Burnz’s picture

@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.

xjm’s picture

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. =/

Michelle’s picture

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

Everett Zufelt’s picture

+1 on the ideas in 23 / 24

Jeff Burnz’s picture

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?

dozymoe’s picture

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.

xjm’s picture

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

mgifford’s picture

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?

mgifford’s picture

Issue tags:+a11ySprint

Adding tag for sprint

mvc’s picture

Status:Active» Needs review
StatusFileSize
new5.21 KB
FAILED: [[SimpleTest]]: [MySQL] 36,988 pass(es), 0 fail(s), and 332 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, comment-parent-context-1272870-31.patch, failed testing.

mvc’s picture

StatusFileSize
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 37,003 pass(es).
[ View ]

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.

mvc’s picture

Status:Needs work» Needs review
StatusFileSize
new8.32 KB
FAILED: [[SimpleTest]]: [MySQL] 36,996 pass(es), 0 fail(s), and 328 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, comment-parent-context-1272870-34.patch, failed testing.

mvc’s picture

Status:Needs work» Needs review
StatusFileSize
new5.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-parent-context-1272870-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

mgifford’s picture

Status:Needs review» Needs work
Issue tags:+accessibility, +a11ySprint

The last submitted patch, comment-parent-context-1272870-36.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new5.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-parent-context-1272870-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reroll

Status:Needs review» Needs work
Issue tags:-accessibility, -a11ySprint

The last submitted patch, comment-parent-context-1272870-39.patch, failed testing.

Drupali’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+accessibility, +a11ySprint

The last submitted patch, comment-parent-context-1272870-39.patch, failed testing.

Drupali’s picture

StatusFileSize
new3.94 KB
PASSED: [[SimpleTest]]: [MySQL] 42,812 pass(es).
[ View ]

Adding patch

Drupali’s picture

Status:Needs work» Needs review

changing status so that it gets reviewed by testbot.

Status:Needs review» Needs work

The last submitted patch, comment-No_semantics_for_nested_comments-1272870-43.patch, failed testing.

franz’s picture

Status:Needs work» Needs review

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

xjm’s picture

mgifford’s picture

Issue tags:+D8UX usability
StatusFileSize
new426.38 KB
new418.56 KB

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!

Bojhan’s picture

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.

mgifford’s picture

StatusFileSize
new4.17 KB
PASSED: [[SimpleTest]]: [MySQL] 42,814 pass(es).
[ View ]

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.

franz’s picture

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.

mgifford’s picture

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.

xjm’s picture

Issue tags:+Needs tests

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

xjm’s picture

Assigned:Unassigned» xjm
xjm’s picture

Assigned:xjm» Unassigned
Issue tags:-Needs tests
StatusFileSize
new9.79 KB
new13.8 KB
FAILED: [[SimpleTest]]: [MySQL] 46,239 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new9.43 KB
FAILED: [[SimpleTest]]: [MySQL] 46,236 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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).

xjm’s picture

Assigned:Unassigned» xjm

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.

Status:Needs review» Needs work

The last submitted patch, comment-1272870-55.patch, failed testing.

xjm’s picture

Assigned:xjm» Unassigned
Status:Needs work» Needs review
StatusFileSize
new3.19 KB
new13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 46,244 pass(es).
[ View ]

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

mgifford’s picture

StatusFileSize
new206.13 KB

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!

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

This looks ready to me.

webchick’s picture

Title:No semantics for nested comments / bad for screen-readers» Change notice: No semantics for nested comments / bad for screen-readers
Category:bug» task
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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.

webchick’s picture

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.

xjm’s picture

@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.

xjm’s picture

Assigned:Unassigned» xjm

I'll write this change notification.

xjm’s picture

Status:Active» Needs review
dead_arm’s picture

Title:Change notice: No semantics for nested comments / bad for screen-readers» No semantics for nested comments / bad for screen-readers
Category:task» bug
Priority:Critical» Major
Status:Needs review» Fixed
Issue tags:-Needs change record

The change notice is clear and thorough.

scor’s picture

Status:Fixed» Needs review
StatusFileSize
new734 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,382 pass(es).
[ View ]

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

xjm’s picture

Status:Needs review» Reviewed & tested by the community

Thanks @scor. Good catch.

xjm’s picture

Assigned:xjm» Unassigned
Status:Reviewed & tested by the community» Needs work

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

mgifford’s picture

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

I think the others already have inline documentation.

xjm’s picture

+++ 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.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 46,452 pass(es).
[ View ]

Ok, this time with inline docs.

Thanks for the clarification @xjm.

Status:Needs review» Needs work
Issue tags:-accessibility, -D8UX usability, -a11ySprint

The last submitted patch, 1272870_73_bartik_comment_gone.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
Issue tags:+accessibility, +D8UX usability, +a11ySprint

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

xjm’s picture

Status:Needs review» Reviewed & tested by the community

Great, thanks @scor and @mgifford!

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Rock, thanks!

Committed and pushed to 8.x.

mgifford’s picture

That's great news, thanks!

catch’s picture

Status:Fixed» Needs work

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?

xjm’s picture

Priority:Major» Normal

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.

mgifford’s picture

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

Berdir’s picture

mgifford’s picture

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

catch’s picture

Status:Needs work» Fixed
Issue tags:+revisit before beta

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.

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

Anonymous’s picture

Issue summary:View changes

Fixing typo

catch’s picture

Issue summary:View changes
Issue tags:-revisit before beta

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

mgifford’s picture

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

Tschet’s picture

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

mgifford’s picture

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:

andypost’s picture

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

mgifford’s picture

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.

terrill’s picture

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.

mgifford’s picture

@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!