Feed icon should be a CSS background image not a image file

Weren't you planning on removing the template?

Notes:

1. +++ b/core/modules/system/css/system.theme.css
   @@ -162,6 +162,17 @@ abbr.ajax-changed {
   +.feed-icon{
   

   needs a space after.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -162,6 +162,17 @@ abbr.ajax-changed {
   +  background: url(../../../misc/feed.png) no-repeat;
   

   SVG

3. +++ b/core/modules/system/templates/feed-icon.html.twig
   @@ -15,4 +14,6 @@
   +  <span class="visually-hidden">{% trans %} Subscribe to {{ title }} {% endtrans %}</span>
   

   Using a |t filter may be more appropriate here. But space inside should not be there either.

This is a good one. Thanks @mortendk!!

Feed Icon BEFORE

Feed Icon AFTER

+++ b/core/modules/aggregator/aggregator.module
@@ -90,12 +90,6 @@ function aggregator_entity_extra_field_info() {
-      // @todo Move to a formatter at https://www.drupal.org/node/2339917.
-      'image' => array(
-        'label' => t('Image'),
-        'description' => t('The feed image'),
-        'weight' => 2,
-      ),

@@ -107,11 +101,6 @@ function aggregator_entity_extra_field_info() {
-      'feed_icon' => array(
-        'label' => t('Feed icon'),
-        'description' => t('An icon that links to the feed url'),
-        'weight' => 6,
-      ),

why are those removed?

Views uses '#theme' => 'feed_icon', so, it should be fine.
The feed_icon declaration in aggregator_entity_extra_field_info() allows people to show or hide the feed icon from the Manage display options. I still dont understand why its removed

no more objections :) thanks!

I object to the no more objections:P

Let's not leave things in preprocess that are not being used in the template.

Also the |t filter can take the same string as was used before which keeps the string translation context in tact.

I think you can RTBC, basically it was RTBC after yours and you approved the interdiff so RTBC away;)

The title is no longer inside a title attribute and the @ escape works better for the escaping.

There is a slight visual regression, check it:

Before:

After:

@mortendk I'll leave that fun for you.

I do agree with the margin, although margin-top/bottom collapse at the top when the container don't have layout. But what is giving that space to start? Why is there a regression in #26?

Hi Guys,

This looks fine to me. Patch applies fine, feed-icon is working on default and on views. Image is added via CSS.

Shall we move it back to RTBC?

Image for views after patch applied:

Changing status to RTBC

Nice clean-up :)

I realize this is RTBC, so I did think twice about posting this, but I think its worth mentioning.

In #2427213: Replace feed.png with feed.svg we're replacing the feed.png with feed.svg, which is awesome, but now seeing this issue, I wonder if it would be nicer to put the SVG inline inside the feed-icon.html.twig file itself.

Inline SVG files have a couple really nice benefits:

1) Reduces HTTP requests.
2) One less file in drupal/core/misc folder.
3) An inline svg file in the template means technically we could keep around the png file as a fallback image using SVG <switch> and foreignObject tags. (Yes this and item 2 above are mutually exclusive ;) But this really sets a precedent in core on one of the better ways to support SVGs with fallbacks that do not depend on javascript -- this would be HUGE IMHO.
4) You can target the SVG tags in your theme's CSS to change the color fill and possibly other attributes (stroke anyone :P) without actually having to override the SVG file to create your own.

This would give themers two options for theming the feed icon: 1) you can replace the svg with an image background in your theme's css OR 2) you can implement feed-icon.html.twig in your theme and paste in a new SVG.

Would anyone else like to see this issue and #2427213: Replace feed.png with feed.svg merged into one single inline svg solution?

Patch for #36.

The classy theme's version of feed-icon.html.twig uses classes instead of hardcoded colors, and relies on the bartik theme to set the feed-icon's background and element colors in CSS.

Ohh! Fascinating :) This would be the first inline SVG in a Twig template in Drupal core!

Curious to hear what front-end folks think about this!

A benefit not listed in #36: it doesn't look like crap on HiDPI devices :)

rejected! haha, thats ok. Thanks for feedback morten -- was worth a shot. I guess before proposing such novel and drastic ideas we need more discussion and consensus. But i stand by my sentiment that inline svgs is a theming pattern is going to be adopted because it is so darn clever -- and clean.

Yep, agreed on scope.

@jwilson3: basically, we try to avoid expanding the scope of issues to ensure that whatever is agreed upon, can actually get done. Otherwise, too many issues (and improvements!) will just get stuck on discussions.

I'm happy to keep the approach we have taken on this and then when complete and committed and we have some spare time, we look at the (admittedly better) approach of the inline SVG.

Yep, let's keep the conversation there.. #2433761: [meta] svg guidelines / requirements it's getting quite interesting.

But I agree that it's too early to decide about inlining SVG in the templates, until we see all point of views.

Ok, so this is RTBC and is blocking another smaller issue #2427213: Replace feed.png with feed.svg which is also RTBC, but was postponed on this first going in because this one is more complex.

Looks good! Great job mortendk!

Feed Icon BEFORE

Feed Icon AFTER

Are we certain that making this a background image is actually correct? Not that these are our standards but this makes sense to me:

Informational background images must have a visible alternative

From http://www.bbc.co.uk/guidelines/futuremedia/accessibility/mobile/images/...

The span is non visual and the point is that without out the <img> there is no visual element.

Ah that's true, and you can argue that the current implementation provides a visual cue using the title attribute on the <a> element. If we add that attribute back we aren't causing a regression. The img tag does not provide a visual alternative.

See this screenshot of the current implementation:

I had a quick chat with Alex on IRC and we came up with an a solution that solves this problem but requires a small design change. The BBC's design implementation is to always include informative text alongside an icon, see: https://www.dropbox.com/s/egfawpq08s1hamm/Screenshot%202015-03-14%2012.4...

+++ b/core/themes/classy/templates/misc/feed-icon.html.twig
@@ -5,14 +5,13 @@
+  <span class="visually-hidden">{{ 'Subscribe to @title'|t({'@title': title}) }}</span>

Remove the visually-hidden class, and the span.

Adding a visual label is beyond on the scope of this issue IMO, since this is merely about changing to a background image, i.e. we never had a label before.

I would put this back to RTBC (I can't test it right at this moment) and add a new issue for a label in a follow up, thats a pretty big visual change and deserves its own bike shed issue.

@Jeff Burnz We can't as this issue introduces the accessibility regression

@LewisNyman - ok, so you mean when CSS is off the background image won't show? Something along those lines? I solved this by doing an SVG image inline in the template with a hidden span for a non-visual users. This is quite good technique because it survives the "no images/no CSS" test, pretty similar to the patch in this issue already in #37, I don't know why this has been overlooked, it's brilliant and works very well.

I think just pasting inline SVG into the template is a real maintenance problem can we not include the contents of the SVG file using Twig?

Well, we need to frame the conversation by detailing what the accessibility issue is.

Maybe some generalised way of doing inline SVG via twig is an attractive idea, or are we overcomplicating things here?

I'm not sure about the maintenance issue you raise, I can't see this myself, I like the sheer simplicity of it. Caching may or may not be an issue, the code is tiny in this regard.

The rule from the BBC guidelines is:

Informational background images must have a visible alternative

Background images are not perceivable to screen reader users and not supported on phones with basic support for CSS.

So it's not about CSS not loading or the image not loading, and it's not about screen readers. If the mobile browser doesn't support background images or a user chooses not to load images (all mobile browsers have this setting). Then there will be nothing to see. The .visually-hidden class would still hide the text in this case. When images are disabled the alt text for img tags are shown.

I've worked with inlined images before that were manually pasted into code, and it was a big pain to update those images and maintain consistency across the codebase. I'd rather we embedded the external files somehow.

removed span and class.

If I'm reading this right, the bbc seems to solve this problem by putting extra text off screen wrapped in a span, which is only visible then on mobile

http://www.bbc.co.uk/guidelines/futuremedia/accessibility/mobile/links/d...

So, it seems like something like a "mobile-only" class instead of the "visually-hidden" class, is their recommended solution.

I think I read that wrong... the note about "NOTE: <span> is only supported on links in mobile" confused me.

Yeah, but that's not true is it? We use spans in links all the time and it's valid HTML. I don't know how we would pick a media query to target only mobile, do tablets count here? Seems tricky.

I'm honestly not sure what to make of it. I also don't understand the phrase you quoted either: the wording "must have a visible alternative" is totally confusing, since the background image is visible. However the two explanatory sentences makes more sense:

* "background images are not perceived by screen readers", we already have that covered with offscreen text in the last patch.
* "background images are not supported on phones with basic support for CSS", is this something that can actually be tested accurately? Does anyone have a model cellphone like this? If so what brand/model? I'd make a bold statement that we are pretty far from actually supporting a device like this, and so blocking this issue on accessibility of a feed icon, which a phone like this is not likely to have an application to be able to read (yes huge sensational generalization!!) feels like a stretch.

I think in general the guide is not worded well, and doesn't take into account icons (which the BBC UI guide site itself uses for next/previous icons). I still think this would be better going from an inline img tag, to an inline svg tag here, as that solution would satisfy every concern (both accessibility and themability).

I would be happy with a temporary loss of accessibility to get this patch in, and then continue work on #2427213: Replace feed.png with feed.svg and come to consensus there.

@jwilson3 Yeah the problem is that we don't have very lenient accessibility guidelines as they become hard to apply if we just decide that sometimes we don't have to follow them. It's difficult to have that kind of logic in such a large project with some many people involved. I don't see how #2427213: Replace feed.png with feed.svg is going to fix this problem without undoing the work already done in this issue.

Also see the comment #62:

If the mobile browser doesn't support background images or a user chooses not to load images (all mobile browsers have this setting).

This BBC guidelines aren't greatly worded but they do raise a good point. We can't guarantee that images will load on any device.

I would rather have a minor design change in exchange for greater flexibility and a simpler Themer Experience. We shouldn't be that precious about the default design of Drupal, as we've spent the whole of D8 making it very easy to change the defaults.

get this to rtbc then do a followup issue for the "design change" so we can move forward.

Why not commit the design change here? The work is already done. It feels like an a followup that will get forgotten and then we are left with an accessibility regression.

tbh i havent found the bbc guide "demands" anywhere else on the net and its a completely normal way of doing "link icons"

A lot of web projects don't have high accessibility standards like Drupal does.

btw what if the user chooses to not load js, is that the next step as we now are looking at the 0.00001% that might not load images on a webpage

We already support users that do not run JS.

Having a text line in the bottom of all pages that says subscribe to this issue gives me a design headache, it looks fugly.
and Its a design change thats out of the scope of this issue.

You mean you don't want to make this change at all? Then we can't make any progress here. We can't introduce a regression in this issue with no intention of fixing it. We can't just ignore our accessibility standards when it's inconvenient for us.

We just need to decide which trade off is more beneficial. I think it's more useful to have the icon in CSS and have to hide some text than to have the icon in a twig file somewhere.

I'm trying to get what's discussed here, but I fail to understand the accessibility issue here. If we replace a text with an image with CSS, it's perfectly fine to do it like we do it now, using .element-invisible. I believe the BBC guidelines are there for background images that add extra information, that's not already in the source code.

Screen readers can read out 'Subscribe to @site-name' as it is hidden with an .element-invisible instead of a display:none.

While at it, can we remove the span at all? See patch.

Shuck. Forgot to remove the twig file from Classy.

Updated the tests.

1. +++ b/core/modules/system/css/system.theme.css
   @@ -162,6 +162,19 @@ abbr.ajax-changed {
   +  overflow: hidden;
   +  text-indent: -9999px;
   +  display: block;
   
   +++ b/core/modules/system/templates/feed-icon.html.twig
   @@ -5,14 +5,13 @@
   +<a href="{{ url }}"{{ attributes.addClass('feed-icon') }}>
   +  {{ 'Subscribe to @title'|t({'@title': title}) }}
   +</a>
   

   I think from a11y point of view this should be good, but maybe we can get @mgifford have a quick weigh in?

2. +++ b/core/themes/bartik/css/base/elements.css
   @@ -130,8 +130,8 @@ blockquote > p:first-child {
    a.feed-icon {
   -  display: inline-block;
   -  padding: 15px 0 0 0;
   ...
   +  margin: 25px 0 0 0;
    }
   

   Why a change from 15px to 25px? And inline-block to block?
   I only understand the padding to margin change because of the content change inside the link.

   Also, wonder if I can scope creep this a tiny bit and remove the 'a.' selector from this class since we are touching the entire contents already?

Sure, I'll ask other a11y enthusiasts to chip in. However, I'm pretty sure this is fine as-is.
I removed the a selector too. The margin/padding change is introduced in comment 25.

Thanks for the changes @BarisW. I did read up the thread and noticed #25 was the culprit there, should have asked @mortendk directly on that one. @mortendk can you shed some light on that?

Whoops #29 @mortendk regarding the inline-block/block and top margin size changes. #25 is me:)

#80 Is there a reason to not use visually-hidden?

#73 We do already use inline image in CSS files, so I am not sure why the objection to use inline SVG in a template, it's just "code" and we only ever need two of these files as per the standard core/classy duplication.

Perhaps the biggest reason I am surprised at the objections to inline SVG is that it actually solves all the problems quite elegantly and is becoming a pretty standard way to do SVG icons, mainly because of the all-powerful control via CSS it allows. Designers have been gagging for years to be able to use this technique in production, now we actually can.

Is there a reason to not use visually-hidden?

It saves a span to do it this way. If we would use the visually-hidden on the a element, the background-image would be invisible too.

Regarding SVG, let's tackle that #2427213: Replace feed.png with feed.svg

Seems just fine for accessibility. In testing this on SimplyTest.me I only saw the "Subscribe to" and not "Subscribe to name of site":

Maybe that's intentional.

@BarisW - Also, #2427213: Replace feed.png with feed.svg is postponed on this issue. I don't mind if it's dealt with here (or there) but could well be either. Might be easier to combine them though.

@mgifford - regarding the "Subscribe to", I'm working on that one in #2082657: Feed icon on the front page misses site title.

I'm trying to get what's discussed here, but I fail to understand the accessibility issue here.

If images are disabled nothing will show. Previously with img element the alt text would show. Thats the basic issue. There are three ways to solve this:

1. Use img element.
2. Stick a label on it that always shows.
3. Use an inline SVG image.

2 requires a design change, however it's a common idea. I'm not sure I understand the objections to this based on design arguments because using a visual label is quite common. I am not against this as a solution, however I simply prefer no-label which is what I think most of the objections are - personal preference for minimalism.

1 and 3 do not require a design change but are at what we might call opposite ends of the technology spectrum, 1 is old school and 3 is bleeding edge. 1 is a little bit less maintenance but 3 can be styled with CSS. Both pass the images OFF test which no true IR method I have seen can do (none, only ones that use actual img elements can do this).

3 can't be cached, this is worthy of thought, however the code is pretty small and can be gzipped of course.

With regards to the IR method

The Phark IR method (being used in #81) has a performance issue because it draws a huge box offscreen (think paint and reflow), the Kellum method does not do this and is better browser performance. If we are going to sacrifice images OFF then lets at least test with a more up to date IR technique.

Note also that the Phark method can trigger huge scroll bars in RTL in some browsers, however that may be a non-issue, this used to be a big issue in IE7 etc, which is why we simply could not use this for hiding content back in the day and instead we came up with the clip method used in visually-hidden. I would strongly suggest testing RTL with any text-indent method and you need to do this in all supported browsers. Adding a new IR method (like Kellum or Phark) to core requires testing for accessibility and RTL.

@Jeff Burnz I believe there is a maintenance issue around inline SVGs. You can't just edit the file to make changes(save for color changes and a few other minor changes that can be guessed). I'd avoid inline SVG for that sake even though I know it's better for performance. TX is a bigger concern than the performance gains made from this.

#90 You can't edit the PNG image either, which hasn't been a concern before. Neither could you just change the image in CSS, you had to use PHP. I don't really understand this objection to be honest, its just a methodology and getting more common - I'm on projects now and we're using < use > and SVG sprites inline and all sorts of advanced techniques, this is totally normal.

OK, so if you guys are totally against inline SVG, then basically the only resolution here is to use a visible label, if you want to get rid of the img element, or abandon support for images off.

My vote - use the label, but wrap it in a span with with a class so themer can hide it very easily without having to edit a template.

The accessibility issue isn't for screenreaders, it's for users who can't work don't load images.

@mgifford Maybe you can verify whether we support about this user group or not?

@Jeff I think inline SVG is a great but I don't think we should implement it in this one off use case without discussion on how this implementation affects themer experience. It's more important that we make the image easy to override and replace rather than make the default output good.

More food for thought: The best icon is a text label

I think its easier to change an SVG than a raster image. Plus, how much will the users change this default icon? It's a standard.
We, the themers, will waste time replacing the raster image to an svg. You are only thinking in the non-themers who makes small changes, and will ever be mad because they don't know how to generate, download nor save an svg....

I think its easier to change an SVG than a raster image. Plus, how much will the users change this default icon? It's a standard.

If it is inline then it is harder, because if you override the background image you are still loading the old image within the stylesheet, which means you'll have to completely remove or replace that stylesheet to prevent that image from being loaded all the time. I think that icons like this are always replaced so they are consistent with other icons on the site.

I never use a background image except an image needs to be repeated o placed like a full background cover. In this case if that icon is a backgorund, its an error.
I could place that icon with:
<svg><use xlink:href="img/icons.svg#feed"/></svg>
or
<img src="img/feed.svg" alt="Feed">
There is no stylesheet here, only an svg with data because the icon it's a standard. If users wants to change it, they download a new svg or they edit icons.svg sprite and add the feed svg code. Simple.
I wrote some info at this issue if you are interested. Of course its incomplete, svg is a world apart.

Let's get this task back on track. All of the other icons in core (toolbar, admin cheverons, error/status icons), are defined as background images in CSS.

This issue is an intermediate step focused on accessibility, usability, and modern CSS to display icons. The change from a raster file to an svg (increasing consistency with all other background icons) will be tackled in #2427213: Replace feed.png with feed.svg once this is resolved.

The external vs. inline SVG discussion is a separate concern entirely, so feel free to open up a separate issue if you like.

Attaching a reroll of #81.

Reroll

So you use a background image when its not primarily the content of a side, but rather more of a styling?
It would be great if someone explains that quickly in the issue summary :)

Patch no longer applies.

@dawehner we haven't quite formalized (that I know of) when we should use CSS vs images.

Ultimately if meaning is conveyed through the image there should be a textual reference for it too (even if it's hidden with a visuallyhidden class).

Moving it to css gives more flexibility over the styling for themers. I think that's still the big advantage.

looking into this

rerolling a patch.

This looks great to me. Should make theming this much easier. This shouldn't present an accessibility problem to users either.

I think it is good to be RTBC, but probably needs to have some work on the issue summary before we do so to make sure it's clear to every.

This still doesn't take #89 into account (users who can't/won't show images but have CSS enabled). They won't see anything with this solution. A better solution would be to add a text to the image, like 'RSS feed'.

Edit: ignore what I said. #97 tackles this.

Agreeing with Mike Gifford. It'd be great to have this RTBC and then to sort the side issues that it presents as separate issues (but still before 8.0.0). I think getting what we have committed, will make it easier to fix those, as the issue won't be so "meta" (for want of another word).

Also, well done to all who kept on top of this.

+++ b/core/modules/system/templates/feed-icon.html.twig
@@ -5,14 +5,13 @@
+<a href="{{ url }}"{{ attributes.addClass('feed-icon') }}>
+  {{ 'Subscribe to @title'|t({'@title': title}) }}
+</a>

Shouldn't this be in one line? To prevent an extra space before "Subscribe"?

I tested this in in the browser and it does indeed create whitespace but it doesn't have any visual effect. I think this is the kind of thing that IE7 used to have issues with :-P

I've updated the issue summary with the discussion and added the beta eval.

There was two beta evaluations in the IS so I removed other one of them.

Committed 444c476 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.