Problem/Motivation
The files forum-default.png
and forum-new.png
have been removed from core and replaced with the sprite forum-icons.png
(see #821672: Forum icons not accessible to screen-reader users). However, modules/forum/forum.css
was not properly updated to use the new file on the forum list.
Proposed resolution
- Modify CSS to use the new file
- Set an appropriate class in the theme preprocess
- Add screen reader text alternatives consistent with #821672: Forum icons not accessible to screen-reader users
Patch in #58 implements these changes.
Remaining tasks
Patch in #58 is RTBC.
User interface changes
String addition. Two new strings are added:
t('New posts')
t('No new posts')
Additionally, the missing icons on forum list will reappear, but they use the pre-existing interface pattern. Compare http://drupal.org/files/issues/forum-list.png and http://drupal.org/files/issues/Screen%20shot%202011-08-13%20at%2011.46.3... for an illustration of the change.
API changes
None.
Original report by @tsvenson
forum-default.png and forum-new.png are missing in the /misc folder. They are both defined in /modules/forum/forum.css.
Comment | File | Size | Author |
---|---|---|---|
#58 | forum-list-icons-alt-text-1008580-58.patch | 3.96 KB | xjm |
#58 | interdiff-37-58.patch | 2.39 KB | xjm |
#52 | forum-list-icons-alt-text-1008580-52.patch | 3.95 KB | xjm |
#52 | interdiff.txt | 2.38 KB | xjm |
#42 | Screen shot 2011-08-13 at 11.42.11 AM.png | 161.58 KB | TransitionKojo |
Comments
Comment #1
montesq CreditAttribution: montesq commentedThese icons have been deleted following the issue #821672: Forum icons not accessible to screen-reader users
The forum.css file must be adapted to use forum-icons.png instead
Comment #2
montesq CreditAttribution: montesq commenteddouble post due to server issue
Comment #3
bleen CreditAttribution: bleen commentedadding tags
Comment #4
bleen CreditAttribution: bleen commentedComment #5
montesq CreditAttribution: montesq commentedThese 2 images are used in the forum list page (forum-list.png).
However to be coherent with the topic list page (topic-list.png), I suggest adding a new column (without header) that will display only the new/default icon (or alt label). Can you confirm it's the right way to fix it?
Comment #6
bleen CreditAttribution: bleen commentedWebchick/Dries can chime in here, but I think it's too late to make a front-end change like that (even though it is fairly minor). I would simply adjust forum.css to use the correct .png files.
You make a good point about being consisten though, so for that I would open a D8 issue.
Comment #7
geerlingguy CreditAttribution: geerlingguy commentedSubscribe...
Comment #8
droplet CreditAttribution: droplet commentedthe images combined into css sprite. either we add two new image or using css3..
Comment #9
montesq CreditAttribution: montesq commentedYes indeed, in this case the less intrusive solution is for me to restore forum-default.png and forum-new.png on the short run...
And we fix it neatly in D8 by doing the same thing as for the topic list
Comment #10
Jeff Burnz CreditAttribution: Jeff Burnz commentedWell, we could do something like this patch, or just add back the removed images.
From what i can see the reference to forum-new.png is legacy code and now redundant, since the sprite covers that one.
My preference would be to fix this properly and take advantage of the sprite as was originally intended, this is what the patch does. We need this to work with all browsers so any CSS3 trickery probably won't go down well here, in any case I can't really think of a decent CSS3 implementation that would work well, instead I opted to add markup.
The patch is pretty rough, such as the margins, coding standards etc - its a proof of concept.
I've attached the old forum-default.png, simply dropping this into /misc solves the issue also - I'd be perfectly happy with this also although we add one HTTP request which is what we're trying to avoid.
Comment #11
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #12
Jeff Burnz CreditAttribution: Jeff Burnz commentedCertainly it needs work as there is a whitespace in there and I was a bit loose and free with the margin/padding and while it looks fine in Bartik maybe the margins are not great defaults.
Will this fly or not, that is the question. We could do a patch like the suggestion in #5, but I'm not so sure that it's the same same as topics - the topics icons are text-image replaced and actually really mean something, whereas this forum-default icon is pure decoration.
Comment #13
montesq CreditAttribution: montesq commented@#12
My understanding is there are two icons forum-default.png (that is meanly decoration) *and* forum-new.png that shows there exist at least one new topic in the forum...
Comment #14
Jeff Burnz CreditAttribution: Jeff Burnz commented@13, I thought this also, however if you look in the template forum-list.tpl.php this is the code:
The classes are not really dynamic (no classes array here), thats just hard coded to ".forum".
See the screenshot, there are 6 new topics but the class is just .forum, is this what you are talking about or something different?
Comment #15
geerlingguy CreditAttribution: geerlingguy commented@13 - There aren't any dynamic classes for indicating whether a forum row has 'new' posts or not, so we really only need to display the first icon in forum-icons.png. The only reasonable way I think we can do this is using Jeff Burnz' approach in #10. However, I think the icon should be on the left of both the forum title and description, as it was in D6...
I'll see if I can get a more thorough patch working.
Comment #16
geerlingguy CreditAttribution: geerlingguy commentedI was working on an alternative approach that used another td to display the icon (
<td class="icon"></td>
, but abandoned that idea pretty quickly, since doing so would require even more RTL-language support work, and it still added HTML cruft (and made fixing the forum indentation a pain).I'm thinking we either stick with putting the graphic back in (forum-default.png)—which is by far the easiest/least annoying route, and also likely the least code-and-markup-heavy, or use Jeff Burnz's approach in #10 and add the graphic floated to the left (right in RTL). It would still be nice to have it on the left of both the forum title and description (see, for instance, drupal.org's forums).
See attachments for screenshots of before/after of the patch in #10.
Comment #17
anavarreSubscribe
Comment #18
jcarlson34 CreditAttribution: jcarlson34 commentedsubscribing
Comment #19
kehan CreditAttribution: kehan commentedsubscribing
Comment #20
mgiffordOk, I think we're all agreed that /misc/forum-default.png should be added in the very, very next maintenance release. 404's in core are just bad.
Here's what it looks like with Jeff's patch in #10 http://a11yyow.ca/en/forum
It may not be ideal, but it's way better than a 404 so let's see what we can to to get this RTBC so this can be fixed quick.
Comment #21
Starminder CreditAttribution: Starminder commentedsubscribe
Comment #22
droplet CreditAttribution: droplet commentedComment #23
Jeff Burnz CreditAttribution: Jeff Burnz commented#10: forum-image.patch queued for re-testing.
Comment #25
Jeff Burnz CreditAttribution: Jeff Burnz commentedThe patch needs a reroll, I didn't look why it failed, I'll try to get this later today if someone else doesn't beat me to it...
whitespace....
Powered by Dreditor.
Comment #26
tim.plunkettRerolled, I think it was just choking on the removed $Id$ tag.
Fixed the whitespace.
Comment #27
mgiffordPatch applies nicely and looks fine in D8.
Comment #28
droplet CreditAttribution: droplet commentedwhere's forum-new.png ??
Comment #29
sunI think it's too late to do the major markup changes for D7. Also, when using icon placeholders like this, they should be part of the anchor, since users tend to click on icons instead of the text.
I'd recommend to do a quick fix for D8 + D7 first, and only afterwards, possibly do a larger change for D8.
Lastly, this can be easily fixed from CSS. Nothing major here.
Comment #30
klonosGetting this in latest 7.x-dev. ...subscribing.
Comment #31
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis is the fix:
Drupal 7 - put the forum-default.png (http://drupal.org/files/issues/forum-default_6.png) in the misc folder.
Lets RTBC this for 7 now. Please can we not bikeshed this for another minute - the fix for Drupal 7 is so simple.
Drupal 8 - the patch from http://drupal.org/node/1008580#comment-4500790 (may need some work, I still see a reference to forum-new.png which I think needs to be removed). This patch merely makes it consistent with how all other forum icons are added.
The icon on the anchor is a new UI pattern (albeit a small one), however forum icons have never worked like this, and none of the others do either, afaik this has never been reported in any usability test ("user tried to click forum icon etc..").
Comment #32
droplet CreditAttribution: droplet commented#forum tr.new-topics td.forum {
background-image: url(../../misc/forum-new.png
coding still there, how come it fixed ?
Comment #33
Jeff Burnz CreditAttribution: Jeff Burnz commentedThe CSS is not the problem, the image is missing from Drupal. Please see the OP - that is the bug.
The fix, and what is RTBC, is simply adding the image to Drupal.
Comment #34
webchickNo, we have forum-icons.png in core, we should just use it. Whatever's making it work on the individual forum listing needs to be done on the main forum listing.
I'm not comfortable committing "band-aid" fixes to stable releases of core, with the possible exception of the most vile of critical bugs. This definitely doesn't qualify (though it is pretty embarrassing).
Comment #35
webchickAnd I would call Drupal core modules spewing 404 entries in the logs about their own files a "major" bug.
Comment #36
sunI have absolutely no idea why I started to look into this, but here's a proper stop-gap fix for D8 and D7.
Note, however, that this entire forums/container/topics/icon theming logic is so horribly broken that I'd actually recommend to
git rm -r modules/forum
for D8. Well, separate issue.Comment #37
sunMade the preprocess code for icon_class more in line with the existing (mess of) code.
Comment #38
xjmTagging issues not yet using summary template.
Comment #39
catchThat patch looks fine but we probably need either screenshots, or someone to manually review it.
Comment #40
TransitionKojo CreditAttribution: TransitionKojo commentedOk, I made a rookie mistake and wasn't looking in the log files for the 404 errors. I'll apply the patch from #37 tomorrow and see if that fixes things.
Sorry for the mistake. :-(
Comment #41
droplet CreditAttribution: droplet commenteduntil you doing test with it, still keep review for other guys.
Comment #42
TransitionKojo CreditAttribution: TransitionKojo commentedI've tested @sun's #37 patch in 7.x and 8.x, but I don't know how to do what @webchick did in #34 to show status for two branches. But the problem existed in 7.x and 8.x. @sun's patch from #37 fixes the issue in 7.x and 8.x
Here are the screenshots for 8.x (since that seems to be the focus); please note the timestamp file names, as the indicate the sequence of events
So, Patch #37 fixes this issue in 8.x.
I hope this is clear and sufficient. If not, let me know what else is needed. I've gotten a lot of help from folks in #drupal-contribute, but this is my first time doing this.
My write up and screenshots for the 7.x testing are at http://www.wuxing.me/?q=node/9. If needed, I can put that stuff here too. Patch #37 fixes the issue in 7.x as well.
Comment #42.0
xjmUpdated issue summary.
Comment #42.1
xjmUpdated issue summary.
Comment #43
xjmAdded summary. Do we need to add a title tag and invisible title span to match #821672: Forum icons not accessible to screen-reader users?
Comment #44
mgifford@xjm It should be changed so that there is a description perhaps in forum-list.tpl.php. Perhaps:
Similar open issue:
#1060700: Text in new topic and new post links does not describe its purpose
Comment #45
xjm#44 would introduce a new string, so we should try to reuse what was added in #821672-41: Forum icons not accessible to screen-reader users if possible. It has:
Edit: "Normal topic" does not make sense here, but we could use "New comments" and a blank string, as suggested above. The normal icon doesn't really convey any information, so there's no need to add a text alternative.
Comment #46
mgiffordAgreed. Thanks @xjm. Do you want to modify the patch and add it to forum-list.tpl.php?
EDIT: This would go in the forum.module as per the example here http://drupal.org/node/821672#comment-3301374
Comment #47
klonos...besides of course the case of screen readers. So perhaps it could be "No new comments" (...or something) instead of blank (??).
Comment #48
bleen CreditAttribution: bleen commentedklonos: if an image does not convey any information to a sighted user than there is no expectation that a text alternative is needed for a screen reader. If we were to include alt text then we would be adding a string (requiring a new translation into 12 bazillion languages) and this is akin to an API change with respect to back-porting this to D7
Comment #49
klonosFair enough, point taken.
Comment #50
mgiffordI figure (as a sighted person) that if I'm not certain (which I'm not) that I should ask (which I will). I'll take a straw poll & see what I can find.
EDIT: Talked to Everett & his view is that if . If an image conveys info it needs alternate text. This goes for the CSS background images too. I personally like to remove redundant info, but as I don't use screen readers it's not my call.
Comment #51
xjmI'll re-roll sun's patch with the pattern in #45 so folks can test it.
Comment #52
xjmComment #53
Everett Zufelt CreditAttribution: Everett Zufelt commentedJust to clarify, if the image conveys info it needs an alt string (likely cannot be backported). However, I didn't read the thread and know nothing about the icon in question. My question would be why is this icon there in the first place if it conveys no information?
Comment #54
xjm#50: To me the default icon isn't actually conveying information in this case, but I definitely defer to someone who has more direct experience with AT. Maybe with the patch in #52 we could do some user testing and see if the UI is correct in a screen reader.
Maybe we could use the version in #52 for backport, and change the strings in D8 to
t('New posts')
andt('No new posts')
?Comment #55
xjm@Everett Zufelt: In this case, there is one icon that indicates there are new posts, for which we can reuse an existing alt text string. The icon we're not sure about is the icon used in its place when there are not new posts.
Comment #56
webchickI think this deserves an alt attribute: whether or not a forum contains new posts is an important factor in information scanning. And since this is fixing a bug, I think we can break strings for this.
Comment #57
xjmCool. I'll reroll with the new strings.
Comment #58
xjmEdit. Gah. Bad file extension on the interdiff so it's gonna come back fail on that one. Testbot needs
brakesa bridle. :)Comment #60
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #61
catch#58 looks good moving back to RTBC.
Comment #61.0
catchClarification
Comment #62
xjmUpdated summary.
Comment #62.0
xjmUpdated issue summary.
Comment #62.1
xjmUpdated issue summary.
Comment #63
Jeff Burnz CreditAttribution: Jeff Burnz commentedIndeed, looks great, like the new strings to convey the information to screen readers +1.
Comment #64
larowlan#58: forum-list-icons-alt-text-1008580-58.patch queued for re-testing.
Comment #65
webchickGreat, thanks a lot folks.
Committed and pushed to 8.x and 7.x.
Comment #66
klonosThanx Angie! One less patch to "babysit" after each install ;)
Comment #68
xjmComment #68.0
xjmUpdated issue summary.