Patch #21256

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

Added by Daniel Ritz about 2 years ago. Updated almost 2 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Code cleanup/refactoring
Target version:3.3.0

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.

0001-Replace-uses-of-image_tag-with-CSS.patch Magnifier (44 KB) Daniel Ritz, 2015-11-17 15:29

0001-Replace-uses-of-image_tag-with-CSS.patch Magnifier - Rebased agains r15041 (44 KB) Daniel Ritz, 2016-01-09 18:15

21256-label-example.png (3.84 KB) Tobias Fischer, 2016-01-21 12:28


Related issues

Related to Redmine - Patch #21257: Use PDF icon for "Also available in PDF" New
Duplicated by Redmine - Patch #21245: UI/CSS refinements, part 1 Closed

Associated revisions

Revision 15068
Added by Jean-Philippe Lang almost 2 years ago

Replace uses of image_tag() with CSS (#21256).

  • Replace image_tag() with CSS classes
  • Create some CSS classes where necessary
  • Change "check all" positioned on the right in "Trackers -> Summary" and
    "Roles -> Permissions report" instead of left like in "Worfklow"
  • Add missing title attribute in a few places
  • Makes some lines shorter while at it

Patch by Daniel Ritz.

History

#1 Updated by Go MAEDA about 2 years ago

  • Duplicated by Patch #21245: UI/CSS refinements, part 1 added

#2 Updated by Toshi MARUYAMA almost 2 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?

#3 Updated by Daniel Ritz almost 2 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 :)

#4 Updated by Toshi MARUYAMA almost 2 years ago

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

#5 Updated by Tobias Fischer almost 2 years ago

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

#7 Updated by Toshi MARUYAMA almost 2 years ago

  • Target version set to 3.3.0

#8 Updated by Jean-Philippe Lang almost 2 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.

#9 Updated by Gregor Schmidt almost 2 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

#10 Updated by Toshi MARUYAMA almost 2 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.

#11 Updated by Tobias Fischer almost 2 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?

#12 Updated by Gregor Schmidt almost 2 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.

#13 Updated by Daniel Ritz almost 2 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...

#14 Updated by Gregor Schmidt almost 2 years ago

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

#15 Updated by Daniel Ritz almost 2 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?

#16 Updated by Jean-Philippe Lang almost 2 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.

Also available in: Atom PDF