"Unpublished" style of rendered entities is not accessible (and looks bad)

Please do :)

bot?

I agree that 'pink' doesn't communicate 'unpublished'.

It works even with one-line comments: http://skitch.com/yoroy/dxrb2/test-d7

#3: comments-unpublished.patch queued for re-testing.

I'm against this, unless the code is moved to theme(s) or a less intrusive solution can be agreed upon. Here's why:

• It's too design-specific for core
• It makes the cardinal sin of defining a font-family and colors, among other things in a core module's CSS file
• It will fail hard on any background other than white
• As pictured in #6, it will also fail when dealing with threaded comments and two filled sidebars
• It's also likely to fail with other translations of "unpublished" if the word ends up being much longer

This will result in unnecessary overrides for template files and CSS for many themes. We have a .comment-unpublished class, and we have easily editable template files, so why can't we just let themes decide how they want to style this?

If we must do something here, is there anything less intrusive we can do to communicate this status clearly?

I also do not see this #394736: Unpublished node's pink background unclear as having been committed.

Actually, it fails out of the box with 2 sidebars.

The textual message is good because it solves a nasty accessibility issue that there is no non-visual indicator of unpublished content - however themes should set the design for this, not a core module.

tagging

@Jacine - Thanks!
#8-2 with the fonts, the blocks module defines a font-family too modules/block/block.css: font-family: "Lucida Grande", Verdana, sans-serif;
#8-3 I think it would work fine with a black background. It certainly would be more visible than the existing pink background for anything other than the same gray used with the text.. For most backgrounds it would be much more visible than what is there now.
#8-5 I wonder if the font size could be smaller for the comments too. We should have translations for unpublished in many different languages. I wonder if there is a way to simply compile all of the known translations for string length.
#9 this would also be a problem with #394736: Unpublished node's pink background unclear then I assume.

This text is key for visually impaired people to be able to understand the difference between published & unpublished comments:

<div class="unpublished"><?php print t('Unpublished'); ?></div>

How do we get that in while encouraging themes to decide how best to style this?

Most of the controversy is around the css, so just focusing on the CSS, what if we nix the font & reduce the size of the text to 4em's:

Index: modules/comment/comment.css
===================================================================
RCS file: /cvs/drupal/drupal/modules/comment/comment.css,v
retrieving revision 1.5
diff -u -r1.5 comment.css
--- modules/comment/comment.css 31 Dec 2008 12:02:21 -0000      1.5
+++ modules/comment/comment.css 1 Aug 2010 23:37:24 -0000
@@ -6,8 +6,19 @@
 .indented {
   margin-left: 25px; /* LTR */
 }
-.comment-unpublished {
-  background-color: #fff4f4;
+.comment-unpublished div.unpublished {
+  height: 0;
+  overflow: visible;
+  color: #d8d8d8;
+  font-size: 4em;
+  line-height: 1;
+  font-weight: bold;
+  text-transform: uppercase;
+  text-align: center;
+}
+.preview .comment div.unpublished {
+  display: none;
 }
 .preview .comment {
   background-color: #ffffea;

Better title.

What we will do for D7 is the following:

+++ modules/comment/comment.tpl.php	1 Aug 2010 23:37:24 -0000
@@ -76,6 +76,10 @@
+    <?php if ($status == 'comment-unpublished'): ?>
+      <div class="unpublished"><?php print t('Unpublished'); ?></div>
+    <?php endif; ?>

1) We only add that textual representation of the unpublished status, and only that, and with the usual .element-invisible class applied to it.

2) We move that output into a proper theme function, and we make it generic. I.e., to make this crystal clear: theme_unpublished_marker() [or similar] produces always the same output, regardless of whether for nodes or comments or whatnot, and always has the .element-invisible class (set by the theme function), and optionally an additional, generic "unpublished" class. The template only contains a template variable. The themed output is prepared and generated as usual in the template preprocessor, depending on the respective entity's status.

3) Zero, absolutely none CSS styles go into D7 core for this.

Lastly, since the old issue title somehow suggested that there was or is some existing stuff for nodes already, and if there actually is something, then we are reverting that here to make this consistent.

Proper category and component.

1) Why hide this, seems this information is useful to everyone not just screen reader users. For screen readers it needs context, i.e. a heading, dumping the word "Unpublished" in a node or comment may not be useful if its not contextualized.

2) Agree, but, can we get that into D7 - we need to fix but can we do it properly or will we need a hacky solution for D7 (strait in the tpl's)

3) Agree - zero CSS in modue css for this.

Looks like there was no commit for the node template stuff so I marked #394736 as a duplicate in favor of cleaning it all up in one issue.

For screen readers it needs context, i.e. a heading, dumping the word "Unpublished" in a node or comment may not be useful if its not contextualized.

Am I missing something? I thought the whole purpose of this issue was to fix accessibility of unpublished entities (comments), which are currently just colorized...?

The list was meant additive, i.e., we do all of (1) to (3), but we don't change the existing visual behavior/look. We merely and solely fix the accessibility bug.

By introducing a theme function and standardizing its usage in templates, themes are free to override that and do whatever they want, but D7 core will not. D7 core will just fix the accessibility bug.

The original issue was just that pink was not a good indicator of status so the proposal was to add text as well, the problem with the node and comment patches are the styles & for screen readers the "message" would have no context - the screen reader user could learn that hearing the word "unpublished" when viewing node and comments means something important. Its better if we hide a heading above it just for screen reader users.

So the issue as I see it pans out to several things:

1. Color alone is not sufficient indicator of status, and a WCAG violation (for sighted users).
2. A simple message with no context is difficult/problematic for screen reader users.

Hrm. WCAG violation for sighted users... If you'd ask me (but I'm not familiar with all of this), then I'd say a visual improvement for sighted users can happily wait for D8. The only definite bug I see is that non-sighted users have no effin' clue why a unpublished post still appears, because they don't see that background color at all. So, if it was me, I'd limit this bugfix for D7 to the hard accessibility bug for non-sighted users, especially as there is no valid and working solution for a visible "unpublished" text yet, so I'd leave that visible part to contrib for now (which can be easily done, given the theme function we're about to implement here).

However, that's just me, and so we need to ask Dries/webchick for their perspective. Until they follow-up, we can and should already work on the first part.

Being a screen-reader user I know if a node or comment is unpublished (unapproved) because I have an option to approve / publish. Would anyone without the ability to publish / approve see an unpublished / unapproved entity?

As for colour, as long as it is perceivable, it is the same WTF for everyone about what the colour means, and again, can they not see the ability to pub / approve?

#8-2 with the fonts, the blocks module defines a font-family too modules/block/block.css: font-family: "Lucida Grande", Verdana, sans-serif;

This is wrong. It should never happen. It's unfortunate and disappointing that these styles keep making it into core CSS files. block.css is an atrocity, but unfortunately it's D8 material: #865084: Clean up block module css styles.

#8-3 I think it would work fine with a black background. It certainly would be more visible than the existing pink background for anything other than the same gray used with the text.. For most backgrounds it would be much more visible than what is there now.

#8-5 I wonder if the font size could be smaller for the comments too. We should have translations for unpublished in many different languages. I wonder if there is a way to simply compile all of the known translations for string length.

I don't want to see any background color, color, or font-size changes. Every single piece of code that messes with CSS in this way, ends up as another inconsistency and annoyance for a theme developer writing a custom theme. It's up to each theme to decide how this should fit into their design.

I'm not just objecting to the CSS though. I don't like how this is placed in the template file. I find it over designed for core, intrusive and IMO it's misplaced as it really only makes sense for this specific of design. If this text is properly placed, there will be no need to provide CSS for it. I'd much prefer a variable in the templates containing something like: <span class="status">This [entity] is [status].</span> or something, right underneath the title. I also agree that it should be generated by a theme function as opposed to spreading similar code across multiple template files, as there are more than a few places where this makes sense.

• This node is unpublished.
• This comment is unpublished.
• This comment is marked as spam.
• This user is blocked, etc...

#9 this would also be a problem with #394736: Unpublished node's pink background unclear then I assume.

Yes.

Bottom line is that it's not just an WCAG issue. I think it's clearly D8 material, but if for whatever reason an exception is made, it should be done right.

sub

#3: comments-unpublished.patch queued for re-testing.

Looks like the best we can do here is the suggestion from @sun in #13 for D7.

With that we can add the css in a theme or the accessibility helper module easily enough.

Can someone roll up a patch so we can try to get this into core?

No CSS, just the invisible text

also, this post has been favoured from #394736: Unpublished node's pink background unclear which is now marked as a duplicate.

All the text from from http://drupal.org/files/issues/node-unpublished-394736.patch will need to be brought over and marked with element-invisible's class.

Not hard, but...

Ok, this improves things for screen readers at the very least. Hopefully this can get into core & address unpublished nodes & comments.

I don't think there are any i18n considerations as the terms are very generic and must be used elsewhere.

Still waiting on an answer to #19. The best method for solving this issue would seem to require us to:

1. Assess where we are.
2. Decide where we want to be.
3. Determine the best path between 1 and 2.

:)

Its very hard to assess where we are Everett, your reports while interesting do not reflect a wider community. You are making an inference that a node or comment has a particular status, but its not stated, I think that is quite brittle, to rely of the appearance and detection of a link - in your own words you demand additional cognitive cycles - you make me think about this, rather than it simply being stated as such - this node is unpublished.

Was there some earth shattering reason why we can't have <?php print $unpublished; ?>, & push the logic back to template_preprocess_node etc, its not an API change, technically its an addition. Our templates are already clogged to the brim with confusing crap, really rather not have to do that in tpl.

@Jeff

I am really not being argumentative here. I am attempting to gather information, about something that I am admittedly ignorant about, so that I can participate in a discussion about the best solution to the problem.

@Everett - I Angie's point with the cat is better, but I've tried to lay out as much detail as I can here:
#928776-20: Unpublished comments have a styling issue

Please either apply the patch or check out my sandbox. Sorry you need to request another account.

@Jeff - <?php print $unpublished; ?> sounds good to me. Do you have time to roll up another patch. If not I can try to review it tomorrow.

After getting some more information about this problem let me summarize my thoughts.

Problem

Entities do not explicitly display their status textually. Nodes only implicitly display their status textually if the node is in Edit mode.

Severity

Not critical since this existed in D6, but certainly important as this would improve usability and accessibility.

Solution

I like @Sun's suggestion in #13, with the exception of thinking that the textual representation should be visible to all by default, I am however flexible on this as long as it is easy for a themer to override the visibility of the text. Sun's approach is in accordance with good clean markup and easy to understand template files.

Challenges

#13 would mean that an API change needs to occur (perhaps this is only an API addition, I am not familiar enough to know for sure). Would #13 be able to go into D7, or would it have to wait for D8? Can we get Dries / Webchick's input on this possible solution without a patch to review, or would we need to roll a patch for #13 to get a review?

I would like to see a solution to this problem in D7, but if there is no clean solution that we can get into D7 I would be content with waiting to fix this problem once, and properly, in D8.

Thank you Everett. I'm happy to see an accurate assessment of the problem and acknowledgement of feedback that @sun and I gave in previous comments. As I mentioned, I'm on board with the solution as described in #13. I think the text should be visible though, or we are not solving anything here, other than making the text available to screen reader users. The reason this issue was opened in the first place is that the pink background is not a sufficient indication for an unpublished status and I think we all agree on that.

The patch in #25 does not take into account that this should be in a theme function, nor is it even consistent across the template files. Just moving the "crap" to preprocess functions it not acceptable either. Littering in preprocess functions is almost as bad files. This sort of thing is the definition of a use case for a theme function, so anything else is just a hack.

As Jeff points out in #27 it should just be printed as <?php print $unpublished; ?> in templates. No logic. This variable should generated in preprocess functions and print the status or and empty string. Also, is it really necessary to add all this markup?

<?php if (!$status && !isset($preview)): ?>
  <h2 class="element-invisible"><?php print t('Status'); ?></h2>
  <div class="element-invisible"><?php print t('Unpublished'); ?></div>
<?php endif; ?>

I don't think so...

Come to think of it, we already have a theme function theme_mark(), which exists for this sole purpose and is already styled (colored red) by default. I don't understand why we are not using it in more places, like in the comment.tpl.php file to indicate a new comment. It would be a good fit for the core solution IMO. Sure, it needs to be improved, and it's probably out of scope for D8, but so are a lot of other, equally important issues.

It's very important that we do not undo the work that was done to improve consistency and to clean up template files. I'd also like to point out that themes have access to the status of each of these entities and can easily implement a solution, in case that's not clear. We should not just throw markup at template files and call it a fix or an improvement, as tempting as it may be and especially, not at this stage. If we are going to fix this, we need to do it right, or do it in Drupal 8.

OK, how about we pursue the theme_mark idea and go for a maintainer review to see where we're at for D7, I too would live with it being shunted to D8 for a proper fix.

@Jacine I don't know if the the H2 line is added.

It would be really nice if we could get the output of #25 into core. Even if it only improves the situation for screen readers, it doesn't add to translation issues. I've worked on bringing this into the templates so it has less impact for themers by adding this to the template.php file in Bartik:

/**
 * Override or insert variables into the node template.
 */
function bartik_process_node(&$variables) {
  $variables['unpublished'] = (!$variables['status'] && !isset($variables['preview'])) ? '<div class="element-invisible">' . t('Unpublished') . '</div>' : '';
}

/**
 * Override or insert variables into the comment template.
 */
function bartik_preprocess_comment(&$variables) {
  $variables['unpublished-comment'] = ($variables['status'] == 'comment-element-invisible') ? '<div class="element-invisible">' . t('Unpublished') . '</div>' : $variables['status'];
}

and the line Jeff suggested to the comment.tpl.php & node.tpl.php files:

  <?php print $unpublished; ?>

I'm missing something for the comments though so that it isn't working.

I'd like to get this set up in a way that is visible, but @sun's said no additional css on this issue & there may be concerns about design changes.

It could be that we have to wait for D8 for this, but if we bring it in for screen readers there will be a precedent for D8 so that this can be addressed better for everyone.

I'm a little perplexed why we want to hide this - that seems like a design decision best left to the themer.

The only reason to make the text invisible as far as I am concerned is to make it possible to get into D7. Otherwise I'm all for going the route that Zen has or some similar way to theme it up.

If we can get a design decision in for D7 I'd be very happy!

Treading here gently, because I read through the issue queue quickly and am still a little foggy in the head from some dental work this afternoon. But:

1. As Jeff has suggested in #34, this is an accessibility issue. With only a faint color and an "Approve" link to indicate that an item has yet to be published, people with cognitive disabilities — WCAG and all the laws based on it cover them, too — would not easily recognize which items are not yet published.
2. The watermark-type fix depicted in #3 would not work for people with reading disabilities or moderate low vision. Heck, I can hardly read it, and I don't consider my vision to be impaired.
3. As Jacine noted in #20, the best way to resolve these accessibility issues would also resolve accessibility issues for people who use screen readers: Add a simple, clear statement of the status of the item right after the item's heading.

So it seems to me that we need a patch that creates content — a clear statement of the item's status, just like the models Jacine gave in #20 — and leaves it to the theme in use to style that content appropriately.

Too much for D7, even in a point release? Then let's work it into D8. But that is certainly not my call to make.

Ok, so if we take the core themes and just add add:

  <?php print $entity_status; ?>

to the comment.tpl.php & page.tpl.php files. If the output from that is:

 <span class="element-invisible">This [entity] is [status].</span>

So the output is invisible by default and does not change any of the visual look/feel of the site (and heck for books in publication changing this would be a pain). This would made it possible for screen readers to know that a node/comment is unpublished. It has no significant changes to anyone else that I can see.

All the logic would go into the template.php files. I really think that this should go in core for D7. Even in a point release.

We need to work on this for D8 so that it is visible & properly themable. However, I think we can leave that for D8.

It was already mentioned earlier that the markup contained in the template variable has to be generated by a generic theme function. Using the theme function, site builders and themers can tweak the output in whatever way to their likings. For D7, the default theme output uses .element-invisible.

However, I have the impression that there is some sort of disagreement about the usefulness of this textual status representation? Would be great to see an unbiased summary of the discussion.

It seems that we are in general agreement that for D7 we should leave the visual display as-is and add invisible text to improve accessibility. This was first suggested by @sun in #13 and this idea seems to be a consensus. For D8 we can return to the idea of using visual wording in addition to the pink background (or instead of the pink background).

@Sun

I think that there is agreement on the usefulness of the text, if anyone implied that it was not useful it may have been me. I do think the text is useful, I was trying to assess how severe of a problem there currently is, since I haven't had any problems myself.

As I understand it the following is required.

1. Enhance theme_mark() to allow for new mark types.

2. Possibly enhance theme_mark to provide the ability to pass 'classes' as a key in $variables, this is so that element-invisible can be passed, not to add any new CSS to Core. I prefer the mark not be invisible, but am content either way.

3. Provide a $unpublished variable in preprocess functions (which will get its value from theme_mark).

4. Add print $unpublished; to entity template files.

Other than that this may be (likely is) out of scope for D7, does anyone wish to correct or object to the above?

Ok, so for the #1, are we looking for something like:

<?php
function theme_mark($variables) {
  $type = $variables['type'];
  global $user;
  if ($user->uid) {
    if ($type == MARK_NEW) {
      return ' <span class="marker">' . t('new') . '</span>';
    }
    elseif ($type == MARK_UPDATED) {
      return ' <span class="marker">' . t('updated') . '</span>';
    }
    elseif ($type == MARK_UNPUBLISHED) {
      return ' <span class="element-invisible">' . t('unpublished') . '</span>';
    }
  }
}
?>

As far as passing CSS through, I suppose one could just extend the array that's passed through by adding:
$class = $variables['class'];

Having the default for new & updated to be marker & the class for unpublished to be element-invisible.

What else can we document before someone wraps up a patch?

3 days before RC, this is going to be D8 material.

Ok, this issue has been cold for a while. How are we going to ensure that we do a better job in Drupal 8? Unpublished content really shouldn't just be a very pale pink. Can we set a better default? I do think Zen set a good example here. Are there others that have been used in D7 themes we should be considering?

It seems like Everett's summary in comment #41 is the best summary of a plan for moving ahead.

If anyone objects to that consensus, please speak up.

Ok, this needs to be cleaned up and tested for the bot, but wanted to put up a more recent patch with /core/ to get us moving with Everett's suggestion above.

Oops.

Looks like this needs some work on the unit testing.

Rerolled.

With the implementation of a draft state in Drupal 8 this will be even more important to clarify.

#51: comment-node-unpublished-867830-51.patch queued for re-testing.

re-roll with attempt to fix notices.

#55: comment-node-unpublished-867830-55.patch queued for re-testing.

Fatal error: Call to undefined function bartik_theme_mark() in /home/s071daa1dd4b1f22/www/core/themes/bartik/template.php on line 82

Failed on simplytest.me when switching to unpublished content.

Not sure how I missed this, but I wasn't respecting theme_mark().

This should resolve that error. Thanks for reporting it Oaryx.

Fixed the comment to give the correct constant name
Refactored theme_mark to add a drupal_clean_css_identifier for sanity, and so we don't have so many identical spans
Improved bartik_preprocess_node and bartik_preprocess_comment to use the MARK_UNPUBLISHED constant

Thanks,
Pete

#59: comments-node-unpublished-867830-59.patch queued for re-testing.

I hadn't run into this function before:
http://api.drupal.org/api/drupal/core!includes!common.inc/function/drupa...

Like your re-factoring of theme_mark(). I do hope that the bot was just buggy as it looks good.

Just got this error on creating a new page though:
Fatal error: Call to undefined function bartik_mark() in /home/s1c6ed7f5a1c61ca/www/core/themes/bartik/template.php on line 82

I'm pretty sure that test fail in HistoryTimestampTest can't be related to this. Rerolling to fix the issue #62.

Setting needs review.

Oh dear, I see the problem here...

This shouldn't be like this:
+ <?php print (isset($unpublished)) ? $unpublished : 'fiwehgewiuf'; ?>

I also couldn't see any text associated with the unpublished comment.

Something was lost between #51 & #55.

Here's the same patch with a few corrections.

Unpublished is under the Comments. This needs to be changed in CSS.

@mgifford - where should unpublished show for each comment?

Good question.. I think in this case it might make sense to just float right. It shouldn't overlap other text or lines and should be within the pink.

I added some more usability tags and hope to get some feedback from Bojhan.

This probally needs a bit of design, I dont think we should randomly float this - this is part of Bartik. Can I see all the cases this shows up and I will design how it should intertwine.

Thanks Bojhan!

#69: comments-node-unpublished-867830-69.patch queued for re-testing.

Reroll as the filename changed to bartik.theme

#77: comments-node-unpublished-867830-77.patch queued for re-testing.

There where whitespace errors in the last patch.

There were 2 failing tests in Drupal\custom_block\Tests\CustomBlockTranslationUITest but those are probably un-related. I hope..

#80: comments-node-unpublished-867830-77-nowhitespace-errors.patch queued for re-testing.

This is all likely going to need to be re-written in Twig. I still don't know where the best docs are to guide the transition over from phpTemplate.

Think this addresses the twig changes.

Go bot.

#88: comments-node-unpublished-867830-87.patch queued for re-testing.

re-roll

#92: comments-node-unpublished-867830-92.patch queued for re-testing.

Reroll...

#96: comments-node-unpublished-867830-96.patch queued for re-testing.

#96: comments-node-unpublished-867830-96.patch queued for re-testing.

Reroll.

103: comments-node-unpublished-867830-103.patch queued for re-testing.

reroll

What happened to return ' <span class="marker">' . t('updated') . '</span>';?

The patch needs a re-roll as theme.inc has changed.

This needs to be refactored as the pink is just looking bad on the text it's sitting on too.

EDIT: Note, this is a screenshot of the unpublished node, not also the unpublished comment.

This is a re-roll but I haven't addressed changing the "bad" aspect of aesthetic.

This really is something we should fix in D8.. It just looks bad..

Fatal error: Call to undefined function theme() in /core/themes/bartik/bartik.theme on line 140
$variables['unpublished'] = theme('mark', (array('type' => MARK_UNPUBLISHED)));

That's probably what's causing the failures... That only popped up when adding an article.

Let's try this using _theme() instead.

missed a theme()...

Edit: That works now in http://s53abb3f3182618d.s2.simplytest.me/

Unpublishing a comment looses it's state.

We really need to be able to unpublish comments....

EDIT: This is a problem in core that is unrelated to this patch.

Wanted to add the styling back in as per the "bad" aesthetic...

#119 Got the CSS wrong.. It's probably in nearly the right place, but the patch clearly needs work.

Old patch needed a reroll. Still needs styling..

Well, there's styling to the node now, but it's the wrong comment.

Plus need to style the comments.

I think the "Mark content" functionality is broken.

It doesn't seem to be used at all for comments from what I could tell.

With nodes it's not clear how to set it. the only place I could see it being set was drupal_common_theme() and with MARK_NEW. Not sure how to get it to be another mark logically.

Is this better documented somewhere?

This was tagged for committer feedback back in 2010 as to whether it could be allowed in D7, so I'm going to remove this tag. ;) The issue could use an up-to-date summary, though. The approach in the patch with the constant seems off to me, and doesn't seem to have anything to do with fixing a WCAG violation. I've added a beta evaluation as best I understand it, but please correct and add up-to-date details to the summary.

+++ b/core/includes/theme.inc
@@ -1428,6 +1433,29 @@ function template_preprocess_tablesort_indicator(&$variables) {
+print_r($type);
+exit();

Pretty sure that shouldn't be there.

Adding needs reroll, because I assume after 2 months it's gunna to need it. Lets get this finished.

Rerolling #125

On #125:

1. +++ b/core/includes/theme.inc
   @@ -1428,6 +1433,29 @@ function template_preprocess_tablesort_indicator(&$variables) {
   +print_r($type);
   +exit();
   

   Omitted in the reroll

2. +++ b/core/modules/node/node.module
   @@ -251,6 +251,9 @@ function node_mark($nid, $timestamp) {
   +  // if (!$node->isPublished()) {
   +    return MARK_UNPUBLISHED;
   +  // }
   

   $node is not available here, so this chunk we will have to figure out I suppose.

   I have left it there, commented as in the previous patch.

3. +++ b/core/themes/bartik/css/style.css
   @@ -839,8 +839,8 @@ ul.links {
   -  margin: -20px -15px 0;
   -  padding: 20px 15px 0;
   +  margin: -10px 0 0;
   +  padding: 15px 0 0;
    }
   

   Core had changed the padding here, and had removed the margin.

   I have changed that to have our styles, we can figure out if we broke something later on.

Attached the reroll.

1. Thanks. Sorry that got left in there.

2. So if we don't have access in node_mark() can we bring this into mark.html.twig - certainly the docs in that template should be changed to include references to MARK_UNPUBLISHED

3. I was going to capture a screenshot, but hit "Fatal error: Call to undefined function _theme()". Looks like _theme() got removed. Not sure what to replace it with.

At some point since #1939092: Convert theme_mark() to Twig, theme_mark() was returned (it had been removed). I'm thinking we need a new issue to bring back the marker, then continue with this issue that's focused on the styling?

I've attached a reroll that also attempts to address 2 and 3 from #136. Fixed the WSOD, but doesn't make the marker show up.

Also, is the "unpublished" marker something that should be in core, or just in Bartik?

This is a screenshot of unpublished node & comment

The 1st comment is unpublished the 2nd is published. There's no difference if the node is unpublished. Probably not all that important.

Thanks for letting me know what did happen to theme_mark(). Setting up a new issue to add back in the marker would probably make sense. So postpone this one until that one is fixed....

It's important to have the semantic information available to let screen readers know that something is published or unpublished. Right now there is no way to indicate this.

I do think this should be a pattern in Core for all themes & not just Bartik & Seven. We want the default unpublished text to contain information in it which improves accessibility & usability. Being able to clearly know what nodes are unpublished will help with this.

Patch no longer applies.

Rerolling...

Rerolled.
Comments on the conflicts fixed:

1. +++ b/core/modules/system/css/system.theme.css
   @@ -8,6 +8,30 @@
   +  box-shadow: 0 -8px 0 #d8d8d8;
   +  z-index: 0;
   +}
   +.node-preview div.unpublished {
   +  display: none;
   +}
   +.preview .comment div.unpublished {
   +  display: none;
   +}
   +.marker {
   +    color: #d8d8d8;
   +    font-family: Impact,"Arial Narrow",Helvetica,sans-serif;
   +    font-size: 75px;
   +    font-weight: bold;
   +    height: 0;
   +    line-height: 1;
   +    overflow: visible;
   +    text-transform: uppercase;
   +    word-wrap: break-word;
   +    float: right;
   +    position: relative;
   +    left: -50%;
   +    text-align: left;
   +    z-index: -1;
    }
   

   The file system.theme.css is now gone, placed these on /core/themes/bartik/css/components/node.css

   Someone from the Bartik team should take a look at this change.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -76,9 +100,6 @@ label.option {
   -.marker {
   -  color: #e00;
   -}
   

   This was moved onto core/modules/system/css/components/form.theme.css - removed from there now.

3. +++ b/core/modules/system/templates/mark.html.twig
   @@ -18,5 +19,7 @@
   +  {% elseif status is constant('MARK_UNPUBLISHED') %}
   +    <span>{{ 'unpublished'|t }}</span>
   

   This file now contains no markup, so I added this without the span.

   Made the change as well inside core/themes/classy/templates/content/mark.html.twig with the span class="marker"

4. +++ b/core/themes/bartik/templates/comment.html.twig
   @@ -74,6 +74,7 @@
   +    {{ unpublished }}
   

   This whole file's markup was redone, so ive added as what I see the equivalent place as it was before. We should probably do testing on this.

I've also added the changes we make to node and comment templates on the classy theme.

Fixing several nits and hopefully the errors. Details in interdiff.

Please revert file mode changes on commit (or in the next patch). I am not sure why they happened.

Thanks @hussainweb for fixing the failing tests!

Restoring the file mode changes...

Glad to see progress here. Took a look at the patch. Now technically, the light grey/pink really don't have enough colour contrast, but they are really, really big http://leaverou.github.io/contrast-ratio/#%23FFF4F4-on-%23d8d8d8

Anyways, I don't think we can deal with that as we really want it to be a background to the main black text.

I attached to screenshots though of problems with the CSS.

When the screen is reduced, the text extends beyond the left hand margin of the text for the node body. It also gets cut off for the comment body. It's hard to tell but both the node & comment are unpublished here:

Also, if the body is published but the comment isn't the 'unpublished' text just doesn't show up:

I think the unpublished text needs to be re-sized based on the size of the body that contains it.

Sorry to push this issue in another direction but I'm not feeling this new design either. How about we have something like this instead?

@LewisNyman I like that idea better because you can see better how the node will look like after you publish it.

That looks way better. I've got no problem with that approach. We'd need to make sure it was accessible in its implementation, but something like that would be as visible as what we have now (if not more so).

Yeah, I'd much rather have a label than the background. I've always disliked the background because the interference with the node's display.

I like the label concept. I think its a nice improvement and gets rid of the pink background :-)

Nice one! This makes a lot of sense. Color choice in line with the style guide as well :-)

This would also be my prefered way of doing it, the only challenge is that no one ever themes this stuff. So it needs to look really good by default and/or we should show a very nice looking practice in Bartik

Yeah this styling should go into Classy, like with the messages.

@Bojhan It would be great if it looked really good by default. However, I'd settle for it looking better than what we have now & being accessible.

@LewisNyman are you going to write up a patch for review? Maybe this can be fixed up quickly now that we've got a critical mass of folks who seem to like this approach.

I went quite far with this patch. I seemed quite difficult to add this to the title and all the unpublished logic is in the node.html.twig file. I realised that we had to solve the same problem. for comments, so I created a reusable component that could be used in other situations going forward. It has similar success/warning/error options that messages does. This could probably get turned into a theme component in 8.1.

Clean up comments for the new CSS, so they are consistent with the other component CSS files.

#2476947: Convert "title" page element into a block in theory could help with this if you want unpublished after the titled, as it could allow the unpublished badge to be added to title_suffix with a preprocess_page_title as Shortcut module will do if that goes in.

Clean up comments for the new CSS, so they are consistent with the other component CSS files.

@googletorp Do you think that the information before was more useful than the information your replaced it with?

no one ever themes this stuff

No one's ever really "themed" it because there wasn't really anything to theme before. It was just a class that allowed you to change very little about the div in question. This is technically replacing this existing implementation with a whole new, and entirely separate, element to be themed. I know for a fact that people do theme the new/updated labels found elsewhere through out the site (e.g. admin content view, comments).

No, what this issue should now be proposing is to add a new "mark" type (node_mark() and mark.html.twig), not introducing a whole new (and separate) "status label" element.

Subsequently, this is now a feature request and no longer a bug. Especially considering that this is now adding new elements and removing old ones (which is technically a BC break btw, someone may actually be using this class and introducing their own :before pseudo element).

Color choice in line with the style guide as well :-)

So we're using Seven's style guide for Bartik now? I understand that Bartik doesn't actually have a style guide, but this somehow seems like a very wrong direction to be heading towards.

I agree with adding a label for this information, however not so much with the color choice. It looks rather out of place. I would prefer that we keep this in the red hue range.

I was under the impression that this was a generic solution. Since the checkbox for 'edit content in the admin theme' is on by default in standard profile it seems a good default to base this off of Seven. Wouldn't hurt to pick a better color for Bartik indeed.

I don't see how a different kind of solution turns the initial problem from bug into feature request though.

Subsequently, this is now a feature request and no longer a bug. Especially considering that this is now adding new elements and removing old ones (which is technically a BC break btw, someone may actually be using this class and introducing their own :before pseudo element).

We are fixing a bug by designing a solution that is reusable. I don't consider this a feature request. This is the right way to solve these kinds of problems. Also, we can break markup BC before Release Candidate 1.

No, what this issue should now be proposing is to add a new "mark" type (node_mark() and mark.html.twig), not introducing a whole new (and separate) "status label" element.

Ah nice, it would be great to make this a simple template that we can apply the classes to. Could you help with this? We still have time before RC.

So we're using Seven's style guide for Bartik now? I understand that Bartik doesn't actually have a style guide, but this somehow seems like a very wrong direction to be heading towards.

We use the Seven style guide colors for messages, the Seven style guide is the closest we have to a color scheme, if Bartik wants to override the defaults then we could do that. It doesn't at the moment though.

I agree with adding a label for this information, however not so much with the color choice. It looks rather out of place. I would prefer that we keep this in the red hue range.

Red means "error". I imagine a workflow state like "needs work" would use this status. Unpublished seems more important that just "information" but less important than "something is wrong".

@LewisNyman I figured that we want consistency in core and comments that abide the coding standards, both got fixed with the patch I made.

@googletorp:
Admin components are a lot more complex than components in a standard visual theme and may need more description. I think the comment change in #165 gives the user less detailed information, which is not what the coding standards suggest.

Well commented code is extremely important. Take time to describe components, how they work, their limitations, and the way they are constructed. Don't leave others guessing as to the purpose of uncommon or non-obvious code.

— https://www.drupal.org/node/1887862#comments

+++ b/core/themes/classy/css/components/status-label.css
@@ -1,9 +1,6 @@
 /**
  * @file
- * Status labels can be used to signify the status of system entities.
- * eg. Unpublished nodes or comments.
- * @see node.html.twig.
- * @see comment.html.twig.
+ * Visual styles for status labels.
  */

@emma.maria Coding standards dictate that the file doc should be

• 1 line explaining the the file is about
• optionally a blank line and a more detailed description

The initial line is too long and doesn't explain what is in this file and there is no blank line.

Status labels can be used to signify the status of system entities. eg. Unpublished nodes or comments.

The comment explains what a status label is, it should be explaining what the file is used for (visual styles for status labels).

Also I would like to argue that we shouldn't document what a component is, in a CSS file, since that doesn't have anything to do with what the component is, only how it should appear on the screen. This documentation should be in the node module or wherever we define this.

The reference to the files could be ok, but I opted out of this, since it's not done anywhere else.

Anyways the file comment is a small thing, I believe it's more important to actually solve the issue.

Lets gets a working patch first before discussing comments anymore, I would hate to take the focus away from the most important thing here. If you want to change the comment, then fine by me, but please fix grammar and the overall standards for comments (so machines like api.drupal.org can read the comments and expose them to users).

A somewhat related issue at least for how we choose to style this in #2290101: UI telling you a field is shared across languages is way too subtle

Ah, thanks :)

The patch needed a reroll.

I had a discussion with Maouna about #2290101: UI telling you a field is shared across languages is way too subtle and how we can share the same implementation. If we take @markcarver's recommendation and reuse mark.html.twig we are going to have to expand it considerably to accommodate more than just the node markers.

This is the code that they would expect to use in #2290101: UI telling you a field is shared across languages is way too subtle:

$suffix = array(
  '#theme' => 'mark',
  '#status' => 'shared',
   '#type' => 'warning'
);

The current mark.twig.html implementation looks like this:

{#
/**
 * @file
 * Theme override for a marker for new or updated content.
 *
 * Available variables:
 * - status: Number representing the marker status to display. Use the constants
 *   below for comparison:
 *   - MARK_NEW
 *   - MARK_UPDATED
 *   - MARK_READ
 */
#}
{% if logged_in %}
  {% if status is constant('MARK_NEW') %}
    <span class="marker">{{ 'new'|t }}</span>
  {% elseif status is constant('MARK_UPDATED') %}
    <span class="marker">{{ 'updated'|t }}</span>
  {% endif %}
{% endif %}

We'd have to open it up so you can pass any status in there, and also remove the logged in requirement, which seems like a weird layer to check for this anyway.

Here's a patch that completely rewrites the implementation of mark.html.twig. No big deal. Look at these lovely markers.

Whoops. Somehow I missed that I had already applied a patch.

Me and @thewilkybarkid will be looking to address the failing tests! Wish us luck!

Good luck :)

I will be reviewing the code changes and @thewilkybarkid will be looking into some of the failing tests.

1. +++ b/core/includes/theme.inc
   @@ -32,21 +32,6 @@
   -const MARK_NEW = 1;
   -
   -/**
   - * Mark content as being updated.
   - */
   -const MARK_UPDATED = 2;
   

   These constants are still referred to in mark.html.twig

2. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -36,7 +36,6 @@
     *     admins.
   

   'admins' also needs to be removed if you are getting rid of node--unpublished

+++ b/core/modules/comment/comment.module
@@ -736,9 +736,20 @@ function template_preprocess_comment(&$variables) {
+  if($variables['status']) {

This condition causes an exception to be thrown if $variables['status'] is undefined.

This patch address this.