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

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

montesq’s picture

These 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

montesq’s picture

double post due to server issue

bleen’s picture

Issue tags: +CSS, +Quick fix, +Novice

adding tags

bleen’s picture

Title: Missing forum icons in misc/ » Fix image references in forum.css
montesq’s picture

FileSize
73.97 KB
64.61 KB

These 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?

bleen’s picture

Webchick/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.

geerlingguy’s picture

Subscribe...

droplet’s picture

the images combined into css sprite. either we add two new image or using css3..

montesq’s picture

Yes 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

Jeff Burnz’s picture

FileSize
962 bytes
2.75 KB

Well, 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.

Tor Arne Thune’s picture

Status: Active » Needs review
Jeff Burnz’s picture

Status: Needs review » Needs work

Certainly 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.

montesq’s picture

@#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...

Jeff Burnz’s picture

@13, I thought this also, however if you look in the template forum-list.tpl.php this is the code:

<td <?php print $forum->is_container ? 'colspan="4" class="container"' : 'class="forum"'; ?>>

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?

geerlingguy’s picture

@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.

geerlingguy’s picture

I 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.

anavarre’s picture

Subscribe

jcarlson34’s picture

subscribing

kehan’s picture

subscribing

mgifford’s picture

Ok, 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.

Starminder’s picture

subscribe

droplet’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: -Quick fix
Jeff Burnz’s picture

Status: Needs work » Needs review
Issue tags: -CSS

#10: forum-image.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +CSS

The last submitted patch, forum-image.patch, failed testing.

Jeff Burnz’s picture

The 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...

+++ modules/forum/forum-rtl.css	3 Jan 2011 08:18:20 -0000
@@ -1,9 +1,8 @@
+  float: right; ¶

whitespace....

Powered by Dreditor.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
2.07 KB

Rerolled, I think it was just choking on the removed $Id$ tag.
Fixed the whitespace.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies nicely and looks fine in D8.

droplet’s picture

Status: Reviewed & tested by the community » Needs work

where's forum-new.png ??

sun’s picture

Priority: Major » Normal

I 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.

klonos’s picture

Getting this in latest 7.x-dev. ...subscribing.

Jeff Burnz’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Reviewed & tested by the community

This 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..").

droplet’s picture

#forum tr.new-topics td.forum {
background-image: url(../../misc/forum-new.png

coding still there, how come it fixed ?

Jeff Burnz’s picture

The 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.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

No, 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).

webchick’s picture

Priority: Normal » Major

And I would call Drupal core modules spewing 404 entries in the logs about their own files a "major" bug.

sun’s picture

Status: Needs work » Needs review
FileSize
3.35 KB

I 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.

sun’s picture

Made the preprocess code for icon_class more in line with the existing (mess of) code.

xjm’s picture

Tagging issues not yet using summary template.

catch’s picture

That patch looks fine but we probably need either screenshots, or someone to manually review it.

TransitionKojo’s picture

Status: Needs review » Needs work
FileSize
117.78 KB

Ok, 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. :-(

droplet’s picture

Status: Needs work » Needs review

until you doing test with it, still keep review for other guys.

TransitionKojo’s picture

I'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

  1. 404 error report after patch but BEFORE cache cleared
  2. Forum reloaded after cache clear; note the default icon, which hadn't been showing before.
  3. Forum with new topic added; note default icons
  4. Same forum view, different user; Original poster was "d8coh", new user is "kojo"; note "New" forum icons
  5. 404 error report after the above screenshots/changes; still 14 errors==>no new errors

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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Added summary. Do we need to add a title tag and invisible title span to match #821672: Forum icons not accessible to screen-reader users?

mgifford’s picture

@xjm It should be changed so that there is a description perhaps in forum-list.tpl.php. Perhaps:

+          <div class="icon forum-status-<?php print $forum->icon_class; ?>">
+          <span class="element-invisible"><?php print ($forum->icon_class == 'new') ? t('New post') : ''; ?><span>
+          </div>

Similar open issue:
#1060700: Text in new topic and new post links does not describe its purpose

xjm’s picture

#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:

  $variables['icon_class'] = $variables['new_posts'] ? 'new' : 'default';
  $variables['icon_title'] = $variables['new_posts'] ? t('New comments') : t('Normal topic');

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.

mgifford’s picture

Agreed. 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

klonos’s picture

...The normal icon doesn't really convey any information, so there's no need to add a text alternative.

...besides of course the case of screen readers. So perhaps it could be "No new comments" (...or something) instead of blank (??).

bleen’s picture

klonos: 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

klonos’s picture

Fair enough, point taken.

mgifford’s picture

I 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.

xjm’s picture

Assigned: TransitionKojo » xjm
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs accessibility review

I'll re-roll sun's patch with the pattern in #45 so folks can test it.

xjm’s picture

Everett Zufelt’s picture

Just 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?

xjm’s picture

#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') and t('No new posts')?

xjm’s picture

@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.

webchick’s picture

I 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.

xjm’s picture

Cool. I'll reroll with the new strings.

xjm’s picture

Edit. Gah. Bad file extension on the interdiff so it's gonna come back fail on that one. Testbot needs brakes a bridle. :)

Status: Needs review » Needs work

The last submitted patch, interdiff-37-58.patch, failed testing.

Tor Arne Thune’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Reviewed & tested by the community

#58 looks good moving back to RTBC.

catch’s picture

Issue summary: View changes

Clarification

xjm’s picture

Issue tags: +String freeze

Updated summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Jeff Burnz’s picture

Indeed, looks great, like the new strings to convey the information to screen readers +1.

larowlan’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks a lot folks.

Committed and pushed to 8.x and 7.x.

klonos’s picture

Thanx Angie! One less patch to "babysit" after each install ;)

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Issue summary: View changes

Updated issue summary.