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.
Comment | File | Size | Author |
---|---|---|---|
#51 | screen-capture-20.png | 132.72 KB | mgifford |
#49 | status-report-icons_906738-9.png | 52.32 KB | markabur |
#49 | status-report-icons_906738-9.patch | 4.85 KB | markabur |
#43 | screen-capture-15.png | 156.56 KB | mgifford |
#43 | screen-capture-16.png | 160.03 KB | mgifford |
Comments
Comment #1
mgiffordmostly 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.
Comment #2
mgiffordPatch 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.
Comment #3
tstoecklerSmall nitpick:
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.
Comment #4
markabur CreditAttribution: markabur commentedIn 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:
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.
Comment #5
markabur CreditAttribution: markabur commentedRelated patch where the th was removed: #783534: Make Report table better
Comment #6
yoroy CreditAttribution: yoroy commentedVisually, 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?
Comment #7
mgifford@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.
Comment #8
bowersox CreditAttribution: bowersox commented+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.
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, 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:
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.
Comment #10
markabur CreditAttribution: markabur commentedHere 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.
Comment #11
Jeff Burnz CreditAttribution: Jeff Burnz commentedLooking 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...
Comment #12
Cliff CreditAttribution: Cliff commentedSubscribing.
Comment #13
markabur CreditAttribution: markabur commentedAgreed 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.
Comment #14
mgiffordI 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:
Comment #15
yoroy CreditAttribution: yoroy commentedMy designer eye does not like that the icon isn't centered in the available space between border and text, could use a tweak imo.
Comment #16
Jeff Burnz CreditAttribution: Jeff Burnz commentedLets nail the RTL and alignment issues first.
Comment #17
mgifford@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.
Comment #18
markabur CreditAttribution: markabur commentedHere'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.
Comment #19
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, 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.
Comment #20
Jeff Burnz CreditAttribution: Jeff Burnz commentedWith RTL, not perfect but about as good as it gets, works OK.
Comment #21
mgiffordI'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?
Comment #22
markabur CreditAttribution: markabur commentedShouldn't the "else" be using status-value for the class, not requirement-value?
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.
Comment #23
yoroy CreditAttribution: yoroy commentedI is happy
Comment #24
Jeff Burnz CreditAttribution: Jeff Burnz commentedClass 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...
Comment #25
markabur CreditAttribution: markabur commentedLooks good.
Comment #26
Dries CreditAttribution: Dries commentedI 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?
Comment #27
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, 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),
Comment #28
mgifford@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.
Comment #29
markabur CreditAttribution: markabur commentedOf 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! :-)
Comment #30
Jeff Burnz CreditAttribution: Jeff Burnz commentedProblem 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.
Comment #31
markabur CreditAttribution: markabur commentedSure, 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.
Comment #32
markabur CreditAttribution: markabur commentedHere 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.
Comment #33
Jeff Burnz CreditAttribution: Jeff Burnz commentedGood 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?
Comment #34
markabur CreditAttribution: markabur commentedWell, 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.
Comment #35
Jeff Burnz CreditAttribution: Jeff Burnz commentedYes, 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.
Comment #36
mgifford@yoroy & @Dries, can we get consensus on the look of this before we wrap up a patch?
Comment #37
Cliff CreditAttribution: Cliff commented@Jeff and @markabur, each icon does need to have an appropriate "alt" attribute. That's basic accessibility.
Comment #38
markabur CreditAttribution: markabur commented@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?
Comment #39
Jeff Burnz CreditAttribution: Jeff Burnz commentedNo 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.
Comment #40
Cliff CreditAttribution: Cliff commentedWell, I've learned something! Having read @Damien’s and lotyrin’s comments in the other thread, I agree. Text replacement makes better sense.
Comment #41
markabur CreditAttribution: markabur commentedGreat, 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.
Comment #42
markabur CreditAttribution: markabur commentedHere'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?
Comment #43
mgiffordI 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
Comment #44
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'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.
Comment #45
markabur CreditAttribution: markabur commentedThanks 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.
Comment #46
Jeff Burnz CreditAttribution: Jeff Burnz commentedGood 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.
Comment #47
markabur CreditAttribution: markabur commentedJust 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.
Comment #48
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #49
markabur CreditAttribution: markabur commentedHere 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.
Comment #50
markabur CreditAttribution: markabur commentedComment #51
mgiffordLooks great @markabur - I think we're good to go. I've attached a screenshot of the new RTL view.
Comment #52
yoroy CreditAttribution: yoroy commentedLooking 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?
Comment #53
markabur CreditAttribution: markabur commented@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
Comment #54
mgiffordYa, 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..
Comment #55
markabur CreditAttribution: markabur commented@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
Comment #56
Jeff Burnz CreditAttribution: Jeff Burnz commentedLooks good to me.
Comment #57
yoroy CreditAttribution: yoroy commentedOk good, I figured it was an issue with the screengrab, not the actual patch but had to check :)
Good to go
Comment #58
webchickI 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.