Use CSS for file icons in file fields

Seems fine to me. The only thing that's not clear to me is that how to tell CSS what path to use for the image. I'll be happy to provide a patch if the above point is clarified.

@cs_shadow We tend to just use the relative path technique: ../../../misc/etc

@LewisNyman I was not referring to how to specify the path of the image. I actually wanted to ask that how do we determine which icon to use which depends on file type. If we intend to do it this way we'll have to define as many css classes as many filet types we have. Also this will be difficult to extend when we want to add a few more file types IMO. Since css class will depend on the file type, whats the best way to accomplish this?

@cs_shadow Looks like I never got back to you, sorry. I think it's ok to allow it to be extendable with new file types, as long as it doesn't look broken. We should use a default icon for all file types and then overwrite them for the ones we know about.

In my patch we add grouped class of the mime type for the link, and if there's no group, then we add a converted mime type class.

1. I'm not sure if we should have always a specific mime type class? The reason why we shouldn't add it always is that we have some mime types that are terribly long. Adding the specific classes could be a good idea in terms of adding extending the functionality, but in core we wouldn't need them. Any opinions?

2. Should we have always a file class included in the classes?

If there's any CSS wizards around, we could start implementing the CSS part.

Adding the classes would certainly make swapping them out easier if I didn't like the default implementation. Any thought about using font based icons rather than images?

This needs to be rerolled due to this: file.module - Convert theme_ functions to Twig

New patch attached.

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/GenericFileFormatter.php
@@ -28,6 +28,10 @@ class GenericFileFormatter extends FileFormatterBase {
+    ); ¶

redundant space

Thanks @swentel for noticing the typo. I removed it & updated the patch.

15: css-file-icons-2236855-15.patch queued for re-testing.

1. --- /dev/null
   +++ b/core/modules/file/css/file.generic_file.css
   

   This should be changed to something more descriptive such as file.icons.css

2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/GenericFileFormatter.php
   @@ -28,6 +28,10 @@ class GenericFileFormatter extends FileFormatterBase {
   +    $elements['#attached'] = array(
   +      'library' => array('file/drupal.file.formatter.generic_file'),
   +    );
   

   Tests are failing because of this

Indeed! That makes the field shown even when there is no file attached and hence fails the test. Thanks @lauriii for that! I'll provide a new patch soon.

New patch added. The css file is renamed to file.formatter.generic.css since it's used for generic_file formatter.

I hope it's working....

I think there are a couple of things missing in the re-rolled patch. On a site, I'm not seeing an image next to a file link, as we should expect.

I'm trying to include the extra bits now...

okey dokey - this builds on yuki77's work and now includes a few more of the changes proposed in #1 but up to date with 8.0.x. I think so, anyway - it does show the file icons via css - as seen in attached png.

It looks good. Patch applies cleanly and code looks good.

+1 for "I love Dries"

+++ b/core/modules/file/css/file.formatter.generic.css
@@ -0,0 +1,57 @@
+  padding-left: 20px;
...
+  background-position: left center;

It feels like this might need RTL-specific CSS too. Can you post an RTL screenshot for this?

Now it's fixed.

Ah - we need to remember to 'cancel' the left margin in [dir="rtl"] as well as set a right margin. Otherwise, we are introducing little extra blank margins after the link in rtl templates.

Come to think of it - do we need to think about this anywhere else?

Great! Thanks rachel_norfolk for fixing the issue correctly ;)

Now it looks great without extra paddings. It's a RTBC for me.

Overall this seems like a great change, and thanks for all of the RTL testing as well.

A couple of questions though:

1. +++ b/core/modules/file/file.module
   @@ -1589,80 +1578,50 @@ function template_preprocess_file_link(&$variables) {
   - * @param \Drupal\file\FileInterface $file
   + * @param \Drupal\file\File $file
   ...
   -function file_icon_path(FileInterface $file, $icon_directory = NULL) {
   ...
   +function file_icon_classes(File $file) {
   

   Is there a reason we removed the FileInterface type hint here in favour of File? That's unusual.

2. +++ b/core/modules/file/file.module
   @@ -1693,7 +1652,7 @@ function file_icon_map(FileInterface $file) {
   -      return 'x-office-document';
   +      return 'office-document';
   

   Curious, why this change (and the other x-office -> office changes)? Looking at the search results for "x-office-document" on Google, this seems to be a pretty standard way to label these icons.

Sorry I didn't get a chance to review this, I found an improvement we can make to the CSS.

+++ b/core/modules/file/css/file.formatter.generic.css
@@ -0,0 +1,64 @@
+.file.package-x-generic {
+  background-image: url(../icons/package-x-generic.png);
+}

We should be using SMACSS variants for this instead of chaining classes, so: .file--package-x-generic

Good work! This will be a great improvement.

+++ b/core/modules/file/file.module
@@ -1589,80 +1578,50 @@ function template_preprocess_file_link(&$variables) {
+  // Set file classes to the options array.
+  if (isset($variables['attributes']['class']) && is_array($variables['attributes']['class'])) {
...
   }
-  return FALSE;
+  else {
+    $variables['attributes']['class'] = file_icon_classes($file);
+  }

If this was already an attribute object as I'm attempting to do over at #2108771: Remove special cased title_attributes and content_attributes for Attribute creation this could become:

$variables['attributes']->addClass(file_icon_classes($file));

But simply now you could clean it up with:

$variables['attributes'] = new Attribute($variables['attributes']);
$variables['attributes']->addClass(file_icon_classes($file));

Because we have an array() in the hook_theme().

And ditto on questions form @webchick in #37

We can use the Attribute object to simplify everything - also we're calling $file->getMimeType a lot and in a loop during the preprocess I think we can remove this. Plus the logic is now a bit different - I think we should always add the full mime type to the class since this best reproduces the logic of testing whether or not a specific mime type icon file exists. If we keep the logic as in #35 then you won't be able to provide specific overrides only using css. #35 would also add the class returned by file_icon_map() and the category discovered in file_icon_class() - which seems wrong too (n.b. this could only occur for text files).

Patch attached addresses these issues and removes the now unnecessary array containing human-readable names, for use as text-alternatives to icons.

Also we need a change record because we removing a theme function and completely changing how file icons work.

The patch in #41 does not address the feedback from #37, #38, or #39.

Fixed the patch from #41 and changed CSS classes as suggested in #38. #39 was fixed already in @alexpotts patch.

Looks like the CSS file is missing from the patch

oh - the file.formatter.generic.css file seemed to go missing at some point.

I've done a bit of digging back and picked it up from #35. This means that the css changes do NOT include Lewis' SMACSS thing - I'm still working on that but wanted to drop this here in the meantime...

Ah - so I found out some things but I could use some input to help me understand what *should* happen...

In function template_preprocess_file_link(), we set the file class, a sanitized mime type class and our own 'grouped' mime type class, prefixed file-- as per SMACSS. The grouping is done in file_icon_map().
You can see how a few files are shown by my css so far (including showing the actually classnames) here:

I feel we should only set icons by default on those classes with the file-- prefix. I don't know why - it just feels right.
To do this, we need to either:

1. change ALL classes added in template_preprocess_file_link() to be prefixed file--. Mainly by adding this in line 1567 of file.module;
2. add some further popular mime types, like application/pdf and it's siblings to the file_icon_map() function.

Which would people prefer?

Okay, I thought it would be easiest to just make a choice and see how people feel about it!

So, I have chosen a mixture of both choices above. I have:

1. updated each css selector to use the form .file.file--some-filetype
2. told template_preprocess_file_link() to prefix each raw mime type as file--mime-some-mimetype
3. told template_preprocess_file_link() to check for dots in mime types and convert to dashes
4. added some pdf mime types into file_icon_map().

So, the classes of some files now look like below:

As you can see, we now have a situation where the most popular files are painted with a pretty icon and themes can override these AND add further icons within a group should they wish to.

+++ b/core/modules/file/css/file.formatter.generic.css
@@ -0,0 +1,67 @@
+.file.file--general {
+  background-image: url(../icons/application-octet-stream.png);
+}
+.file.file--package-x-generic {
+  background-image: url(../icons/package-x-generic.png);
+}

Thanks! We can actually just remove the .file class from the selectors now, so just .file--general etc.

I've updated the patch so others can review the PHP.

Whoops

<3 This patch:)

Gave it a read through and a peek on simplytest.me. Great work on this all!

Working for me :-)

Oh - and I have made a Change Record - see https://www.drupal.org/node/2342157

Rerolled after #2343661: Rename l() to _l() and url() to _url(), and document replacements

putting back to rtbc

back to rtbc again

It's the issue that just keeps on giving, eh? Sorry, the css file has gone AWOL.

(this does kind of make me think we should have a test to check that files referenced in the yml files do actually exist)

Including the css... (I hope!)

The patch applies cleanly and does exactly what it should.

Screenshot before:

Screenshot after:

sorry, accidental duplicate..

Here is a reroll #67.

Another try, as accidentally included interdiff in the patch.

One issue with this approach is that file icons will not appear if someone prints the page.

We could instead change this to something like

.file--general:before {
  content: url(../icons/application-octet-stream.png);
}

But I'm not sure how that plays out for accessibility.

hm, is that behaviour desirable? I know they were being printed before but that feels like an unintentional side effect of the previous technique rather than an intentional decision.

All icons should be presentational only and implemented with background-image and from what I can see they are consistently implemented with background-image throughout core. If browsers choose not to print presentational images then that is their decision, let's not make an exception here.

This feels like a simple decision that sticks to the rest of core. Leaving on needs review for a day or so so others can weigh in.

...and people can set their browsers to print background images, if they so choose.

@rachel_norfolk I didn't know that, I found some documentation.

I've written the change record, we need someone to check it over before we RTBC: https://www.drupal.org/node/2358511

Change record looks good, certainly better than mine!

Back to RTBC because this has been reviewed very thoroughly now and it's a *really* beneficial change. :-)

Assigning to alexpott, to give him a chance to verify that his concerns have been adequately answered.

I'm not that fussed about printing a web page - I just wanted to make sure that the concern with the new approach was discussed.

Here is an reroll.

Thanks for the rerolls. I've manually tested the patch again just to make sure. I think we are back to RTBC

Looks like alexpott's concern was resolved, and that answer is fine for me as well.

Committed/pushed to 8.0.x, thanks!

Publish the change record