As discussed here #2095969: Accessibility problem with headings in issue queue there shouldn't be blank headings in comments.

We can just check to see that a title exists though before inserting the <h3>.

Remaining tasks:

  • (done) manual testing see comments #18-37
  • (done) add tests Instructions, test should add a module that adds a preprocess function so this is possible to test.
  • Review and verify tests added in #32
CommentFileSizeAuthor
#61 2170251-61.patch5.75 KBlauriii
#60 interdiff.txt1.14 KBlokapujya
#60 2170251-60.patch5.76 KBlokapujya
#58 comment-a11y_tests_for_empty_titles-2170251-58.patch5.89 KBmgifford
#57 comment-a11y_tests_for_empty_titles-2170251-57.patch1.44 KBmgifford
#54 interdiff-2170251-50-54.txt6.21 KBlauriii
#54 comment-a11y_tests_for_empty_titles-2170251-54.patch5.88 KBlauriii
#50 comment-a11y_tests_for_empty_titles-2170251-50.patch5.98 KBlauriii
#50 interdiff-2170251-47-50.txt458 byteslauriii
#47 comment-a11y_tests_for_empty_titles-2170251-47.patch5.99 KBlauriii
#43 comment-a11y_tests_for_empty_titles-2170251-42.patch5.98 KBlauriii
#43 comment-a11y_tests_for_empty_titles-2170251-42-test.patch4.54 KBlauriii
#37 comment-a11y_tests_for_empty_titles-2170251-37.patch5.3 KBjoshi.rohit100
#35 interdiff-2170251-34.txt4.56 KBjoshi.rohit100
#35 comment-a11y_tests_for_empty_titles-2170251-34.patch1.44 KBjoshi.rohit100
#33 Selection_010.png61.4 KBmgifford
#32 comment-a11y_tests_for_empty_titles-2170251-32.patch6.8 KBMirabuck
#26 Patch-With-No-Title-in-Comments.png46.05 KBmgifford
#26 Forced-No-Title-Core.png48.72 KBmgifford
#18 2170251-18.patch1.44 KBlokapujya
#14 comment-heading-d8-2170251-14.patch1.5 KBJalandhar
#11 comment-heading-d8-2170251-11.patch1.47 KBmgifford
#5 comment-heading-d8-2170251-5.patch1.26 KBmparker17
#5 interdiff.txt518 bytesmparker17
#2 comment-heading-d8-2170251-2.patch1.26 KBmgifford
#1 comment-heading-d7-2170251-1.patch1.98 KBmgifford
comment-heading-d7.patch595 bytesmgifford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

Forgot Bartik & Garland templates.

mgifford’s picture

Version: 7.x-dev » 8.x-dev
FileSize
1.26 KB

And now for the D8 version.

The last submitted patch, comment-heading-d7.patch, failed testing.

mparker17’s picture

Status: Needs review » Needs work

Did a quick code-only review of both patches.

Both look good overall; but in the D8 patch,

+++ b/core/modules/comment/templates/comment.html.twig
@@ -71,9 +71,11 @@
+    <h3{{ title_attributes }}>{{ title }}</h3

There's a missing ">" on the closing H3 tag.

mparker17’s picture

Status: Needs work » Needs review
FileSize
518 bytes
1.26 KB

Fixed!

mgifford’s picture

Damn.. Thanks for the re-roll!

Bojhan’s picture

What does this change visually?

mgifford’s picture

Nothing that I am aware of. Empty headings don't seem to show up.

mgifford’s picture

Note, that we might want to take a slightly different approach as we've recommended here #2095969: Accessibility problem with headings in issue queue

Essentially, the comment #1's can be used for navigation for screen readers. I haven't thought through the general implications for Drupal Core yet.

jessebeach’s picture

+++ b/core/modules/comment/templates/comment.html.twig
@@ -71,9 +71,11 @@
-  <h3{{ title_attributes }}>{{ title }}</h3>
+  {% if title %}
+    <h3{{ title_attributes }}>{{ title }}</h3>
+    {{ title_suffix }}
+  {% endif %}

title_prefix should be wrapped in its own if block as well.

mgifford’s picture

That was so way up high.. Totally missed the prefix. Thanks Jesse!

There's no permalink, so the extra stuff done for Bluecheese isn't needed for Core.

mgifford’s picture

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Novice
Jalandhar’s picture

Status: Needs work » Needs review
FileSize
1.5 KB

Rerolled patch.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

This is a pretty simple patch to address an outstanding accessibility problem. It presents no UI changes. I don't see how the additional conditional statements would have much impact on performance either.

Thanks for the re-roll @Jalandhar.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs a test
+++ b/core/themes/bartik/templates/comment.html.twig
@@ -101,10 +101,12 @@
+    ¶

Whitespace

We should have a test for this.

oadaeh’s picture

Issue tags: -Needs a test +Needs tests

Setting test tag to the common one.

lokapujya’s picture

Issue tags: -Needs reroll
FileSize
1.44 KB

I will get the re-roll out of the way. I was going to write a test.

How can this be tested manually? I don't even see titles for comments; I see subject. If I don't put a subject, then comment automatically uses the body as the subject.

lokapujya’s picture

Status: Needs work » Needs review

kick of the test.

mgifford’s picture

Hmm.. I'm not sure how to replicate the error in D8.. You can see lots of examples in D7 on this page mind you.

@lokapujya Good question.... Maybe it isn't possible in D8.

drumm’s picture

Issue tags: +affects drupal.org
drumm’s picture

As far as I know, this is only reproducible via custom code, on both D7 and D8. When you hide the comment subject field, the subject is auto-filled with a truncated comment. The truncated subject is redundant and good to remove. For D7, we use:

/**
 * Implements hook_preprocess_comment().
 */
function drupalorg_preprocess_comment(&$v) {
  // Force the comment title to be empty for node types with a disabled comment
  // title field, such as project_issue.
  if (!variable_get('comment_subject_field_' . $v['node']->type, 1)) {
    $v['title'] = '';
  }
}
lokapujya’s picture

Just checking... Do we still want the subject to get auto-filled when the subject is disabled? If I disable an Article title, the Article title does not get auto-filled. Although, there might be a good reason comment behaves differently.

mgifford’s picture

@drumm First I was hoping I could just toss this into Bartik by following this upgrade guide:

/**
* Implements hook_preprocess_comment(). 
*/
function bartik_preprocess_comment(&$v) { 
  // Force the comment title to be empty for node types with a disabled comment
  if (\Drupal::config('comment_subject_field_' . $v['node']->type)->get(1)) {
    $v['title'] = '';
  }
}

No such luck. Then I figured

In D8 though it looks like the only preprocess_comment on offer is template_preprocess_comment

Which gave me:
( ! ) Fatal error: Class 'Element' not found in /var/www/drupal8/core/themes/bartik/bartik.theme on line 195

So I'm missing how to best test this using the D7 example.. It shouldn't be that hard, but...

lokapujya’s picture

mgifford: Since it's just for testing, you can probably just set the $v['title'] without the surrounding if statement, using that code you posted.

mgifford’s picture

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

I'm willing to mark that RTBC now. With this I successfully overrode the title.

function bartik_preprocess_comment(&$v) {
    $v['title'] = '';
}

Not sure how much of a fringe case this is, but hopefully it will help get it into d.o...

Screenshots attached.

mgifford’s picture

With title removed and Core:

Screenshot with firebug

With patched Core & title remove:
Screenshot with firebug

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We still need tests

YesCT’s picture

Issue summary: View changes

So it looks like comments #18-37 did manual testing.

We can add tests for this if:
the test adds a module that adds the preprocess function (... and then makes two comments, one with and without title? I'm not totally sure about that bit.)

lokapujya’s picture

Here is a proposed test to possibly go in the 'Comment interface' test:

<?php
    // Tests that markup is not created for blank comment headings.
    $this->assertNoFieldByXPath("//article[contains(@class, 'comment')]/h3", NULL, 'No h3 element found.');
?>

It currently fails. After doing what YesCT said, it should pass, theoretically.

Mirabuck’s picture

Assigned: Unassigned » Mirabuck

Taking this on as part of Austin sprint.

Mirabuck’s picture

Assigned: Mirabuck » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.8 KB

Added suggested module and tests for both populated and unpopulated comment titles. Ended up separating this into two tests as I don't believe we can toggle the helper module on and off within the scope of a single test. I've done simple test work in D7 before, but it looks like file placement has changed in D8. Someone should verify that I've put everything in the appropriate place. Patch includes template changes from #18.

mgifford’s picture

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

Patch looks good to me. By forcing a blank title for the comment (as is done in drupal.org) we could replicate a blank header and it is much more accessible:

Comments with no title

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/comment/src/Tests/CommentEmptyTitlesTest.php
    @@ -0,0 +1,56 @@
    +  function testCommentEmptyTitles () {
    
    +++ b/core/modules/comment/src/Tests/CommentPopulatedTitlesTest.php
    @@ -0,0 +1,51 @@
    +  function testCommentPopulatedTitles () {
    

    Lets combine the tests... you can enable the module during the test using \Drupal::moduleHandler()->install()

  2. +++ b/core/modules/comment/tests/modules/comment_test_empty_titles/comment_test_empty_titles.info.yml
    @@ -0,0 +1,8 @@
    +  - comment
    \ No newline at end of file
    
    +++ b/core/modules/comment/tests/modules/comment_test_empty_titles/comment_test_empty_titles.module
    @@ -0,0 +1,16 @@
    +}
    \ No newline at end of file
    

    Missing a new line

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
4.56 KB

please review the submitted patch.

lokapujya’s picture

Status: Needs review » Needs work

The last patch removed the tests.

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
5.3 KB

sorry my bad. Please review now

Status: Needs review » Needs work

The last submitted patch, 37: comment-a11y_tests_for_empty_titles-2170251-37.patch, failed testing.

mgifford’s picture

gotta fix the tests..

Anonymous’s picture

Assigned: Unassigned »
Anonymous’s picture

Assigned: » Unassigned
lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
4.54 KB
5.98 KB

The last submitted patch, 43: comment-a11y_tests_for_empty_titles-2170251-42-test.patch, failed testing.

iMiksu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

lokapujya’s picture

+++ b/core/modules/comment/src/Tests/CommentTitleTest.php
@@ -0,0 +1,81 @@
+   * Tests markup for comments with empty titles.

This comment should say populated titles, not empty titles.

    I think the assert messages should say what we are asserting, for example:

  1. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    // Tests that markup is not generated for the comment without header.
    +    $this->assertNoPattern('|<h3[^>]*></h3>|', 'H3 for comment title element found when no title present.');
    
  2. H3 for comment title element not found when no title present.

  3. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    // Tests that markup is created for comment with heading.
    +    $this->assertPattern('|<h3[^>]*><a[^>]*>' . $subject_text . '</a></h3>|', 'H3 element not found when title present.');
    
  4. H3 element found when title present.

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.99 KB

Fixed issues pointed in #46.

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks lauriii.

lauriii’s picture

Assigned: Unassigned » lauriii
Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/src/Tests/CommentTitleTest.php
@@ -0,0 +1,81 @@
+ * Definition of Drupal\comment\Tests\CommentEmptyTitlesTest.

This is still wrong.. I'm fixing that.

lauriii’s picture

Status: Needs work » Needs review
FileSize
458 bytes
5.98 KB
lauriii’s picture

Assigned: lauriii » Unassigned
mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    + * Definition of Drupal\comment\Tests\CommentTitleTest.
    

    Should be Contains \Drupal\comment\Tests\CommentTitleTest.

  2. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +use Drupal\Core\Language\Language;
    

    Unnecessary use

  3. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    + * Tests comment with empty titles.
    

    Tests comment titles.

  4. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +      'description' => 'Test to ensure that appropriate and accessible markup is created to titles.',
    

    "created for comment titles"

  5. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    // Enables module that sets comments to = ''.
    

    // Enables module that sets comment titles to an empty string.

  6. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    // Post comment #1 and verify that title is rendered in h3.
    

    and verify that h3's are not rendered

  7. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    $this->drupalLogin($this->web_user);
    

    No need to log in again

  8. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    $this->assertNoPattern('|<h3[^>]*></h3>|', 'H3 for comment title element found when no title present.');
    

    The test message should be for the assertion not the negative case. So something like "Comment title H3 element not found when title is an empty string."

  9. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +  /**
    +   * Tests markup for comments with empty titles.
    +   */
    +  public function testCommentPopulatedTitles() {
    

    with populated titles

  10. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    $this->drupalLogin($this->web_user);
    

    No need to log in again.

  11. +++ b/core/modules/comment/src/Tests/CommentTitleTest.php
    @@ -0,0 +1,81 @@
    +    $this->assertPattern('|<h3[^>]*><a[^>]*>' . $subject_text . '</a></h3>|', 'H3 element not found when title present.');
    

    Same thing other way around.

  12. +++ b/core/modules/comment/templates/comment.html.twig
    --- /dev/null
    +++ b/core/modules/comment/tests/modules/comment_test_empty_titles/comment_test_empty_titles.info.yml
    
    +++ b/core/modules/comment/tests/modules/comment_test_empty_titles/comment_test_empty_titles.info.yml
    --- /dev/null
    +++ b/core/modules/comment/tests/modules/comment_test_empty_titles/comment_test_empty_titles.module
    

    it is a standard that test modules end with _test so comment_empty_title_test

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
6.21 KB

I made the changes @alexpott suggested in #53. Thank you for your feedback!

mgifford’s picture

I went through the latest patch. The only discrepancy I could find was alexpott suggested:

comment_empty_title_test.module
comment_empty_title_test_preprocess_comment()

And @lauriii implemented:
comment_empty_titles_test.module
comment_empty_titles_test_preprocess_comment()

The code is works and I'm happy with it as it is now. Can we mark it RTBC?

lokapujya’s picture

+++ b/core/modules/comment/tests/modules/comment_empty_titles_test/comment_empty_titles_test.module
@@ -0,0 +1,16 @@
+function comment_empty_titles_test_preprocess_comment(&$v) {
+  $v['title'] = '';
+}

$v could be expanded.

mgifford’s picture

True.. Good catch.

EDIT: Is it worth opening up another issue for places where $v is used like this in Core?

EDIT2: Gotta re-roll the patch, sorry.

mgifford’s picture

lokapujya’s picture

EDIT: Is it worth opening up another issue for places where $v is used like this in Core?

Not worth it. Anyway, I didn't find any other instances where $v was used for variables.

+++ b/core/modules/comment/src/Tests/CommentTitleTest.php
@@ -0,0 +1,77 @@
+    // Set comments to have a subject with preview disabled.
+    $this->drupalLogin($this->admin_user);
+    $this->setCommentPreview(DRUPAL_DISABLED);
+    $this->setCommentForm(TRUE);
+    $this->setCommentSubject(TRUE);
+    $this->drupalLogout();

I know that other comment tests login, but I think it might not be required. I was able to pass the tests without it.

EDIT: if we do remove the login, it's in both tests.

lauriii’s picture

FileSize
5.75 KB

I changed the test modules name for the one that @alexpott suggested at #53.

lokapujya’s picture

I don't see anything different.

mgifford’s picture

core/modules/comment/tests/modules/comment_empty_title_test/comment_empty_title_test.module

vs

core/modules/comment/tests/modules/comment_empty_titles_test/comment_empty_titles_test.module

Easy to miss. Bullet #11.

This should be RTBC at this point.

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 70016d8 and pushed to 8.x. Thanks!

  • alexpott committed 70016d8 on 8.x
    Issue #2170251 by lauriii, mgifford, joshi.rohit100, lokapujya,...

Status: Fixed » Closed (fixed)

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