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.
Comment | File | Size | Author |
---|---|---|---|
#73 | 1272870_73_bartik_comment_gone.patch | 1.8 KB | mgifford |
#68 | 1272870_68_bartik_comment_gone.patch | 734 bytes | scor |
#60 | Semantic Relationship.png | 206.13 KB | mgifford |
#59 | comment-1272870-59.patch | 13.83 KB | xjm |
#59 | interdiff-59.txt | 3.19 KB | xjm |
Comments
Comment #1
franzhttp://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.
Comment #2
scor CreditAttribution: scor commented@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).
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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.
Comment #4
franzBut 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. =)
Comment #5
mgiffordIs 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.
Comment #6
catchFor 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.
Comment #7
danillonunes CreditAttribution: danillonunes commentedSuggested 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?)".
Comment #8
Everett Zufelt CreditAttribution: Everett Zufelt commentedMarked #1316648: Threaded comments should be nested as a duplicate of this issue.
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commentedIn 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:
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.
Comment #10
mgiffordNot 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/
Comment #11
dozymoe CreditAttribution: dozymoe commentedUm, 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.
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.
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.
Comment #12
catchAll 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).
Comment #13
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #14
Jeff Burnz CreditAttribution: Jeff Burnz commented@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.
Comment #16
yoroy CreditAttribution: yoroy commentedThreaded 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?
Comment #18
xjmWell, 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.Comment #19
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #21
Bojhan CreditAttribution: Bojhan commented@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?
Comment #22
Jeff Burnz CreditAttribution: Jeff Burnz commentedIn 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.
Comment #23
xjmSo, to clarify, we're thinking something like:
Possibly with some more semantic markup? Unfortunately that wouldn't be backportable. =/
Comment #24
MichelleThis 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
Comment #25
Everett Zufelt CreditAttribution: Everett Zufelt commented+1 on the ideas in 23 / 24
Comment #26
Jeff Burnz CreditAttribution: Jeff Burnz commentedMichelle, 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?
Comment #27
dozymoe CreditAttribution: dozymoe commentedMaybe 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.
Comment #28
xjmI guess we'd have to always have the full link rather than just the fragment to account for pagination.
Comment #29
mgiffordCan 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?
Comment #30
mgiffordAdding tag for sprint
Comment #31
mvcHere'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.
Comment #33
mvcgah, 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.
Comment #34
mvcoops -- i uploaded a patch for the wrong issue. too much code sprinting! trying again.
Comment #36
mvcI'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.
Comment #37
mgifford#36: comment-parent-context-1272870-36.patch queued for re-testing.
Comment #39
mgiffordreroll
Comment #41
Drupali CreditAttribution: Drupali commented#39: comment-parent-context-1272870-39.patch queued for re-testing.
Comment #43
Drupali CreditAttribution: Drupali commentedAdding patch
Comment #44
Drupali CreditAttribution: Drupali commentedchanging status so that it gets reviewed by testbot.
Comment #46
franzThe error doesn't seem at all related (base table field_config doesn't exist). So let's try again.
Comment #47
xjm#43: comment-No_semantics_for_nested_comments-1272870-43.patch queued for re-testing.
Comment #48
mgiffordPatch 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!
Comment #49
Bojhan CreditAttribution: Bojhan commentedYhea, 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.
Comment #50
mgiffordOk, 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:
Comment #51
franzThere 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.
Comment #52
mgiffordAgreed 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.
Comment #53
xjmI'll clean this up a bit and write a test.
Comment #54
xjmComment #55
xjmAttached:
Edit: Sorry, the interdiff below is wrong (against an intermediate commit).
Comment #57
xjmGot so into the test I forgot about the rest of the code style errors:
This else doesn't follow our coding standards.
This first line is over 80 chars. Also $comment_parent also needs a bullet.
Comment #59
xjmCleans up the points mentioned in #58 and corrects a typo.
Comment #60
mgiffordApplies nicely & provides the the desired output.
Thanks @xjm!
Comment #61
tim.plunkettThis looks ready to me.
Comment #62
webchickHm. 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.
Comment #63
webchickHm. 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.
Comment #64
xjm@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.
Comment #65
xjmI'll write this change notification.
Comment #66
xjmhttp://drupal.org/node/1826444
Comment #67
dead_armThe change notice is clear and thorough.
Comment #68
scor CreditAttribution: scor commentederr, the hunk for Bartik's comment.tpl.php didn't get committed.
Comment #69
xjmThanks @scor. Good catch.
Comment #70
xjmOh actually we also need the documentation of the variables up top as well.
Comment #71
mgiffordThat's just
$parent
in comment.tpl.php, right?I think the others already have inline documentation.
Comment #72
xjmSorry, just meant that this hunk needs to be added in bartik's template as well.
Comment #73
mgiffordOk, this time with inline docs.
Thanks for the clarification @xjm.
Comment #75
mgifford#73: 1272870_73_bartik_comment_gone.patch queued for re-testing.
Comment #76
xjmGreat, thanks @scor and @mgifford!
Comment #77
webchickRock, thanks!
Committed and pushed to 8.x.
Comment #78
mgiffordThat's great news, thanks!
Comment #79
catchJust saw this one:
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?
Comment #80
xjmDowngrading for the followup.
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.
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.
Comment #81
mgiffordWould be good to clean this up in the sprint this weekend if there is time.
Comment #82
BerdirThere'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.
Comment #83
mgiffordAny thoughts on how to deal with this? I guess we should follow up in the other patch.
Comment #84
catchOpened #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.
Comment #85.0
(not verified) CreditAttribution: commentedFixing typo
Comment #86
catchEntity render caching is in and we still have #1857946: Comment parent template variables are built twice open. Untagging.
Comment #87
mgifford@catch we could consider using aria-owns to provide that relationship.
Comment #88
Tschet CreditAttribution: Tschet commentedI agree @mgifford, the aria-owns approach seems like a good approach to this.
Comment #89
mgiffordWe 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:
Comment #90
andypost@mgifford Maybe better to file a follow-up issue?
Comment #91
mgiffordReally 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.
Comment #92
terrill CreditAttribution: terrill commentedSorry 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.
Comment #93
mgifford@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!