As per http://drupal.org/node/874370#comment-3426790

This is a quick accessibility follow-up issue to add these icons to admin/reports/status, where we're still only using colour to indicate status.

as with #874370: System messages need identifying icons (WCAG 2.0) If you can't see the color, it's awfully difficult to tell a yellow warning message from a reddish-orange error message. Without the information encoded in the color, both are simply a description of a problem and a link to something relevant.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

Issue tags: +Accessibility

mostly tagging, but also raising a link to a related issue #447816: WCAG violation: Relying on a color by itself to indicate a field validation error that @brandonojc will hopefully be re-rolling soon to address @Dries comments.

mgifford’s picture

Status: Active » Needs review
FileSize
133.58 KB
186.65 KB
160.01 KB
3.25 KB

Patch with appropriate screen shots for review. CSS should get some closer review. I'm not sure why th is being called in the css when td is what is being used.

tstoeckler’s picture

Small nitpick:

+      $requirements = isset($requirement['severity']) ? (int) $requirement['severity'] : 0;

It seems that should be $severity instead of $requirements?

Also (unrelated):
The different colored text in Seven looks really weird. For instance the text for 'warning' and 'error' seems almost identical, but I think it isn't.

markabur’s picture

In Safari 5 Mac, the icons are duplicated in the right-hand column. There has always been weirdness related to assigning background image to a tr in Safari. I imagine this is why d6 used th for the "subject" column and td for the status, and assigned the background image to the th.

I am not seeing icons on "info" rows at all, e.g. Upload progress.

The following css has no effect; padding only belongs in td or th:

+table.system-status-report {
   padding-top: 6px;
   padding-bottom: 6px;
 }

In screen-capture-1_4.png I see some missing borders (between the BC Math and Configuration rows; also between Database updates and Drupal core), but I'm not seeing this problem omm so maybe it's ok.

We will have to remember to patch for RTL too.

markabur’s picture

Related patch where the th was removed: #783534: Make Report table better

yoroy’s picture

Visually, this would be so much more effective if the green/ok one was omitted ("no news = good news") and have only warnings and errors stick out for attention. Would that work from an accessibility / AT perspective as well?

mgifford’s picture

@tstoeckler - that's a better choice of variable names. I've made the change. I can't see the colour difference you are talking about though.

@markabur - Damn Safari. But ya, tested it, and you're right. I've fixed it so that the images are tied to the TD which is more specific anyways. Better. I nixed the padding as you specified too. Thanks for the references to the TH removal.

@yoroy - in this case I think "no news = good news" works. Also addresses @markabur issue about displaying icons in image upload location. It's definitely a differentiator if there is no image.

I'll roll the RTL patch if we've got approval for this.

bowersox’s picture

+1 for this improvement. I haven't tested this patch myself yet, but if we get some additional testing and feedback we could move towards RTBC.

Jeff Burnz’s picture

Status: Needs review » Needs work
+++ modules/system/system.admin.inc	10 Sep 2010 20:01:35 -0000
@@ -2576,15 +2576,16 @@ function theme_status_report($variables)
-      $class = $classes[isset($requirement['severity']) ? (int) $requirement['severity'] : 0] . ' ' . $class;
+      $severity = isset($requirement['severity']) ? (int) $requirement['severity'] : 0;
+      $class = $classes[$severity] . ' ' . $class;
 
       // Output table row(s)
       if (!empty($requirement['description'])) {
-        $output .= '<tr class="' . $class . ' merge-down"><td>' . $requirement['title'] . '</td><td>' . $requirement['value'] . '</td></tr>';
+        $output .= '<tr class="' . $class . ' merge-down"><td class="' . $classes[$severity] . '-image">' . $requirement['title'] . '</td><td>' . $requirement['value'] . '</td></tr>';
         $output .= '<tr class="' . $class . ' merge-up"><td colspan="2">' . $requirement['description'] . '</td></tr>';
       }
       else {
-        $output .= '<tr class="' . $class . '"><td>' . $requirement['title'] . '</td><td>' . $requirement['value'] . '</td></tr>';
+        $output .= '<tr class="' . $class . '"><td class="' . $classes[$severity] . '-image">' . $requirement['title'] . '</td><td>' . $requirement['value'] . '</td></tr>';
       }

OK, bit stumped as to why we need to make all these changes - afaict all we need to do is add some sensible classes to the td.

I do not like these classes: $classes[$severity] . '-image"

I would much prefer we simply add classes to the td's to make them themeable rather than this special case class just for the addition of an icon, for example:

      $class = $classes[isset($requirement['severity']) ? (int) $requirement['severity'] : 0] . ' ' . $class;

      // Output table row(s)
      if (!empty($requirement['description'])) {
        $output .= '<tr class="' . $class . ' merge-down"><td class="title">' . $requirement['title'] . '</td><td class="value">' . $requirement['value'] . '</td></tr>';
        $output .= '<tr class="' . $class . ' merge-up"><td colspan="2" class="description">' . $requirement['description'] . '</td></tr>';
      }
      else {
        $output .= '<tr class="' . $class . '"><td class="title">' . $requirement['title'] . '</td><td class="value">' . $requirement['value'] . '</td></tr>';
      }

Also this patch uses the 24px icons which I think are too big and we should use the 16px icons - this will look very OTT when the user has 7 warnings and 3 or 4 security updates.

Would like to see this tried with no background for OK, agreeing with yoroy that no news = good news.

Powered by Dreditor.

markabur’s picture

Status: Needs work » Needs review
FileSize
76.15 KB
2.3 KB

Here we go, my first patch. :-) Incorporates suggestions from #9.

And instead of removing that 6px vertical padding altogether, I put it on all of the tds. This improves the readability in Stark when the window is narrow and/or the type is large.

Jeff Burnz’s picture

Looking good markabur. I had a rethink about the classes and perhaps generic classes like .title might inherit something unwanted at some stage - I switched to name-spaced classes. I'm a big fan of classes on everything, one of the traditional bug-bears of Drupal has been lack of classes - while strictly speaking we don't need them in this instance, they could be helpful.

Removed the redundant background-image: none off OK, adjusted some padding etc and snuck in a small fix for Bartik...

All we need now are some RTL styles - but I just ran into a bug with Overlay and RTL so busy debugging that first...

Cliff’s picture

Subscribing.

markabur’s picture

Agreed on the title class, it's pretty loaded-up as it stands. I also thought about classing the other cells. Makes sense to do it now even though it's not really need for the changes we're working on.

mgifford’s picture

FileSize
215.62 KB

I like it. Thanks for rerolling this Jeff & Mark. Much better.

Jeff, should we figure out the RTL before marking this one RTBC?

I know that

There's just these two places where it needs to change, right:

+table.system-status-report td.status-title {
+  padding-left: 30px; /* LTR */
 }
-table.system-status-report tr.error th {
-  background-image: url(../../misc/watchdog-error.png);
+table.system-status-report tr.merge-up td {
+  padding: 0 6px 8px 30px; /* LTR */
 }
yoroy’s picture

My designer eye does not like that the icon isn't centered in the available space between border and text, could use a tweak imo.

Jeff Burnz’s picture

Lets nail the RTL and alignment issues first.

mgifford’s picture

@yoroy - could you give me a screenshot of what you would like it to look like? Even better, the modifications to the patch or even the CSS so that we know what you're ideal is.

markabur’s picture

FileSize
3.05 KB

Here's #11 with the icons moved 2px to the right. This is better-centered but might be off by a pixel for certain characters, antialiasing settings, etc.

RTL will be tricky as the right-side background position can't be expressed in pixels afaik.

Jeff Burnz’s picture

OK, I'll apply this patch in 18 and see if I can get something acceptable for RTL, at least we should have a crack at it.

Jeff Burnz’s picture

With RTL, not perfect but about as good as it gets, works OK.

mgifford’s picture

FileSize
173.71 KB
171.67 KB

I'm noting @markabur's related links #665790: Redesign the status report page & #538666: Seven theme not right with the status report screen about the status report here.

@Jeff patch in #20 works as it should in my sandbox although you have to look through all of the RTL bugs that are there and that you've noted before.

I think this is ready to RTBC when the bot's happy with it.

@yoroy is that 2px change sufficient?

markabur’s picture

Shouldn't the "else" be using status-value for the class, not requirement-value?


      if (!empty($requirement['description'])) {
        $output .= '<tr class="' . $class . ' merge-down"><td class="status-title">' . $requirement['title'] . '</td><td class="status-value">' . $requirement['value'] . '</td></tr>';
        $output .= '<tr class="' . $class . ' merge-up"><td colspan="2" class="status-description">' . $requirement['description'] . '</td></tr>';
      }
      else {
        $output .= '<tr class="' . $class . '"><td class="status-title">' . $requirement['title'] . '</td><td class="requirement-value">' . $requirement['value'] . '</td></tr>';
      }

Otherwise, I think it's probably RTBC. I've been lazy and haven't checked in a lot of different browsers, but this is straightforward stuff so I can't imagine what would break. I looked at RTL by switching to Arabic, but without the language actually installed. This switched my layout to RTL but in English. Status table looks fine to me like that.

@mgifford, thanks for completing the other half of my linking.

yoroy’s picture

I is happy

Jeff Burnz’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.71 KB

Class fixed as per #22, its is pretty basic stuff and I've checked in IE6, 7, 8, Opera, Chrome etc, no issues (well, apart from all the other existing brokeness in IE6, 7 for Seven in RTL mode... but that's another story...).

Preemptive RTBC, go bot go...

markabur’s picture

Looks good.

Dries’s picture

I think these icons are ugly but I understand they are required for accessibility. The negatively affect the experience for me, but positively affect the experience of others. I'm obviously in support of accessibility improvements but it would be nice if -- at some point in future -- we could make this look more attractive. :)

To me, the fact that the icons aren't aligned with the baseline of the text is slightly upsetting. ;) They seem to dangle in mid air; see the warning triangle on http://drupal.org/files/issues/screen-capture-3_6.png, for example. Am I the only one who is bothered by that?

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs work

OK, lets try to align them better, this probably means line-height adjustments and/or using a relative offset for top rather than 50%, such as ems - this actually looks very good in some browsers but clearly not others, lets try to get this right (at least better),

mgifford’s picture

@Dries I think most of us would be happy with more attractive icons. They are consistent with the ones used in #874370: System messages need identifying icons (WCAG 2.0) at least.

Now there has been some discussion about changing icons for forums #821672: Forum icons not accessible to screen-reader users but as @webchick points out we're well beyond the UX freeze.

As you know there are lots of neat options in #606490: Drupal GPL icons - a softfacade initiative too. Deciding on which ones to use though has been a bit of a challenge.

Fortunately, these can all be changed fairly easily with some CSS. However, although it's easy to change in core, if we're already sick of how the icons look (and D7 isn't released yet) we should be seriously looking at other options because we'll be really sick of them by the time we're ready to move to D8.

Anyways, hopefully Jeff's able to pull off some alignment changes that make it look better.

markabur’s picture

Of course the nice thing about this patch is that if your site is OK then you don't have to see these icons at all! :-)

Jeff Burnz’s picture

Problem is a set of icons, even a few, is a lot of work to build - its takes ages and ages to get them right - and then we have all the really tough hard ass accessibility people (like me) to fight with while you're at it :/

Perhaps we can make it a priority to get some designers on board for much better icons for D8, so far its been yoroy and Mr nobody doing the grunt work to get us some usable icons (D4D where are you?).

@markabur - feel free to re-roll this with improvements, I'm pretty slammed the next couple of days.

markabur’s picture

FileSize
17.11 KB

Sure, I can take a look at some options.

I think mgifford's screen shots are from firefox with the font size set to 22px instead of the default 16px. That is kind of an edge case where normally I wouldn't expect things to line up perfectly, and I'm not sure if it's totally fixable there. See attached for what this patch looks like OMM, which has the default settings. We'll want to get the changes made here first, before trying to make the larger-font scenario work too.

The tough thing I see with baseline-aligning the icon is that if the icon moves up, then it gets oddly close to the top border, but if instead the text moves down, then the table will get pretty spaced-out. I can put up some mockups to show what I'm talking about.

markabur’s picture

Here are some mockups.

In status-icons-alignment.png, (A) is how the current patch looks with the default settings on a Mac, in either Safari or Firefox. (B) has the icons shifted up so they're baseline-aligned with the text. (C) is also baseline-aligned, but by moving the text down instead of the icon up. This option adds 3px more space at the top of the table cell, which has to be also added at the bottom to keep the table tidy-looking. I think (A) is easily the best-looking option here.

In large-text-alignment.png "Before" shows the current patch, in Firefox with a larger-than-default font size. "After" shows the icon centered on the text, rather than baseline-aligned. I propose that this is what we aim for -- icon centered on the text regardless of its size -- instead of aligning on the baseline.

Also, this patch really ought to improve the accessibility for screen-reader users too, not just color-blind or low-vision folks. For blind people, adding the icons as a background image makes the status of the row just as invisible as the colors do. So, the icons should be added using text replacement as discussed in #851496: Invalid to use the CSS background to display non-background images in Forum.

Jeff Burnz’s picture

Good work - I like A and the AFTER screen-shots, that looks great. I'm not sure we need to use text-image-replacement for these icons because there is identifying text in the table cell - the title of the issue - be good to hear from screen-reader users what they think, I mean we can do it but do we need to?

markabur’s picture

Well, if I look at the status box before the patch, and imagine it with no colors, then I have no idea which of the messages means I have to do something and which are safe to ignore.

If we agree that the coloring and the icons have any non-decorative value at all, then we're obligated to extend that value to non-sighted users too, imho.

Jeff Burnz’s picture

Yes, I tend to agree now I've looked at this more and thought about it - if you hear "Security Alert!" you are much more likely to pay attention than if you hear nothing.

mgifford’s picture

@yoroy & @Dries, can we get consensus on the look of this before we wrap up a patch?

Cliff’s picture

@Jeff and @markabur, each icon does need to have an appropriate "alt" attribute. That's basic accessibility.

markabur’s picture

@Cliff, yes, img tag + alt text would be another approach to making these icons more accessible, but I think it makes more sense to use text replacement instead as discussed in #851496: Invalid to use the CSS background to display non-background images in Forum. Basically that the img tag ought to be used for imagery such as photos, illustrations and diagrams, but not graphic elements like icons, buttons, or decoration. As a designer I like the distinction and it seems like a usability/accessibility win too. What do you think?

Jeff Burnz’s picture

No way should we be going back to img elements for icons - they're difficult to theme and offer nothing in terms of better accessibility. The text replacement method is better because the AT user is not trying to infer meaning from an img alt tag - its a strait text label.

Cliff’s picture

Well, I've learned something! Having read @Damien’s and lotyrin’s comments in the other thread, I agree. Text replacement makes better sense.

markabur’s picture

Great, it's good to get your stamp of approval on this method. :-) I can go ahead and put together a patch that outputs a structure similar to #821672: Forum icons not accessible to screen-reader users, which will be easy to keep aligned with the text.

markabur’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

Here's the patch. The icons are 16x16 divs and everything in the table is vertical-align: middle, so the icons are aligning like A and After in #32. For the text equivalents I went with the class names: Info, OK, Warning, and Error. These are translatable. RTL should be good though I haven't tested it. I also caught a couple more th rules that could be deleted (in the merge-up and merge-down sections).

Those table-row-building lines are getting pretty long so maybe we should break those up?

mgifford’s picture

FileSize
160.03 KB
156.56 KB

I think that the RTL needs a bit more padding on the right hand side.

Here's are some screenshots. It's looking good though other than that. Thanks for bringing over the approach from #821672: Forum icons not accessible to screen-reader users

Jeff Burnz’s picture

Status: Needs review » Needs work
+++ modules/system/system.admin.inc
@@ -2581,15 +2581,24 @@ function theme_status_report($variables) {
+      $icon_class = $classes[isset($requirement['severity']) ? (int) $requirement['severity'] : 0];
+      $icon_title = $titles[isset($requirement['severity']) ? (int) $requirement['severity'] : 0];
+      $icon = "<div class='status-icon-$icon_class' title='$icon_title'><span class='element-invisible'>$icon_title</span></div>";

I'm not really liking how this all revolves around an "icon" namespace - the icon is just a visual rendering of the severity level, to me the naming convention here is wrong. Also why do we set basically the same thing into two different variables? Can't we reduce this all to one array/one variable and use drupal_html_class()?

Powered by Dreditor.

markabur’s picture

Thanks guys. @Jeff, I assume you're talking about $icon in the variables? Partly it's to differentiate $icon_class from the $row_class (formerly known as $class).

But really a separate $row_class isn't needed in the first place -- it's only there to accommodate even/odd on the rows, which doesn't make any sense on this table. It's loaded with information as it is and I don't see anyone wanting to zebra-stripe it. So, on the next roll I'd like to rip out the even/odd classes.

Then we can go back to $class and $title, or we could use $severity_class and $severity_title for a bit more readability.

Note that $icon_title is translatable, since that's text, but $icon_class is always going to point to english-named icons. So, we can't base the class name on the titles using drupal_html_class().

It would be possible to omit the severity class from the div if we don't mind being a little bit different from #821672: Forum icons not accessible to screen-reader users. What do you think?

Drat, I should have tested the RTL omm... I see the problem.

Jeff Burnz’s picture

Good explanation - I hadn't thought about the translation stuff.

What are you thinking with regards to a little bit different?

Agree with the zebra striping stuff, we can get rid of that surely.

markabur’s picture

Just that in #821672 the class that corresponds to the title is applied to the div, so it's all one self-explanatory unit. Here, we could omit that class from the div since we have to use it on the tr anyway (for the bg color). So, we could be more efficient at the cost of deviating slightly from the output in #821672.

Jeff Burnz’s picture

I think we can deviate here, no point in setting the class twice if we don't need to - that patch in #821672: Forum icons not accessible to screen-reader users is not in yet so its not set in stone either (I hope it does get in though).

So now we just need to fix RTL a bit, remove the row classes, tidy up the CSS and we're there. Sounds good.

markabur’s picture

Here we go...

RTL has right-padding added.

Unnecessary zebra-striping code removed.

Class removed from the icon div and css updated to hook onto the enclosing tr's class instead.

For this round I've switched to using a $severities array to hold each pair of severity title and severity class. It makes more sense to group each severity than to keep all the titles together in one place and all the classes in another. Also, this meshes nicely with the $requirement['severity'] variable that's used as a key. And that array only needs to be set up once, so I've moved it outside the for loop.

markabur’s picture

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
132.72 KB

Looks great @markabur - I think we're good to go. I've attached a screenshot of the new RTL view.

yoroy’s picture

Looking at the screenshot some rows seem to be missing the grey border (between core update status and file system, between gd library PNG and gd library rotate). Just checking, is this introduced by the patch?

markabur’s picture

@yoroy no, @mgifford's last screenshot looks to have some sort of scaling (the icons are smaller than 16px), which would account for the missing 1px borders. OMM the horizontal borders are all there except between merge-down and merge-up rows. The only border-related CSS that changed was related to th cells.

So... are we still wanting to get this in without the "OK" checkmarks? I understand the "no news" == "good news" idea, but I also feel like "lots of good news" == "better news". In other words having a report full of checkmarks feels better than one that merely lacks triangles and octagons... basically I agree with @wretched sinner and @dww here: http://drupal.org/node/665790#comment-2401296

mgifford’s picture

Ya, the sizing might be off in my screenshot as I've been messing with text sizing.. Sorry if that throws people off.. Might be useful to have another screenshot..

markabur’s picture

@mgifford, the variation turned out to be good for the project, now the css is more robust. So, no prob, and thanks. :-)

My screenshot has samples of default-settings output in Safari, RTL in Safari, and larger-text in Safari (Firefox is nearly identical):

http://drupal.org/files/issues/status-report-icons_906738-9.png

Jeff Burnz’s picture

Issue tags: +markup

Looks good to me.

yoroy’s picture

Ok good, I figured it was an issue with the screengrab, not the actual patch but had to check :)
Good to go

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I do share Dries's concerns in #26. The icons do seem to clash a bit with the current Bartik/Seven styling and look a little awkward and out of place (likely because they were designed on their own, far before we had either Seven or Bartik iirc).

This patch, however, is pretty straight-forward, and just restores consistency so that we're not using two different styles for system messages on the front-end and the back-end. So seems fine to me.

Committed to HEAD. (Obviously, Dries, if you feel strongly, we can roll this back.)

I'm loathe to open up a bikeshed argument about icons again at this stage, but maybe we should re-open the original icon discussion (or create a new issue) and see what alternatives would look like now that we have the message colours and look mostly worked out.

Status: Fixed » Closed (fixed)
Issue tags: -Accessibility, -markup

Automatically closed -- issue fixed for 2 weeks with no activity.