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.
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
Comment | File | Size | Author |
---|---|---|---|
#61 | 2170251-61.patch | 5.75 KB | lauriii |
Comments
Comment #1
mgiffordForgot Bartik & Garland templates.
Comment #2
mgiffordAnd now for the D8 version.
Comment #4
mparker17Did a quick code-only review of both patches.
Both look good overall; but in the D8 patch,
There's a missing ">" on the closing H3 tag.
Comment #5
mparker17Fixed!
Comment #6
mgiffordDamn.. Thanks for the re-roll!
Comment #7
Bojhan CreditAttribution: Bojhan commentedWhat does this change visually?
Comment #8
mgiffordNothing that I am aware of. Empty headings don't seem to show up.
Comment #9
mgiffordNote, 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.
Comment #10
jessebeach CreditAttribution: jessebeach commentedtitle_prefix should be wrapped in its own if block as well.
Comment #11
mgiffordThat 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.
Comment #12
mgifford11: comment-heading-d8-2170251-11.patch queued for re-testing.
Comment #13
mgiffordComment #14
Jalandhar CreditAttribution: Jalandhar commentedRerolled patch.
Comment #15
mgiffordThis 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.
Comment #16
alexpottWhitespace
We should have a test for this.
Comment #17
oadaeh CreditAttribution: oadaeh commentedSetting test tag to the common one.
Comment #18
lokapujyaI 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.
Comment #19
lokapujyakick of the test.
Comment #20
mgiffordHmm.. 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.
Comment #21
drummComment #22
drummAs 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:
Comment #23
lokapujyaJust 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.
Comment #24
mgifford@drumm First I was hoping I could just toss this into Bartik by following this upgrade guide:
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...
Comment #25
lokapujyamgifford: Since it's just for testing, you can probably just set the $v['title'] without the surrounding if statement, using that code you posted.
Comment #26
mgiffordI'm willing to mark that RTBC now. With this I successfully overrode the title.
Not sure how much of a fringe case this is, but hopefully it will help get it into d.o...
Screenshots attached.
Comment #27
mgiffordWith title removed and Core:
With patched Core & title remove:
Comment #28
alexpottWe still need tests
Comment #29
YesCT CreditAttribution: YesCT commentedSo 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.)
Comment #30
lokapujyaHere is a proposed test to possibly go in the 'Comment interface' test:
It currently fails. After doing what YesCT said, it should pass, theoretically.
Comment #31
Mirabuck CreditAttribution: Mirabuck commentedTaking this on as part of Austin sprint.
Comment #32
Mirabuck CreditAttribution: Mirabuck commentedAdded 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.
Comment #33
mgiffordPatch 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:
Comment #34
alexpottLets combine the tests... you can enable the module during the test using
\Drupal::moduleHandler()->install()
Missing a new line
Comment #35
joshi.rohit100please review the submitted patch.
Comment #36
lokapujyaThe last patch removed the tests.
Comment #37
joshi.rohit100sorry my bad. Please review now
Comment #39
mgiffordgotta fix the tests..
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #42
lauriiiComment #43
lauriiiComment #45
iMiksuLooks good to me.
Comment #46
lokapujyaThis comment should say populated titles, not empty titles.
I think the assert messages should say what we are asserting, for example:
H3 for comment title element not found when no title present.
H3 element found when title present.
Comment #47
lauriiiFixed issues pointed in #46.
Comment #48
lokapujyaThanks lauriii.
Comment #49
lauriiiThis is still wrong.. I'm fixing that.
Comment #50
lauriiiComment #51
lauriiiComment #52
mgiffordLooks good to me. Back to RTBC.
Comment #53
alexpottShould be
Contains \Drupal\comment\Tests\CommentTitleTest.
Unnecessary use
Tests comment titles.
"created for comment titles"
// Enables module that sets comment titles to an empty string.
and verify that h3's are not rendered
No need to log in again
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."
with populated titles
No need to log in again.
Same thing other way around.
it is a standard that test modules end with _test so
comment_empty_title_test
Comment #54
lauriiiI made the changes @alexpott suggested in #53. Thank you for your feedback!
Comment #55
mgiffordI 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?
Comment #56
lokapujya$v could be expanded.
Comment #57
mgiffordTrue.. 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.
Comment #58
mgiffordComment #59
lokapujyaNot worth it. Anyway, I didn't find any other instances where $v was used for variables.
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.
Comment #60
lokapujyaRemoved the admin login.
Comment #61
lauriiiI changed the test modules name for the one that @alexpott suggested at #53.
Comment #62
lokapujyaI don't see anything different.
Comment #63
mgiffordcore/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.
Comment #64
lokapujyaComment #65
alexpottCommitted 70016d8 and pushed to 8.x. Thanks!