Unpublished comments have a styling issue

OK what are we looking at here Jacine, are those meant to be threaded and the time/date is wrapping?

What is the problem with the HTML? What do we want it to be? I also like testing with http://wave.webaim.org/toolbar

Yes, we're supposed to be deprecating use of color to indicate unpublished status and instead use actual text, if you've ever used Zen you'll know what I mean.

Ok, so so can we just re-use their approach? If so, can you roll a patch?

I'm assuming this is a duplicate:

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

Yes, but what issue are you pointing out with the screenshot in #2 - I see a number of issues, the comment "arrow" is not transparent, else the background is just pink - I'm not clear what you are pointing out here. If its the fact that no "Unpublished" text appears then yes this is an issue and should be fixed, but not in Bartik, it should be in core - to provide an easy printing variable, as sun points out in the other thread.

so just to be clear what we're talking about.

It's taking the unpublished code from for Bartik.
#867830-3: "Unpublished" style of rendered entities is not accessible (and looks bad)

And applying it just for this core theme.

I'd certainly approve of that if we can get the Bartik team behind it.

Should nodes have been included too?

This issue always scares the hell out of me when I see it in the queue, so re-titling to something more accurate. ;)

Ya, that makes sense. Broken is scary.

There are accessibility issues that are being addressed. Unpublished nodes & comments have always been broken for screen readers and for folks with low vision.

#867830: "Unpublished" style of rendered entities is not accessible (and looks bad) should have a generic solution that makes it accessible to screen readers for all themes. The patch I proposed is just Bartik. I think they want to follow more of a Zen type approach to unpublished content.

Anyways, I slightly modified the title to make it Bartik specific & ensure that it wasn't just a styling issue.

I have yet to see any evidence that unpublished entities, including comments, pose a problem to screen-reader users.

I'm sorry Everett. Maybe I'm missing something. However, without this patch the only way I can see that you'd know that a node or comment were unpublished would be because you were looking at the CSS classes.

If you can determine the published state when viewing a node, please let me know how. I don't know where you are perceiving the state other than in the add/edit form.

@13 - this is a very simple test Everett, if I sit two nodes side by side and one is published and the other is not, can you tell me which one is published and which one is not? Nothing extra Everett, can you tell me strait off, without going anywhere, doing anything special, which one is unpublished? If you cant, then its a fail.

It does not matter if you have access to admin pages where you can filter for these, we're talking about the front end site, the main river of news, a taxonomy term list of node teasers, or viewing a node with all its comments, directly.

Sighted users, with good vision, can. They have this feature, and its a good one - very handy indeed.

In Genesis I solved this problem by generating a warning message and adding an anchor link to unpublished comments inside the warning message - probably not the solution we want in core, nevertheless that is my solution in my contrib theme.

I was not aware that an unpublished entity would ever be shown to a user that doesn't have the permissions to change the state of the entity. If this is the case then yes, we do need to design a solution to the problem.

I was not aware that an unpublished entity would ever be shown to a user that doesn't have the permissions to change the state of the entity.

They're not. At least not unless you have a bug in your view and aren't filtering it properly.

I think a better test is, open up two browser windows. In one, go to node/1 where node 1 is a published node. In the other, open up node/2 where node 2 is an unpublished node. Now walk away from your computer for a few minutes while your cat attacks a piece of yarn all over your keyboard. When you come back after chasing the cat away, can you tell which page holds the unpublished node vs. the published one?

That's totally funny description.. Thanks webchick! This is a good joke site for folks who co-habitat with cats. Sadly it doesn't use alt tags very well.

@Webchick

Makes sense, thanks for the example.

Unpublished comments, AFAIK, should always be perceivable as unpublished, since there's a link right there that says "Approve". But, an unpublished node is not perceivable as unpublished unless in edit mode.

P.S. I'm not being argumentative here, I just want to fully understand the situation so that we can make sure that we are implementing the best solution.

Ok. We're not talking about anonymous users seeing unpublished content. I suppose it is possible, but the realistic case is you've got a wiki in Drupal with some simple workflow. A team is working on a bunch of documents, at first they are unpublished, when they are happy with them someone on the team promotes it as published so the world can see it. One member of the team is blind, one member of the team say can't view color. Everyone else on the team can see a list of items that they can edit and easily see which items have been promoted & which haven't by simply viewing the node.

I've had to blow away my sandbox a few times, but if you had access to the site (please re-apply), you'd be able to see this page with the title 'zz' highlighted in a light pink:
http://drupal7.dev.openconcept.ca/node/1

You would learn (if you could see the pink) to notice the color to be able to quickly determine if a page you are viewing is visible to anonymous users or not. You would confidently know that 'Page 2' was visible to everyone because it didn't have a highlight color around it:
http://drupal7.dev.openconcept.ca/node/5

What the Zen theme does is take unpublished content & present it with a more obvious background text that says it's Unpublished rather than relying on a faint color.

I don't think there's any way that the faint color passes WCAG 2.0 A. Heck, I know it's there and it's not that obvious. However, when we get this patch into core screen readers will know that a node or comment is indeed Unpublished from the View screen and it can more easily be themed up like Zen does to provide better highlighting for more accessible themes too.

I do hope that the links help to clarify where we are expecting to see notification about it being in an unpublished state.

Jacine - with all due respect this has not been hijacked and nor are we arguing - its all part of one issue - design changes require accessibility review - both design of the system and the visual design of the theme. At almost every point where design changes took place either I made accessibility reviews of them, and where I felt the need tagged them as needing accessibility review to invite the a11y team to be part of the review process, just as I might tag for UX review or markup review.

Now, I think if you really want to point the finger it should be at me, not Mike. I am the one who has aggressively pushed the accessibility angle. I sincerely apologize if you feel this is out of scope of the issue and are frustrated by process, however there are issues at play here that need to be discussed - I would rather get this right now, rather than coming back to it again later.

FWIW your citation is no longer relevant, it refers to candidate themes, but Bartik is not a candidate, its part of core now. We should not just fix things in Bartik so it alone benefits, if at all possible we fix the root of the problem, so everyone can benefit - this reduces special casing and needless duplication.

So, to balance the conversation we need to discuss what needs to be done in terms of visual design and get some peeps into writing patches and building new images to support that. Sorry I have run out of time right now, but IMO the outline of the comment body should contain the color change for unpublished comments (yes I do think they should change color, this is a very loud visual clue for sighted users that something is not published). This mean the arrow needs to change color - so we need a new arrow sprite and a bit of CSS.

@Jacine I am sorry if I went a direction you weren't expecting with this. I'd love for you to provide a patch to provide a better example of what you are seeking. It's not clear to me if you were bringing up this issue as a design issue or a usability issue. I think the pale pink is pretty bad from all angles. I'd much rather have the Zen approach to this.

There were a few related posts about unpublished entities, and this is one. I may well have overlapped them inappropriately. Heck, I even closed this post assuming it was a duplicate of the others, but you opened it up again to say it needed to get fixed in Bartik. So please provide a patch providing what you would want it to be.

I'm not trying to push folks around on this issue, but it became clear (pretty late in the game as you know) that there are quite a few folks who won't be able to perceive the difference between published & unpublished nodes. Any effort to improve that in Bartik & in core would be appreciated.

And no, not every accessibility issue can possibly be fixed in D7, even in the point releases. However, I do think that there are many similarities in the introduction if multi-lingual drupal functionality in D6. D6 needed to rely on i18n to create a bilingual site and to enhance multi-lingual support between major releases. Features were added in the i18n module & bug fixes continued to be added in D6 point releases. There are going to be more accessibility issues in D7 than we've had time to investigate & fix by the time of release of D7. Only some of them will be able to be addressed in point releases of D7. I suspect that others will be addressed by the Accessible Helper Module or themes like AdaptiveTheme that are working to be super accessible. Of course the closer we get to the final release of D7 the harder it will be to get any change into core. That being said if fixing an accessibility bug doesn't have a big impact on the other teams, there is a good chance it will be able to be fixed with other bugs in maintenance releases.

Accessibility isn't more important than other issues, but it does need to be given proper consideration.

I'm actually going to side with Jacine on this one. This issue is killing kittens and blocking what should be a very straight-forward fix.

This issue should be about only the following:
1. Fixing the stupid white background of the arrow thingy on the dialog box.
2. Fixing the spacing of comment links in RTL so they're consistent with LTR.
3. Fixing whatever else is broken in terms of fixing the existing styling on unpublished comments. (the above were the only things that stuck out to me)

#867830: "Unpublished" style of rendered entities is not accessible (and looks bad) is where we can discuss what further accessibility improvements, if any, are allowed on comments at this stage. Hint: Not much, if anything at all. We're post-beta now.

Agreed with @Webchick in #24

I wasn't aware that this issue was about anything but fixing Bartiks comment styles? I thought I made that clear in #8.

FYI there is one other far more important issue for comments #895898: Comment user picture can overflow its container

Thanks @webchick for clarifying this issue.

Hoping to tackle this tomorrow.

From jacine in #21:

The problem is simple: When comments were designed originally, unpublished comments were not taken into account. I thought this screenshot was a good illustration of the problem.

Yup.. We did a lot of work on comments, but until this issue, no one looked at unpublished comments. Clearly the background is targeted to and awkward part of the HTML. It's not a complicated fix.

From webchick in :

This issue should be about only the following:
1. Fixing the stupid white background of the arrow thingy on the dialog box.
2. Fixing the spacing of comment links in RTL so they're consistent with LTR.
3. Fixing whatever else is broken in terms of fixing the existing styling on unpublished comments. (the above were the only things that stuck out to me)

I'm on it.

The rest of this discussion, about whether or not marking with color is a good idea in the first place, or is accessible... that should all go to another issue.

Proposed change:

BEFORE

AFTER

I've also been looking at the unpublished node styling.
TOP

BOTTOM

First of all, there is a new bug with those arrows. We made them transparent a long time ago. They should not have a white background. So we will fix that in another issue. That fix is needed for many reasons — the ability to recolor the whole page is the biggest one. So please ignore the fact the arrow is white.

Can you please provide descriptions of your screen-shots.

Is there any way to make that white < thing next to the talk bubble pink instead of white? That looks ooky.

After lots of in-person discussion with timplunkett and webchick, this the deal. The biggest problem is the fact the comment arrow is white, and not clear. This is not only a problem for unpublished comments, but for every comment everywhere on a site where the background color if anything but white.

My vague memory is that we already fixed this bug months ago — and this is a regression... but also, I can't seem to find proof of that. In any case, it just needs to be fixed.

Beyond the needing-to-be-clear arrow, I don't see any other visual fixes that we can make. I think that the way this is already is the best way to leave it.

Oh I meant to make this needs-review.

I keep commenting on this too soon. Sorry. I'm very tired and and vomiting my thought process all over this issue.

Meanwhile, I'm still working on this.

Ok, here's my fix. This patch does 4 things:

1) Adjusts the background that goes behind the entire node (including submitted info, the node itself, comments, and comment submit form) so that the sandy-pink (a color chosen in core) extends 10px or so beyond the edge of the node.

2) Does the same adjustment to unpublished comments, so that the edge of the unpublished-comment-background is about 10px away from the edge of the content, rather than touching it.

3) Replaces the arrow for comments that points to the comment author information with an arrow image that is translucent, rather than one that has a white background. This fixes it for when the unpublished background shows. It also fixes it for when the site owner has changed the background color of the page. There's both a new comment-arrow.png and comment-arrow-rtl.png.

4) Adds a little CSS fudge to the comment arrow image — placing a 1px border on the left and right so that as the page background is recolored, that line is also recolored. It also changes with the unpublished state. The border is needed to cover up the border of the box that goes around the comment content, so that the arrow looks like an arrow that sticks out of the box, rather than one that's next to the box (separated by a line). This line is applied to both the left and right sides of the image — instead of one side for LTR and the other for RTL, because otherwise we'd need a colors-rtl.css, which likely will not work. Instead it works to just have the line on both sides of the image all the time.

Screenshots of what I just explained:

I have not tested this in RTL, so it would be great if someone else could. I have also not tested it in IE.

That patch looks like it's missing something. What happened to the border-left-width: 0 idea?

I haven't zoomed in on the images or tested this but isnt there a 1px gap between the vertical border and the top and bottom of the triangle? Is it not possible to get rid of that?

Other than that the actual visuals are looking good.

Re #38 — we don't need it. It doesn't work anyway for the reason I explained right before we left the coders lounge at BADCamp. It wasn't really working, and I realized it's not needed.

Re #39. Yes, there is. That's the one flaw with this. A 1px by 1px gap. :( But OMG, I worked on solutions for this for at least 8 hours, with 3 other smart people jumping in with ideas.... this was the best I got to.

Missed the RTL change. Someone with IE, come RTBC this!
I'm not going to re-up the images, but they're still in #37.

Here is an improved patch for this issue.

I fixed the 1px by 1px gap in all browsers and also added some ie6 specific tweaks by using gif images instead of png.

This patch adds three files to bartik: comment-arrow-ie6.gif, comment-arrow-ie6-rtl.gif and ie6-rtl.css.

Screenshots are attached to this comment and the patch and images that need to be commited in the next one.

No screenshots for IE8/Chrome/Opera because they look the same as in FF :)

And the patch. You know what to do with ie6-rtl.txt ;)

A) the patch doesn't apply cleanly because it's trying to create a file.

B) The removable of the -1px makes a line show up in FireFox (3.6 for Mac). It's ok in Safari.

New patch that doesn't try to add that file and a screenshot for FF 3.6 / Win7.

Screenshot from a vm. Looks ok to me..

Edit: Also tried on FF 3.6.12 / OS X 10.6.5 and there was no extra line.

amateescu, do we still use all the images in #43 for this patch, and is the ie6 rtl file no longer required? Just be good if you could briefly explain how to apply the patch/files for reviewers. Cheers.

@Jeff, yes, all the images from #43 are required and so is ie6-rtl.txt which should be renamed to ie6-rtl.css.

To make this easier to review, i'm attaching an archive with the required files. The four images from the archive should go to themes/bartik/images and ie6-rtl.css should go to themes/bartik/css. The required patch is still bartik-928776-43.patch from #43.

I very much liked the patch in #45, I think thats a long way towards a solution. I took that patch and worked it a bit more - basically I don't think we really need to use a png version and can get away with just the transparent gif - I built a new set of images to give us a really sharp line, however the gif optimizations slightly soften it. I made it a few shades darker which all together gives us a very good match to the CSS border.

Take a look and see if you can improve it more, I tested in Firefox, Chrome, IE6/7/8 (all Win7). So far I can't see any show stopping flaws.

You need to apply the patch and add the two comment arrow gif images to the images directory.

Screenshots showing the patch and gif images from #49 in Firefox.

Great! I tested #49 in every browser I could find (IE 6,7,8, FF, Chrome, Opera LTR &RTL/Win7-WinXP and Safari, FF/Mac) and everything looks good.

I'd RTBC this if it weren't for Jen's FF/Mac which didn't like my previous patch :)

For committers, comment-arrow.png and comment-arrow-rtl.png from themes/bartik/images should be replaced with the gifs from #49.

Looks like you did a good round of testing (more than me actually!), I'd say this is ready to go, hopefully Jen can get in a round of testing also, better to be safe than sorry :)

Tested LTR in Firefox 3.6.12 on Ubuntu GNU/Linux and Chromium 10.0.601.0 on Ubuntu GNU/Linux. Looks good to me.

I tested in FF3.6 for Mac (the one Jen had issues with before) and #49 is looking good.
The last of the BADCamp bugs! Awesome work Jen, Jeff, and amateescu.

Jen and amateescu are heatedly discussing this patch atm, so bumping out of my queue for the time being. :)

Ok, I think I did that right.

Removed the old .pngs
Added the new .gifs (careful to remove the _0 thing on the second one)
Added the patch.

If someone could double-check, that'd be awesome.

Committed to HEAD. Thanks!

I checked this with a fresh CVS HEAD and everything is good :)