Project

General

Profile

Actions

Patch #21256

closed

Use CSS instead of image_tag() to show icons for better theming support

Added by Daniel Ritz over 8 years ago. Updated about 8 years ago.

Status:
Closed
Priority:
Normal
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Replaces all uses of image_tag() to render icons with icon-* definitions. This makes is possible to replace all these icons with Font-based icons, e.g. FontAwesome.

This is split from #21245. Based on r14882, everything passes "rake test" and is tested in Firefox 42, Safari 9 and Chrome 46.


Files


Related issues

Related to Redmine - Patch #21257: Use PDF icon for "Also available in PDF"New

Actions
Related to Redmine - Defect #31510: Fix missing closing tags in workflows/permissions.html.erbClosedGo MAEDA

Actions
Has duplicate Redmine - Patch #21245: UI/CSS refinements, part 1Closed

Actions
Actions #1

Updated by Go MAEDA over 8 years ago

  • Has duplicate Patch #21245: UI/CSS refinements, part 1 added
Actions #2

Updated by Toshi MARUYAMA over 8 years ago

Daniel Ritz wrote:

Replaces all uses of image_tag() to render icons with icon-* definitions. This makes is possible to replace all these icons with Font-based icons, e.g. FontAwesome.

Could you explain more details?

Actions #3

Updated by Daniel Ritz over 8 years ago

Toshi MARUYAMA wrote:

Daniel Ritz wrote:

Replaces all uses of image_tag() to render icons with icon-* definitions. This makes is possible to replace all these icons with Font-based icons, e.g. FontAwesome.

Could you explain more details?

Currently, in most places where an icon is shown, it's done with CSS classes, e.g. setting class="icon icon-edit" to a <span> or <a> or whatever. This makes it very easy in themes to replace those .png based icons with font-based icons like FontAwesome. However not all icons displayed in the GUI use CSS. Some rely on simple <img> tags, mostly added with the helper function image_tag().

This patch changes all these icons to use CSS classes as well instead of the <img> tags. Icons that did not have a icon-<name> CSS class defined already are added (11 I think). Where necessary, test cases are adapted as well.

Also, this introduces the CSS class icon-only". It is like icon, but does not add additional padding on the right side since no text is expected after this. This is mostly cosmetic. I did it so the look of certain pages does not change, e.g. the sort icons (top, up, down, bottom) in Administration -> Issue statuses.

With this patch, almost all icons in Redmine can be replaced with font-based icons. The only exception being the "search" icon in some input fields. They are done with CSS as background, but there's no replacement for it with font-based icons w/o an additional tag to wrap the input field. But that's for another patch :)

Actions #4

Updated by Toshi MARUYAMA over 8 years ago

  • Related to Patch #21257: Use PDF icon for "Also available in PDF" added
Actions #5

Updated by Tobias Fischer about 8 years ago

I agree, this would be very useful for theme developers!

Actions #7

Updated by Toshi MARUYAMA about 8 years ago

  • Target version set to 3.3.0
Actions #8

Updated by Jean-Philippe Lang about 8 years ago

  • Category changed from UI to Code cleanup/refactoring
  • Status changed from New to Closed
  • Assignee set to Jean-Philippe Lang

Patch committed, thanks for this nice work Daniel.

Actions #9

Updated by Gregor Schmidt about 8 years ago

Removing the link content and only relying on the icon, which is added via CSS, severely hampers accessibility. To make life easier for screen readers and their users, I think the link description should be made available in a hidden span, e.g. using a technique described in this document.

Only relying on the title attribute might not be sufficient.

Current user agents and assistive technology provide no feedback to the user when links have title attribute content available.

https://www.w3.org/TR/2013/NOTE-WCAG20-TECHS-20130905/H33.html

Actions #10

Updated by Toshi MARUYAMA about 8 years ago

  • Status changed from Closed to Reopened

Gregor Schmidt wrote:

To make life easier for screen readers and their users, I think the link description should be made available in a hidden span, e.g. using a technique described in this document.

Patch welcome.

Actions #11

Updated by Tobias Fischer about 8 years ago

Gregor Schmidt wrote:

Removing the link content and only relying on the icon, which is added via CSS, severely hampers accessibility. To make life easier for screen readers and their users, I think the link description should be made available in a hidden span, e.g. using a technique described in this document.

Only relying on the title attribute might not be sufficient.

For cases where the icon was missing an adjacent label, I'm completely with you.

However, in most cases, the current icons were "bundled" with a label like this:

The label should then be enough as a description for the CSS icon attached to it, don't you think so?

Actions #12

Updated by Gregor Schmidt about 8 years ago

However, in most cases, the current icons were "bundled" with a label like this:

The label should then be enough as a description for the CSS icon attached to it, don't you think so?

You're right. If the link still contains descriptive text, everything is fine.

But if there's an "empty" link with an icon, next to a text-only link pointing to the same target, then I would say that should be changed to a single link containing the icon and the descriptive text.

Actions #13

Updated by Daniel Ritz about 8 years ago

Gregor Schmidt wrote:

However, in most cases, the current icons were "bundled" with a label like this:

The label should then be enough as a description for the CSS icon attached to it, don't you think so?

You're right. If the link still contains descriptive text, everything is fine.

But if there's an "empty" link with an icon, next to a text-only link pointing to the same target, then I would say that should be changed to a single link containing the icon and the descriptive text.

There should be no such thing as an empty icon link followed by a text-only link with the same target. There are links with icon/text and links with icon only. This patched changed only the latter not to use <img> tags. I'm pretty sure this is no change wrt accessibility. Your suggested hidden span is relatively easy to implement on top of what we have now. It however means checking all icon links. The new icon-only are few and easy to find, but there are others without the "icon-only" class so they need to be checked manually. I can have a look over the weekend if time permits...

Actions #14

Updated by Gregor Schmidt about 8 years ago

Daniel Lopez: That sounds great.

Also sorry for mentioning it not in my first comment: Thanks for the effort and braveness to create such a big patch in the first place. The changes are a great improvement with respect to extensibility and page speed.

Actions #15

Updated by Daniel Ritz about 8 years ago

Gregor Schmidt wrote:

Removing the link content and only relying on the icon, which is added via CSS, severely hampers accessibility. To make life easier for screen readers and their users, I think the link description should be made available in a hidden span, e.g. using a technique described in this document.

Only relying on the title attribute might not be sufficient.

Current user agents and assistive technology provide no feedback to the user when links have title attribute content available.

https://www.w3.org/TR/2013/NOTE-WCAG20-TECHS-20130905/H33.html

Daniel Ritz wrote:

Your suggested hidden span is relatively easy to implement on top of what we have now. It however means checking all icon links. The new icon-only are few and easy to find, but there are others without the "icon-only" class so they need to be checked manually. I can have a look over the weekend if time permits...

I implemented this in #21805. Please have a look. Maybe the discussion should move to the new patch with this one closed?

Actions #16

Updated by Jean-Philippe Lang about 8 years ago

  • Status changed from Reopened to Closed

Daniel Ritz wrote:

I implemented this in #21805. Please have a look. Maybe the discussion should move to the new patch with this one closed?

Agreed.

Actions #17

Updated by Go MAEDA almost 5 years ago

  • Related to Defect #31510: Fix missing closing tags in workflows/permissions.html.erb added
Actions

Also available in: Atom PDF