Comment rendering broken: comment body rendered outside of the bubble

I can confirm the behavior.

What's even more weird, and I think even more critical than that rendering imo, I can't even submit a comment in when the comment box has an editor ..
Getting following errors in console too:

An invalid form control with name='comment_body[und][0][value]' is not focusable.

Jess says this might be easy to fix. :D Tagging novice.

Also, this probably doesn't need to be *critical*. It's very dumb, but we could certainly ship D8 with dumb styling. Reducing to major.

Just looking at the markup not sure that this is a novice issue. In D8 the <header> markup contains the attribution. I might be wrong (css and markup is not my area of expertise) but I think that in order to make this look like d7 options are:

• some seriously funky css

OR

• we are going to have to move the attribution outside of the header and wrapper the header, content and footer in a wrapper.

OR

• come up with a new design

D8 comment markup

<article class="comment by-anonymous clearfix" about="/comment/1#comment-1" typeof="sioc:Post sioct:Comment" role="article">

  <header class="comment-header">

    <div class="attribution">
      <article class="profile">
  </article>

      <div class="submitted">
        <p class="commenter-name">
          <span rel="sioc:has_creator"><span class="username" lang="" typeof="sioc:UserAccount" property="foaf:name" datatype="">jibran (not verified)</span></span>        </p>
        <p class="comment-time">
          <span property="dc:date dc:created" content="2013-04-02T07:16:52+01:00" datatype="xsd:dateTime">02 April 2013</span>        </p>
        <p class="comment-permalink">
          <a href="/comment/1#comment-1" class="permalink" rel="bookmark">Permalink</a>        </p>
              </div>
    </div> <!-- /.attribution -->

    <div class="comment-text">
      <div class="comment-arrow"></div>

      
            <h3 class="" property="dc:title" datatype=""><a href="/comment/1#comment-1" class="permalink" rel="bookmark">Congrats</a></h3>
          </div> <!-- /.comment-text -->

  </header> <!-- /.comment-header -->

  <div class="content">
    <span rel="sioc:reply_of" resource="/node/8" class="rdf-meta"></span><div class="field field-name-comment-body field-type-text-long field-label-hidden" data-edit-id="comment/1/comment_body/und/full"><div class="field-items"><div class="field-item even" property="content:encoded"><p>Congrats</p>
</div></div></div>  </div> <!-- /.content -->

  <footer class="comment-footer">
    
    <nav>
      <ul class="links inline"><li class="comment-reply odd first last"><a href="/comment/reply/7/1">reply</a></li></ul>    </nav>
  </footer> <!-- /.comment-footer -->

</article>

What this comment looks like...

D7 comment markup

<div class="comment comment-new comment-by-node-author comment-by-viewer clearfix" about="/comment/1#comment-1" typeof="sioc:Post sioct:Comment">

  <div class="attribution">

    
    <div class="submitted">
      <p class="commenter-name">
        <span rel="sioc:has_creator"><a href="/user/1" title="View user profile." class="username" xml:lang="" about="/user/1" typeof="sioc:UserAccount" property="foaf:name">alex</a></span>      </p>
      <p class="comment-time">
        <span property="dc:date dc:created" content="2013-04-12T19:22:18+00:00" datatype="xsd:dateTime">Fri, 04/12/2013 - 19:22</span>      </p>
      <p class="comment-permalink">
        <a href="/comment/1#comment-1" class="permalink" rel="bookmark">Permalink</a>      </p>
    </div>
  </div>

  <div class="comment-text">
    <div class="comment-arrow"></div>

          <span class="new">new</span>
    
        <h3 property="dc:title" datatype=""><a href="/comment/1#comment-1" class="permalink" rel="bookmark">tertr</a></h3>
    
    <div class="content">
      <span rel="sioc:reply_of" resource="/node/2" class="rdf-meta"></span><div class="field field-name-comment-body field-type-text-long field-label-hidden"><div class="field-items"><div class="field-item even" property="content:encoded"><p>werwer</p>
</div></div></div>          </div> <!-- /.content -->

    <ul class="links inline"><li class="comment-delete first"><a href="/comment/1/delete">delete</a></li>
<li class="comment-edit"><a href="/comment/1/edit">edit</a></li>
<li class="comment-reply last"><a href="/comment/reply/2/1">reply</a></li>
</ul>  </div> <!-- /.comment-text -->
</div>

What this comment looks like...

This issue is blocking Twig conversion. Does that make it critical?
See #1898054: comment.module - Convert PHPTemplate templates to Twig

whoopsie, didn't mean to remove the tag.

I've attached a path that fixes the problem.

I felt it was semanticly wrong to have the header element there at all so i removed it, placed the comment body after the title and added a small css change.

Now looks as it did in 7.

Adam

There must be some other problem with the test that failed, all the patch does is re-arrange some markup. There's absolutley no change to logic/php/database and the tests that failed were:

Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest845479semaphore' doesn't exist:

This is a lot deeper than a simple change of markup and css.

#7: core-bartek_comment_template-1965650-0.patch queued for re-testing.

After applying the patch core-bartek_comment_template-1965650-0.patch by rabbit_media in #7 to a Drupal 8 fresh install, I confirm the comment subject and body are within the bubble in Bartik. See screenshots for the markup produced.

As this was introduced in #1179764: Convert Bartik to HTML5 I'd like JohnAlbin to +1 this before committing. Completely changing the semantic markup to fix a visual defect needs oversight...

Postponing this on #1938840: bartik.theme - Convert PHPTemplate templates to Twig.

We can fix this bug once twig is in!

Twig is in!

Adapted patch from #7 for twig

Looking at another issue I ran into this, tried to apply the patch, and it no longer applies as of Today. I re-rolled patch.

I also noticed that the footer links are outside of the bubble in the previous patch, but in Drupal 7 the links are inside the bubble. I have fixed this regression as well in the patch.

#17: drupal-bartik-comment-outside-bubble-1965650-17.patch queued for re-testing.

I tested this. Without the patch I see the bug as described and the latest patch fixes things. I can't review the code quality but seems to me this is rtbc.

+++ b/core/themes/bartik/templates/comment.html.twigundefined
@@ -65,65 +65,61 @@
-  <header class="comment-header">
+  <div class="attribution">
+    {{ user_picture }}

Hmmm are we sure that we want to remove the html5 header tag here?

We still input from someone on the new markup as per #13

No we definitely don't want to remove <header> - it would degrade our semantical markup, see HTML5 specs.
Actually we should use more, not less HTML5 semantics, such as <time>, <link> and <address> elements, here, but this doesn't have to be in this patch.

Also, not critical per #3 and in spite of #5.

[edit:] escaped markup

Here's a new patch that fixes the <header> rather than removing it.

Screenshot after patch:

Implemented <address> and <time> instead of divs.
Also removed classes that are unnecessary now that we can address .comment time, .comment address, .comment header and .comment footer.

Screenshot:

Didn't manage it to avoid a tiny little difference that I couldn't track down. Needs some work, possibly in a followup issue.
Sending it to the testbot, but primarily #23 remains to be reviewed.

Is it a conscious decision to move the comment moderation links outside of the bubble?

I don't see that documented anywhere in the issue other than my comment about it being a regression. I searched for other issues, and I could not find any information regarding this change. I'm not against it, but I would like to confirm that is the intent.

Not supposed to happen. This was fixed with the patch in #17, apparently that got lost again.

Thank you, yoroy. I updated the issue summary to help clarify things.

I actually looked that up a bit ago. There is no format for address. It must only be the relevant contact information for the parent article or body element. The only question would be if a user link is a relevant contact link. I think it is.

<article>
<address>
I can do anything in here as long as it has relevant contact information for the article's contact. It can't be an arbitrary contact info. that doesn't relate to the article element (i.e. random postal addresses, e-mail addresses, user links, etc...).
</address>
</article>

<footer>
<address>I can do anything in here as long as it has contact information for the body.</address>
</footer>
</body>

I am a bit more concerned about the time element, which does have a specific syntax and a more confusing specification. If the datetime attribute is specified, then that needs to have the specific syntax and the content of the time element is the human-readable meaning for that date/time.

I think this is what the specification is trying to say... But it's confusing.

<!-- the time is set to the specific syntax in {{ created }} but created could not match the syntax in the spec? -->
<time>{{ created }}</time>

<!-- created means the specific syntax "11-21-2013" -->
<time datetime="11-21-2013">{{ created }}</time>

Yes, you're both right. The whole footer belongs into the box.
Here's another patch. Output looks really good now.

Screenshot:

Two things have changed against D7:

• "Permalink" isn't lowercased anymore (#1913958: Bartik theme: Don't change capitalization of translatable strings with CSS)
• The text content has become a bit larger. However this is also the case without the patch so should be on purpose or unrelated (either way it's better than in D7) Can only be a browser issue or something. Opened in another tab, now it looks exactly the same as in D7.

Re #29:
While I, IMHO properly, introduced the <address> and <time> elements, I omitted the datetime attribute for now. Can be a followup or rolled in. Didn't want to hold up the bug fix longer for that.

Tags seem to have become a tiny little bit larger. This might also affect other fields.
I wasn't able to track it down though because the inspector on Chromium gave me exactly the same Computed Style with exactly the same CSS rules matching. Might try it in Firefox later.

D7:

D8 with patch #30:

Don't worry too much about font size of the tags. See #1218640: Bartik displays all taxonomy term fields in a smaller font than other fields for that.

#30: drupal-bartik-comment-outside-bubble-1965650-30.patch queued for re-testing.

After applying the patch drupal-bartik-comment-outside-bubble-1965650-30.patch by Pancho in #30 to a Drupal 8 fresh install 8/9 , I confirm the comment subject and body are within the bubble in Bartik and the markup contains the html5 elements. See screenshots for the markup produced. Hope someone can look at this and move it forward, thanks!

After applying the patch drupal-bartik-comment-outside-bubble-1965650-30.patch by Pancho in #30 to a Drupal 8 fresh install 8/9 , I confirm the comment subject and body are within the bubble in Bartik and the markup contains the html5 elements. See screenshots for the markup produced. Hope someone can look at this and move it forward, thanks!

While I am excited beyond belief to see this issue RTBC (yayyyy!!) the changes here look like they're expanding way beyond the scope of fixing just this one bug. :\ Adding elements like <time> and <footer> and whatnot seem like a good idea, but this is taking Bartik's comment.html.twig file quite far away from Comment module's comment.html.twig file. If some of these are good ideas for better semantic markup in all themes (which it seems like they are), then they should be proposed instead to the "upstream" template and then updated in Bartik only after a patch to change Comment module's markup is approved (or actually, as a part of that patch most likely).

Could we please slim this patch down to only the changes required to fix this one bug?

This patch is a re-roll of #23 with the fix from #30 for the node links without any of the extra element changes in #24.

I have added a combined diff for follow-up issues. Note that bartik's comment.html.twig doesn't even have the HTML 5 mark element that core comment.html.twig has now.

Added issue summary.

Updated issue summary based on RTBC -> needs work comments

If someone can give the patch in #37 a review comparing #30 just to make sure I didn't forget or regress anything, then I think this can be set back to RTBC.

After applying the patch drupal-bartik-comment-outside-bubble-1965650-37.patch by mradcliffe in #38 to a Drupal 8 fresh install 8/12 , I confirm the comment subject and body are within the bubble in Bartik and the markup removes the html5 elements address and time, using p and div instead. See screenshots for the markup produced.

This patch fully complies with coding standards, doesn't need tests, and actually solves the problem of the comment printing outside the bubble, without introducing any HTML5 elements unrelated to this particular issue.

I added a space after display and rerolled the patch from 37. Attached interdiff with patch from 37.

+++ b/core/themes/bartik/css/style.css
@@ -859,6 +859,10 @@ ul.links {
+.comment-footer{
+  display: table-row;
+}

And add a space before {?

Edit: referencing https://drupal.org/node/1887862.

Added a space before the bracket.

Yay, back to RTBC.

Great work!!

Committed and pushed to 8.x. Thanks!

SO glad to have this one fixed. :)

That would be #2044863: EntityFromController doesn't assign weights for extra fields that are not yet saved in a EntityFormDisplay. Beware, the title is misleading because the fix will probably be something else entirely :/