Move comment fragment identifier to the comment container

This looks good. +1 as long as anchors pointing to DIVs (rather than other A's) works in all major browsers - and I believe it does. Moving to 6.x-dev queue.

setting back to active now everything is .tpl.php-ified, not sure if this is still valid at all.

Is this issue still needed in current D6?

+1 here, the same problem is existing for Drupal 7, which should be more interesting. I hope the problem will NOT exist in Drupal 8 (or I will cry ;))

The problem is that there is absolutely NO CHANCE to alter the output because all the HTML gets bashed into the #prefix value together with the intendation, 'new' info, etc.!

This is really important and the current solution is a bit crappy :)

You can find the code in comment.module, line 955:

    // Add anchor for each comment.
    $prefix .= "<a id=\"comment-$comment->cid\"></a>\n";
    $build['#prefix'] = $prefix;

I'd suggest to add this kind of prefixes to a tpl file to make it better editable.

Thanks a lot.

This is still the case in D8.

Here is a patch which fixes it for D8. Against 8.0.1

Changed extension to .patch so that testbot will pick it up.

Reuploading after applying a fix.

Removed

Trying a git patch upload.

Status must be set to Needs review to trigger testbot

Multiple test fails. May not be a problem with the patch, maybe more that these tests use a method for detecting a comment exists which is disrupted by this patch, hence tests need adjusting.

Bump! Can someone RTBC this?

The test fails need to be addressed before the patch can be committed.

Please, don't check it, because only diff there

Hi all, there is interdiff & wip patch: todo some core templates and fixed tests by this log https://www.drupal.org/pift-ci-job/132672

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -76,7 +76,7 @@
-<article{{ attributes.addClass(classes) }}>
+<article{{ attributes.addClass(classes) }} id="comment-{{ comment_id }}">

+++ b/core/themes/stable/templates/content/comment.html.twig
@@ -65,7 +65,7 @@
-<article{{ attributes.addClass('js-comment') }}>
+<article{{ attributes.addClass('js-comment') }} id="comment-{{ comment_id }}">

interesting...
we can't change templates here
but can add "ID" via preprocess

suppose that should be like that

Thanks for working on this issue!

According to WCAG 2.0 rule 2.4.4 Link Purpose (In Context), we should not have links without any text or title. This change seem to me really important to clean markup and improve accessibility.

A lot of tests have moved during the last 2 years so this patch needs to be rerolled. Given the patch is easy, I'm tagging this issue as Novice to help newcomers to find something to start with.

Hi! Attaching reroll and interdiff.

Simple reroll after CommentPagerTest has been converted to BrowserTestBase in #2809467: Convert \Drupal\comment\Tests\CommentPagerTest to BrowserTestBase.

@DuaelFr your patch somehow affects composer files, please remove this hunks

Sometimes I just want to slap myself ;)
Thank you for your awareness

This looks good.

This change should have a change record as the change shows tests are often affected and we're changing markup - which is allowed but documenting it is good.

Draft CR created.
https://www.drupal.org/node/3020137

+++ b/core/modules/comment/comment.module
@@ -724,6 +724,8 @@ function template_preprocess_comment(&$variables) {
+  $variables['attributes']['id'] = 'comment-' . $comment->id();

We should ensure that the id is unique by running this through Drupal\Component\Utility\Html::getUniqueId.

@lauriii I'm not sure about that - we should these in anchor links elsewhere on the page - I think it is up to the user not to output the comment twice on the same page.

@lauriii Also as that is more of a change - ie. we don't currently run it through that method - how about a follow-up to discuss all the ramifications of that?

Sure, I'm fine with that 👍

Opened #3020394: Investigate using Html::getUniqueId() on comment-ID identifiers

@lauriii in #49 has a points this might introduce invalid markup. Alternatively, we could use the name attribute instead of the id attribute for identical behavior but without the duplicate id attribute potential.

The name attribute is only allowed on form elements so that's not suitable for this usage.
We need these ids to be accurate because it's needed to build anchored links to the comments.
As the first call to Drupal\Component\Utility\Html::getUniqueId returns an unmodified id, it'd be fine to me to use it to prevent potential multiple comments outputs in the same page to break the ids uniqueness while preserving linkability.

As I understand it, the discussion in #55 and #56 would be better on #3020394: Investigate using Html::getUniqueId() on comment-ID identifiers, as the present patch does not worsen this problem and therefore will not address it.

Looks good! Moving the id to the parent element is a small risk because someone could have used it for targeting CSS or JavaScript but since it isn't generally good practice to use ids for doing that, I'm fine with making this change. Committed 69b99ec and pushed to 8.7.x. Thanks!