Clean up the CSS for Comment module

Tagging!

That was easy...

WHAT ABOUT RTL CSS FILES???

Do they need to be splitted or are they all considered to be theme.css.
I guess so...

Forgot patch

Doesn't the css get included from somewhere?

Probably you're right but how does it work if you have the base and theme css file. Do you need to include both of them?

For the patch in #3 I don't think we'd need both stylesheets. However, the .indented class seems to me like the type of style that provides basic functionality, and might belong in comment.base.css. Should indenting replies be considered standard behavior, or is that a theming choice?

If it's fine to keep them consolidated, here's a new patch with comment.info correctly pointing at the new stylesheet.

For your question about intend:

MODULE.theme.css
Generic design-related styles that could be used with Stark and
other themes would go in this file. It’s where all design assumptions
like backgrounds, borders, colors, fonts, <b>margins</b>, padding, etc, would
go.

I'm aware of the base/admin/theme stylesheets, which is why I'm discussing the purpose of .indented. It's not the property that should determine where each CSS rule lives, but its purpose.

So in this case, .indented provides margin styles that are likely to be used in all themes with consistency. Comments are indented so consistently that I'd consider it to be a functional component of comment.module, not a themable style. Does that make sense?

It does makes sense but you say:

.indented provides margin styles that are likely to be used in all themes with consistency.

If you need to put "likely" in your sentence to guarantee its correct it shouldn't be in a base.css file.
Only stuff that is always always always needed should be in base.css. But I agree its arguable, but we need to have this kind of discussion now before we commited this patch :)

For example, some themes will make the intendation 10pixels, others 20pixels.

I'm tempted to mark this rtbc.

Needs work after all, we need to rename the rtl files to match the new name

Renamed comment-rtl.css to comment.theme-rtl.css

Based on the aggregator approach, this should be a better patch :).

Maybe we should define a default colour for preview and unpublished. (like we have with the messages)
But that is up for discussion.

My patches are crap

Talked with jacine and sun, hopefully this is more what they are looking for.
I removed the margin because bartik and garland overwrite it :)

The patch in #15 is nearly identical to the one I posted in #12, minus the #comments margin.

I found an override in Garland but not in Bartik. For consistency I think #comments margin should stay.

Yes you're right, I found a #comments margin in my grep but that was because I'm screwing up these patches. So your patch in #12 is still good.

Srry for all the noise.

Looks good.

Hey guys, what do you think of dropping .comment-published and .comment-unpublished for just .published and .unpublished?

Since we don't have to support IE6, we are able to do .comment.published { ... } if needed. Node.css currently uses .node-unpublished (with the same exact color), so if we were to do the same thing for the node classes, we could just put .unpublished, and .published in system.theme.css.

Also, I'm not positive, but I think there is another "indented" class in core somewhere. If so, there may be justification to move that to system.theme.css, and we can get rid of this file.

Thoughts?

Tested the #comments margin again and it doesn't do *anything*. Tested in firefox, chrome and ie7.
indented is *only* used by comments but I moved everything to system.theme.css just to keep things moving.

the .preview class is kinda dangerous as garland uses it for something related to color module.

#21: 1216976-comment-proof-of-concept-21.patch queued for re-testing.

Aspilicious is correct. It seems that .indented is limited just to comment.module:

Feynman:d8h5 fkchris$ grep -rn '\.indented' *
modules/comment/comment-rtl.css:2:.indented {
modules/comment/comment.css:5:.indented {
themes/bartik/css/style-rtl.css:117:.comment .indented {
themes/bartik/css/style.css:735:.comment .indented {
themes/garland/style-rtl.css:230:.indented {
themes/garland/style.css:855:.indented {

And again, aspilicious is correct about #comments margin's effectiveness. Although it is used differently across themes, there is somewhat of a page flow issue causing the margin to be visually ineffective. Two screenshots of issue comments #12 and #15 respectively:

This is what I was thinking about:

/**
 * Returns HTML for an indentation div; used for drag and drop tables.
 *
 * @param $variables
 *   An associative array containing:
 *   - size: Optional. The number of indentations to create.
 */
function theme_indentation($variables) {
  $output = '';
  for ($n = 0; $n < $variables['size']; $n++) {
    $output .= '<div class="indentation">&nbsp;</div>';
  }
  return $output;
}

div.indentation {
  float: left; /* LTR */
  height: 1.7em;
  margin: -0.4em 0.2em -0.4em -0.4em; /* LTR */
  padding: 0.42em 0 0.42em 0.6em; /* LTR */
  width: 20px;
}

It's horrendous as hell, but indeed a separate issue. Just wanted to check if there was any way to reasonably combine both of them, but nevermind that.

As far as the color module and .preview goes, IMO color module should be changed to .color-preview or something. Color module classes (and any administrative classes) are way less important than common front-end facing classes, and leaving .comment-unpublished and .node-unpublished would be very IE6 of us, but I'll defer that decision to you guys.

I went to test this patch today, and it no longer applied. In the process of recreating it, I started having second thoughts about the changes I proposed. The inconsistency of class names it introduces was really bugging me (especially in the documentation of the template), so I ended up going back to a previous patch by @aspilicious. Sorry, I didn't mean to throw this off.

Anyway, I have rerolled a previous patch with one change. I renamed .indented to .comment-indented and updated the instances of it in Bartik and tests.

sub

I think the old class named better (.comment .indented)

.indented / .published ..etc (we able to style it globally when we do custom websites)

The problem with .indented is that it's not clear what it's for. I mean, we have to grep through the code to see where it's being used, which kinda sucks. It's very general, and inconsistent because everything else is name-spaced with .comment.

On the .published/unpublished/preview classes, I took the lack of response here to mean that you guys didn't support it, and like I said it looked odd in the template documentation (though this could be solved). If you do though, feel free to set this to needs work and re-roll the patch with those changes. Thanks! :D

How about .children ? Actually the comments are parent & children relatives, better than .comment-indented IMO. Therefore we could style all .children globally.

Assigning myself to work on this.

@jacine, it sounds like a really good idea to generalise the classes and move them to system.theme.css. The HTML/css used for the indentation is just plain awfull. Let's take this opportunity to replace it with someting that is semantically more correct!

Eagerly awaiting the new patch :)

Note: a patch was created in #1217012: Clean up the CSS for the Node module to add .published as a general class in system.theme.css. That updated could be used here to change .comment-unpublished to .unpublished.

Note: the indented massacre will be taken care of in another issue: #1316648: Threaded comments should be nested, so we should not worry about it here.

I also agree that it'd be a good idea to generalize .unpublished and .preview. I'm posting a patch to do that and remove the margin on #comments. I left in .indented for now since it looks like more discussion is going on in #1272870: No semantics for nested comments / bad for screen-readers about the best way to handle this.

The last patch seems to have gotten rid of the test.

Which test? I didn't include the test change from #26 because I left .indented as is.

Patch tested and the only thing that should be changed is replacing comment-unpublished and comment-preview in comment.tpl.php in Bartik.

For the rest: perfect patch!

Thanks! I completely missed Bartik's comment.tpl.php. Here's a patch with that fix.

Patch tested it sounds everything is fine

Patch tested and looks perfect to me now.

+++ b/modules/system/system.theme.cssundefined
@@ -38,6 +38,16 @@ tr.odd {
 /**
+ * Publishing options.

Can we call this "Publishing status?"

+++ b/modules/comment/comment.moduleundefined
@@ -2290,18 +2290,18 @@ function template_preprocess_comment(&$variables) {
+    // Published class is not needed. It is either 'preview' or 'unpublished'.

This is not your fault. I see it was wrong in the previous comment, but the period should be before the end quote.

+++ b/modules/comment/comment.tpl.phpundefined
@@ -31,11 +31,11 @@
- *   - comment-new: New comment since last the visit.
+ *   - comment-new: New comment since last visit.

This is actually called "killing kittens" cuz it's got nothing to do with the changes we're making in this patch. This should actually be done in the comment.tpl.php cleanup issue.

+++ b/modules/system/system.theme.cssundefined
@@ -38,6 +38,16 @@ tr.odd {
 /**
+ * Publishing options.

Can we call this "Publishing states?"

Other than that, this patch look good... Screenshots showing before and afters coming shortly.

And here are the screenshots... which show that everything looks identical.

Here's a patch with those updates. Sorry about the kitten killing. I'll pop my head into the comment.tpl.php issue.

+    // Published class is not needed. It is either 'preview' or 'unpublished'.

This is not your fault. I see it was wrong in the previous comment, but the period should be before the end quote.

Please ignore that remark for know; requires a larger discussion. The current comment is fine in terms of technical documentation. (The value is "unpublished", not "unpublished.")

Ok, here's a patch with the old text.

It looks like I was wrong.... and that it's done this way throughout core, which annoys me, but I assume it's a different convention for technical documentation. And that is a battle I am completely uninterested in fighting about. ;)

Sorry for making you do this patch again @DyanneNova. Thanks for re-rolling so quickly! This looks good to me. Will just wait for the bot to mark RTBC. :)

+++ b/modules/comment/comment.theme-rtl.css
@@ -0,0 +1,4 @@
+.indented {

+++ b/modules/comment/comment.theme.css
@@ -0,0 +1,3 @@
+.indented {

We're missing 1) a @file CSSDoc block header, and possibly 2) a regular CSSDoc block group header for the actual style here. system.theme.css and friends contain examples for these.

-1 days to next Drupal core point release.

@sun: i've only seen the @file and block group headers in the system module so far. Are they required for small css files too?

@jyve I don't think they're needed either, but @sun feels strongly about it, so yeah we're gonna have to add these to the other patches as well, and just discuss it in a separate issue later. :(

Here's a patch with the @file CSSDoc block header, and a block group header to explain .indented.

I don't mean to start a new discussion on the @file headers, but maybe we could follow a certain pattern for them? At firs I also copied the @file header from the system module, but then realized they use the wording 'common markup' since there is also a menu.css file for example.

I now came up with this for the other patches:

module.theme.css: Styling for the module module.
module.theme-rtl.css: RTL styling for the module module.
module.base.css: Generic base styles for module module.
module.base-rtl.css: Generic RTL base styles for module module.
module.admin.css: Admin styling for the module module.
module.admin-rtl.css: RTL admin styling for the module module.

Maybe we should open another discussion on this? Or do we want to focus on the cleanup first and not worry about the file headers too much?

@jyve That makes sense to me. I guess we should start a discussion on conventions somewhere.

I'm posting a patch that should actually pass testing this time.

+++ b/core/modules/comment/comment.theme-rtl.cssundefined
@@ -0,0 +1,13 @@
+ ¶

triling whitespace

+++ b/core/modules/comment/comment.theme.cssundefined
@@ -0,0 +1,12 @@
+ ¶

another one

29 days to next Drupal core point release.

Fixed the trailing whitespace. Sorry about that.

Patch reviewed ans looks perfect now.

+++ b/core/themes/bartik/templates/comment.tpl.phpundefined
@@ -31,9 +31,9 @@
- *   - comment-preview: When previewing a new or edited comment.
+ *   - preview: When previewing a new or edited comment.

or edited comment ?

19 days to next Drupal core point release.

@droplet: according to the comment in #42 we should not update these kind of things in this issue.

Anyone care to review the patch in #56? It would be nice to get this in together with the node patch in #1217012: Clean up the CSS for the Node module

+++ b/core/modules/comment/comment.moduleundefined
+++ b/core/modules/comment/comment.moduleundefined
@@ -2290,18 +2290,18 @@ function template_preprocess_comment(&$variables) {

@@ -2290,18 +2290,18 @@ function template_preprocess_comment(&$variables) {
+    $variables['status'] = ($comment->status == COMMENT_NOT_PUBLISHED) ? 'unpublished' : 'published';

+++ b/core/themes/bartik/css/style.cssundefined
@@ -737,11 +737,11 @@ ul.links {
+.comment.unpublished {

Node module does not have "published" status ? and comment module does.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2290,18 +2290,18 @@ function template_preprocess_comment(&$variables) {
   if ($comment->uid == 0) {
     $variables['classes_array'][] = 'comment-by-anonymous';
   }
   else {
-    // Published class is not needed. It is either 'comment-preview' or 'comment-unpublished'.
-    if ($variables['status'] != 'comment-published') {
+    // Published class is not needed. It is either 'preview' or 'unpublished'.
+    if ($variables['status'] != 'published') {
       $variables['classes_array'][] = $variables['status'];
     }

maybe a separated issue, why anonymous user do not have styles ?

15 days to next Drupal core point release.

@ jyve
I have looked at it and added a bunch of small thingies like article & footer tags
changed the $picture to$user_picture etc.
removed the ever annoing title class on the h2 :/

and killed the clearfixes surronding the comments - no need for them as the markup & css behaves.

@droplet
is spot on avout the missing publish state of a comment (which is only missing if its posted by anonymous)
so i have added that state to a comment made by anonymous users.

okay looks like theres also anothe issue about this comment thingie #1189816: Convert comment.tpl.php to HTML5 gather forces ?

HTML5 markup should absolutely NOT be happening here. Throwing markup changes just for the sake of is in these patches doesn't help. I'm tempted to postpone this issue until the other one is completed, TBH.

that would make sense - why arent they in one issue ?

@mortendk because one is about CSS and one is about converted comment.tpl.php to HTML5. Those are separate issues and this is how ALL of our issues are split. See the roadmap. I see no reason why this patch can't go on without touching the comment template, but since it keeps becoming a problem, I'll postpone this for now until #1189816: Convert comment.tpl.php to HTML5 is completed.

@Jacine, I think you forgot a 'not' in your comment in #65? Just to make sure we understood correctly :)

@jyve Whoops! Thanks for the save. I've edited the comment. :D

@mortendk: care to reroll your latest patch now that #1189816: Convert comment.tpl.php to HTML5 has been done?

In this reroll how much of the markup should we allow to patch to modify? None of it? I've read through this issue, but I think I might have to read through the linked issued to find out. I'll starting reading but if anyone figures this out before I do please let me know or better yet do the reroll.

I might be a bit naive with this patch, but here it goes:

This is the patch from #62, rerolled for HEAD, without the changes that would have effected *.tpl.php files.

Go testbot go

As per jacine's comment in irc: The only thing this patch needs is to use the generalized classes set in the node cleanpu patch : #1217012: Clean up the CSS for the Node module

@cosmicdreams, actually the patch you posted in #72 is missing a bunch of stuff.

Here's what needs to happen. #62 needs to be rerolled with the following changes:

+++ b/core/modules/comment/comment.tpl.phpundefined
@@ -57,21 +57,22 @@
+  <?php print $user_picture ?>

There should be a colon after $user_picture.

+++ b/core/modules/comment/comment.tpl.phpundefined
@@ -57,21 +57,22 @@
+    <strong class="new"><?php print $new ?></strong>

This should use the <mark> element, and it should also have a colon after the variable name.

 /**
+ * Publishing states.
+ */
+.unpublished {
+  background-color: #fff4f4;
+}
+.preview {
+  background-color: #ffffea;
+}
+

This part is being committed in the node patch #1217012-55: Clean up the CSS for the Node module, so there should be no changes to system.theme.css here.

Sorry, disregard my last comment. I need to be clearer about the changes and some of the markup stuff doesn't belong here. I will post an updated patch.

This should do it. Hopefully.

Note: I've removed the prefixes for the other classes that are added via preprocess as well so it's consistent.

Just a reminder... This code will be committed via the #1217012: Clean up the CSS for the Node module which is RTBC.

/**
 * Publishing states.
 */
.unpublished {
  background-color: #fff4f4;
}
.preview {
  background-color: #ffffea;
}

Thanks all!

Looks like we have a whole bunch of tests to update.

Whoops. Forgot about tests!

Here's an updated patch with the classes test fixes.

Well it has fewer fail. It seems to be failing on the first test for "new" comments: tests for the existence of the "new" class on new comments. Hm....

In this patch I:

• fixed a comment that still referred to comment-unpublished etc.
• fixed the logic involved with adding the 'new' class to the classes_array

+++ b/core/modules/comment/comment.moduleundefined
@@ -2150,30 +2150,29 @@ function template_preprocess_comment(&$variables) {
-  if ($variables['new']) {
-    $variables['classes_array'][] = 'comment-new';
+  if (!empty($comment->new)) {

What's wrong with the original logic? $variables['new'] is created a little further up in the function.

I saw that, and it looked good to me but the condition was a bit implicit. With the change I made I'm using the same logical condition that was used in the previous assignment of the $variables['new'].

Doesn't appear to matter much. Both implementations make the test fail. I'm not sure how to fix it.

We need:

1. Help figuring out how to make the patch in #83 pass tests.
2. The comment noted by @cosmicdreams in #87 that still refers to comment-unpublished needs to be changed to refer to just unpublished.

+++ b/core/modules/comment/comment.module
@@ -2150,30 +2150,29 @@ function template_preprocess_comment(&$variables) {
-    $variables['status'] = ($comment->status == COMMENT_NOT_PUBLISHED) ? 'comment-unpublished' : 'comment-published';
+    $variables['status'] = ($comment->status == COMMENT_NOT_PUBLISHED) ? 'unpublished' : 'published';
...
   if ($variables['status'] != 'comment-published') {

You are changing the value of $variables['status'] but you did not update the string comparison condition.

Ah very good, I do a more thorough job of finding these old references today.

This patch does the following:

• Starting with the patch in #83,
• Fixed any remaining references to "comment-published"
• Fixed remaining comments that referred to "comment-unpublished" and "comment-published"

#95 looks RTBC to me if bot comes back green. Let's assume it will. ;)

Still awaiting test results... Going to add this to the current sprint to track it.

haha, my local box Wins! I ran the test suite on my local box for comment and it finished already.

It failed. Same test fail with the new class. and another test failed with user picture. I'm not sure why these tests failed.

anybody know how to output a debug message within that test? I want to know more about what's going on in that test that fails.

After adding a few verbose messages into the test I was able to see that when the test does this:

$this->assertTrue(count($comments) == 1, 'new class found.');

The count($comments) actually equals 2 instead of 1.

I added this statement:

$this->verbose("test before comments new" . implode("|", $comments));

and here's what was returned:

test before comments new |new

so it seems like we have an array returned the first part of which is empty the second part has "new".

Do we need to correct the test or the code?

So, it's meeting the crux of that test: It has > 0 new classes when the comment is new and 0 new classes when the comment is not new. It's just not doing the xpath and count() right.

Ah I get it. The xpath is picking up both the class="new" and the "new" in the body of the span. Here's the markup that is produced in order to illustrate what I mean:

<span class="new">new</span>

Shouldn't the xpath be smarter than that? I mean, we are telling it to look only at class attributes. Is my assumption correct here?

No that's not it! Here's the full markup:

<div id="comments" class="comment-wrapper">
          <h2 class="title">Comments</h2>
      
  <a id="comment-1"></a>
<div class="comment by-node-author by-viewer clearfix">

  <div class="attribution">

    
    <div class="submitted">
      <p class="commenter-name">
        <a href="/user/1" title="View user profile." class="username">admin</a>      </p>
      <p class="comment-time">
        Tue, 03/06/2012 - 22:10      </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>

    
        <h3><a href="/comment/1#comment-1" class="permalink" rel="bookmark">Comment 1</a></h3>
    
    <div class="content">
      <div class="field field-name-comment-body field-type-text-long field-label-hidden"><div class="field-items"><div class="field-item even"><p>I am a comment</p>
</div></div></div>          </div> <!-- /.content -->

    <ul class="links inline"><li class="comment-delete odd first"><a href="/comment/1/delete">delete</a></li><li class="comment-edit even"><a href="/comment/1/edit">edit</a></li><li class="comment-reply odd last"><a href="/comment/reply/1/1">reply</a></li></ul>  </div> <!-- /.comment-text -->
</div>
<a id="new"></a> 
<a id="comment-2"></a>
<div class="comment new by-node-author by-viewer clearfix">  

  <div class="attribution">

    
    <div class="submitted">
      <p class="commenter-name">
        <a href="/user/1" title="View user profile." class="username">admin</a>      </p>
      <p class="comment-time">
        Tue, 03/06/2012 - 22:29      </p>
      <p class="comment-permalink">
        <a href="/comment/2#comment-2" class="permalink" rel="bookmark">Permalink</a>      </p>
    </div>
  </div>

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

          <span class="new">new</span>  
    
        <h3><a href="/comment/2#comment-2" class="permalink" rel="bookmark">Comment 2</a></h3>
    
    <div class="content">
      <div class="field field-name-comment-body field-type-text-long field-label-hidden"><div class="field-items"><div class="field-item even"><p>Second comment</p>
</div></div></div>          </div> <!-- /.content -->

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

      <h2 class="title comment-form">Add new comment</h2>
    <form class="comment-node-page-form comment-form" action="/comment/reply/1" method="post" id="comment-form" accept-charset="UTF-8"><div><div id="edit-author--2" class="form-item form-type-item">
  <label for="edit-author--2">Your name </label>
 <a href="/user/1" title="View user profile." class="username">admin</a>
</div>
<div class="form-item form-type-textfield form-item-subject">
  <label for="edit-subject">Subject </label>
 <input type="text" id="edit-subject" name="subject" value="" size="60" maxlength="64" class="form-text">
</div>
<div class="field-type-text-long field-name-comment-body field-widget-text-textarea form-wrapper" id="edit-comment-body"><div id="comment-body-add-more-wrapper"><div class="text-format-wrapper"><div class="form-item form-type-textarea form-item-comment-body-und-0-value">
  <label for="edit-comment-body-und-0-value">Comment <abbr class="form-required" title="This field is required.">*</abbr></label>
 <div class="form-textarea-wrapper"><textarea class="text-full form-textarea required resize-vertical" id="edit-comment-body-und-0-value" name="comment_body[und][0][value]" rows="5" cols="60"></textarea></div>
</div>
<fieldset class="filter-wrapper form-wrapper" id="edit-comment-body-und-0-format"><div class="fieldset-wrapper"><div class="filter-help form-wrapper" id="edit-comment-body-und-0-format-help"><p><a href="/filter/tips" target="_blank">More information about text formats</a></p></div><div class="filter-guidelines form-wrapper filter-guidelines-processed" id="edit-comment-body-und-0-format-guidelines"><div class="filter-guidelines-item filter-guidelines-plain_text"><h3 style="display: none; ">Plain text</h3><ul class="tips"><li>No HTML tags allowed.</li><li>Web page addresses and e-mail addresses turn into links automatically.</li><li>Lines and paragraphs break automatically.</li></ul></div></div></div></fieldset>
</div>
</div></div><input type="hidden" name="form_build_id" value="form-nsEY0NhuproMU0a0skeZP9wwalI-_9xO9rXMSc71TjI">
<input type="hidden" name="form_token" value="3MZbF_HApXJqZmb0d3aOJ79mfVDrxYVK_ZWxxaIZiZE">
<input type="hidden" name="form_id" value="comment_node_page_form">
<div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Save" class="form-submit"><input type="submit" id="edit-preview" name="op" value="Preview" class="form-submit"></div></div></form>  </div>

That begs the following questions:

• Can we get rid of the <a id="new"></a> because it doesn't seem to do anything?
• Do we need to change the name of the new class for either the div or the span? or...
• Do we need to modify the test so that it can be happy with both the div and span having the "new" class?

That begs the following questions:
Can we get rid of the because it doesn't seem to do anything?
Do we need to change the name of the new class for either the div or the span? or...
Do we need to modify the test so that it can be happy with both the div and span having the "new" class?

I don't understand the first question, but why would we change any of the markup here? The test is the problem. We need help figuring that out. That's all we need.

I wish I could just do this myself, but I already tried, and don't know what is wrong with it. I could have sworn I tagged this asking for PHP help before, but the tag isn't there. Trying to add it again.

This patch fixes the errors by modifying the patch (seemed the most sensible of the choices)

Can we get rid of the because it doesn't seem to do anything?

This allows you to link to the latest comment, so no, we cannot remove it.

woops. didn't mean to step on your tagging

This patch fixes the errors by modifying the patch (seemed the most sensible of the choices)

This doesn't tell me anything about what you did from one patch to the next. Please be specific about what changes you are making when you change the approach in a patch, otherwise it's really, really hard to review. Thanks :)

Looks like this will fix all but one test. Which is the user-picture test that #95 failed on. That one seemed to be filtering by xpath on @class="comment-preview". That looks like it needs to be modified to filter on @class="preview" instead. Testing this on my local test suite...

yep that did it.

new patch pending.

This patch fixes the 19 failed tests from #95.

wow, did it again, adding your tag back

So here's a run down on what I did in order to get the tests to pass:

1. Fix for the "new" class tests

   from line 565 to 579

   // Verify the new class.
         if ($case['comment_status'] == COMMENT_PUBLISHED || $case['user'] == 'admin') {
           $comments = $this->xpath('//*[contains(@class, "new")]');
           if ($case['user'] != 'anonymous') {
             $this->assertTrue(count($comments) == 2, 'new class found.');
   
             // Request the node again. The new class should disappear.
             $this->drupalGet('node/' . $node->nid);
             $comments = $this->xpath('//*[contains(@class, "new")]');
             $this->assertFalse(count($comments), 'new class not found.');
           }
           else {
             $this->assertFalse(count($comments), 'new class not found.');
           }
         }
   

   I changed the assertTrue from $count(comments) == 1 to $count(comments) == 2

2. Fix for the user-picture test

   on line 1007 - 1008

   // Check that the user picture is displayed.
   $this->assertFieldByXPath("//div[contains(@class, 'preview')]//div[contains(@class, 'user-picture')]//img", NULL, 'User picture displayed.');
   

   I changed the class from "comment-preview" to preview

+++ b/core/modules/comment/comment.test
@@ -525,56 +525,56 @@ class CommentInterfaceTest extends CommentHelperCase {
+      // Verify the new class.
       if ($case['comment_status'] == COMMENT_PUBLISHED || $case['user'] == 'admin') {
-        $comments = $this->xpath('//*[contains(@class, "comment-new")]');
+        $comments = $this->xpath('//*[contains(@class, "new")]');
         if ($case['user'] != 'anonymous') {
-          $this->assertTrue(count($comments) == 1, 'comment-new class found.');
+          $this->assertTrue(count($comments) == 2, 'new class found.');

In general:

If you did not change functional business logic in Drupal, and you also did not change the fundamental logic and workflow of a test case/method, then a change to a count() assertion like this can inherently only be not correct.

We haven't changed how Comment module renders comments, or how many of them, or what comments an authenticated user is able to see. We also didn't massively change the test code. In turn, logically, there cannot suddenly be two (new) comments on the page where only one has been before.

It rather looks like the XPath of //*[contains(@class, "new")] has a false-positive match; i.e., there's another, totally irrelevant DOM element on the page that happens to have the "new" class. To debug this, I'd suggest to run the test locally with verbose output, go to the dumped verbose output page that fails, open your browser DOM inspector of choice, and execute jQuery('.new'); there (the equivalent of the XPath expression). (Some browsers also support document.evaluate() to select elements by XPath, but it's not exactly trivial to use.)

Now that I see you already dumped the verbose markup into #105, but didn't state what the actual false-positive match is, let me clarify that for everyone:

<a id="new"></a>
<a id="comment-2"></a>
<div class="comment new by-node-author by-viewer clearfix"> 
  <div class="attribution">...</div>
  <div class="comment-text">

    <span class="new">new</span> 

    <h3><a href="/comment/2#comment-2" class="permalink" rel="bookmark">Comment 2</a></h3>
    <div class="content">

It's not the anchor with ID #new. The wrapping DIV container has a .new class, and there's also a SPAN.new element within that container.

The former is what we've changed here. The latter (SPAN) already existed, and is used to actually output the "new" string/marker for users.

A possible solution to this is to change all the XPaths in the test to

//*[contains(@class, "comment") and contains(@class, :class)]

(Note: The test intentionally uses the * tag/element selector, because Comment module's comment.tpl.php uses ARTICLE already, whereas Bartik's uses DIV. The test shouldn't have to care for the actual HTML element being used, and we want to switch this test to no longer depend on Bartik at some point.)

THANK YOU @sun! ;) I will try this later.

Cool, when I try to use the new syntax for doing the xpath I get a bunch of "invalid syntax" warnings that seem to break the test.

I'll wait for jacine to do this the right way to see what I'm doing wrong.

The fact that the expression in #115 didn't work led me to scour the internet for reasons why. I eventually came across this article: http://stackoverflow.com/questions/1064968/how-to-use-xpath-contains-here

In in the solution and it's comments illustrate that in order to combine the two "contains" tests we need this kind of syntax:

//*[contains(@class, "comment") and *[contains(@class, :class)]]

I thought it was worth trying so I'm running the test suite locally for this singular test. If it works I'll change the others to use this new expression.

OK, I think I was wrong about the proper syntax to use and I ended up using sun's version instead of mine. After re-trying that syntax it seems to have most success. I'm testing an isolated solution in my local test suite. Since that takes about 16 minutes to run I had plenty of time to whip this patch up.

This patch:

• uses sun's xpath query
• spreads the change in the xpath query to all of this test's siblings

local run of this test came back green. So I'll set this to needs review so the rest of the tests (on the siblings) can be tested.

Tested this patch, and looks perfect.

The new classes get applied correctly in all the different scenario's.

Great job!

Excellent. Thanks!

tagging for jhodgdon

There are a couple of small documentation issues in this patch, in comment.tpl.php (both the comment module one and the Bartik one):
a)

+ *   unpublished, published or preview.

Drupal docs standards use , before or in lists like this.

b)

 *   - comment: The current template type, i.e., "theming hook".

This wasn't part of the patch, but it should not be "i.e." here. It should read:

 *   - comment: The current template type; e.g, 'theming hook'.

c)

+ *   - new: New comment since last the visit.

since last the -> since the last

@jhodgdon, I don't understand you're part a) statement, am I missing a comma?

I'm going to guess that all part a) needs is another comma.

Here's a patch to fix it.

Found a typo in comment.tpl.php (the one in core/modules/comment) where I had ";ast" instead of "last"
Fixed with this patch:

Sorry, #124 (a) meant that:

+ *   unpublished, published or preview.

needs to be
unpublished, published, or preview.
Yes, just needs a comma. Still does. :)

And from #124 (b), there needs to be a semicolon before "e.g.". That is still not right.

@jhodgdon: Are you talking about the patch in #128? I'm not seeing any of the omissions you call out in #129 in the patch from #128. Can you please verify?

@cosmisdreams The documentation errors needed to be fixed in both comment and Bartick templates.

thanks @dcmouyard, I see what I missed now.

Committed to 8.x. Thanks.

Add clean up docs.