Replace arrow-asc and arrow-desc images with Libricons and implement using CSS

I replaced the arrow-asc.png and arrow-desc.png files in /core/misc with /core/misc/icons/bebebe/twistie-down.svg and /core/misc/icons/bebebe/twistie-up.svg. Modified template_preprocess_tablesort_indicator in /core/includes/theme.inc to set the 'arrow_asc' and 'arrow_desc' values in $variables to the new icons.

Note that in the default admin theme, Seven, these values are overridden to point to $theme_path . '/images/arrow-asc.png' and $theme_path . '/images/arrow-desc.png.' Based on https://www.drupal.org/node/2032773 it looks like that would be a separate issue that has already been discussed.

@ctraltdel: Could you please put before and after screenshots in the issue summary?

In the screenshots the arrows are slightly smaller and they end up higher. The size doesn't matter, but is there any CSS attached to these that needs to be adjusted?

This looks good in classy as well. I am wondering why Seven didn't go with these. @ctraltdel, maybe you could discuss on that related issue if it was just overlooked.

@davidhernandez Good point. I should have waited for you first!

Since these are SVGs, I assume we can just make them bigger.

I'm not actually seeing any CSS attached to them, so if that is the case I would leave it be. I wouldn't add extra CSS unless Lewis thinks moving them is important.

Screenshots in IS.

I just talked to emmamaria in IRC and she would like to fix the alignment in Bartik. It should only be a small bit of CSS. Add it to table.css in Bartik's components folder.

I updated the table.css stylesheet in Bartik to align the images.

I made the suggested css updates. The end result has not changed from previous screenshots.

Just a quick FYI. When only changing part of a patch, it is good to upload an interdiff. It makes reviewing a little easier. https://www.drupal.org/documentation/git/interdiff

Tested #16 in OSX (10.9.5) under Firefox 35.0, Safari 7.1.2, Chrome 39.

Looks good in Bartik, Stark and Classy.

Tested #16 on windows 7 home premium sp 1 under Firefox 34.0.5, IE 11.0.9600, Chrome 39.0.2171.99.

Bartik, Stark, Classy and Seven look fine.

Well, it's green again. Did we have temporary bot problems?

@davidhernandez the first run failed in an unrelated test in ViewExecutableTest.

changed markup in tablesort-indicator.html.twig to

{% if style == 'asc' -%}
  <span {{attributes.addClass('tablesort tablesort--asc') }}></span>
{% else -%}
  <span {{attributes.addClass('tablesort tablesort--dsc') }}></span>
{% endif %}

so we don't need template_preprocess_tablesort_indicator in theme.inc, also we can change the indicator- style themewise with only drop some lines of css.

All Drupal Core Themes should then add tablesort-indicator.css in their css components.

@dernetzjaeger thank you for the patch and the idea. It was RTBC'd before so will this change give that much improvement? Glad to see the preprocess go away, so that's nice:)

Is there supposed to be CSS with this new patch? Can you somehow get the accessibility back via aria-descriptions or something?

Here's a bit of Twig/class name review:

+++ b/core/includes/theme.inc
index 2edbb17..0000000
--- a/core/misc/arrow-asc.png

+++ b/core/modules/system/templates/tablesort-indicator.html.twig
@@ -12,7 +12,7 @@
+  <span {{attributes.addClass('tablesort tablesort--asc') }}></span>
...
+  <span {{attributes.addClass('tablesort tablesort--dsc') }}></span>

Minor nit pick but don't put a space before the starting print curlies but do but it after because when attributes print it puts a space before each attribute printed.

And 'desc' would be slightly more preferable to 'dsc' as we usually don't use abbreviations with missing letters.

I'm fine with the previous RTBC patch in #16 as well except th img {} is a bit too generic of a selector for the CSS, if this issue goes back to that, I think that should be addressed if possible.

@joelpittet it was RTBC'd and then resetted to needs work, that was confusing.

Yes, i think it's an improvement, because if you would like to change the indicator style, you don't need to rewrite the preprocessing function.

No, there is no css in that patch, because I think that is more theme related. We should then open issues for each theme. Here is a related issue for Seven Clean up images used in the Seven theme.

ARIA: sure, you are right!
We could add aria-sort attribute to both spans.

Thanks for your notes and yeah "desc" is quit better ;)

If the graphic is being removed in favor of a css based solution, there should be a core equivalent. The arrows are functional and not just for styling.

@davidhernandez
Yes, of course we need to add the arrows to a core css.

.tablesort--asc {
  background-image: url(../../../../misc/icons/5181c6/twistie-down.svg);
}
.tablesort--desc {
  background-image: url(../../../../misc/icons/5181c6/twistie-up.svg);
}

but the positioning can be done in theme css using .tablesort class.

+++ b/core/modules/system/templates/tablesort-indicator.html.twig
@@ -11,8 +11,18 @@
+<span {{attributes.addClass( classes ) }}>
+  <span class="visually-hidden">
+    {% if style == 'asc' -%}
+      {{ 'Sort ascending.'|t }}
+    {% else -%}
+      {{ 'Sort descending.'|t }}
+    {% endif %}
+  </span>
+</span>

The text for screenreaders is causing the test to fail. I wonder if this could go in a followup or if it should be fixed in this issue?

+<span {{attributes.addClass( classes ) }}>

Spacing here needs to be fixed. Should be:

+<span{{ attributes.addClass(classes) }}>

@idebr, all patches have to pass testing or they won't get committed, so it will have to be fixed here.

@davidhernandez Yes, I'm aware :). I was just wondering if the additional text for screenreaders should be added in this issue or in a followup, since I didn't expect it to be included in this issue based on its current title and issue summary.

Added the fix mentioned #37.

@emma.maria thank you for fixing that.

@idebr Do you know why screenreader tests are failing?

@LewisNyman et al Should those new SVGs be added to seven theme folder as their specific to that theme?

Old markup for a clicksortable table header:

<a title="sort by ID" href="/test_click_sort?order=id&amp;sort=desc">
  ID
  <img width="13" height="13" title="sort descending" alt="sort descending" src="http://drupal8.localhost/core/misc/arrow-desc.png">
</a>

New markup for a clicksortable table header:

<a title="sort by ID" href="/test_click_sort?order=id&amp;sort=desc">
  ID
  <span class="tablesort tablesort--desc">
    <span class="visually-hidden"> Sort descending. </span>
  </span>
</a>

The test fails on

$this->clickLink(t('ID'));

rerolling patch :)

@lewisnyman Can you please verify

What is the difference between #48 and #49? Please upload an interdiff. #49 was green, so I don't know why it needed to be rerolled.

@davidhernandez Actually i was talking to lewisnyman on IRC about this issue, After that i have reroll a patch and upload it but forget to reload browser. But in the mean time nlisgo also uploaded the same.

Happy for my file to be hidden and the patch @Manjit.Singh provided to be put forward for review but an interdiff would be useful as there does appear to be a small difference in the patch files.

creating interdiff first time, can you please check whether i have'nt done it wrong :)

There were some issues with the interdiff in #53. I use the 'Create interdiff using git' approach documented in https://www.drupal.org/documentation/git/interdiff

Here is the interdiff.

There are some issues with both patches in #48 and #49 but less with #48 so I will resupply a patch with an interdiff with #48.

Feedback for the patch in #49:

The svg paths have a fill colour of #787878 but I believe this should be the fill colour #004875 or #008ee6 depending on which folder the svg is in.
The name of the tablesort-indicator.css.css file is incorrect and should be tablesort-indicator.css

I incorrectly applied the patch to system.theme.css in #48 which is why I need to prepare it again.

The interdiff highlights the mistake in the patch prepared in #48 but this is really just a reroll of #40.

Please review.

I love the fact that someone is taking the time to make the PNG's into SVG's. It saves a lot of space and it automatically scales for retina devices, so that is great.

However, it doesn't load the SVG's. If I apply the patch (which it does brilliantly) and I go to /admin/people I get nothing.

<a href="/admin/people?user=&role=All&permission=All&status=All&=Filter&order=created&sort=asc" title="sort by Member for"> Member for <img src="" width="13" height="13" alt="sort ascending" title="sort ascending" /> on Seven.

Here is a picture from the Chrome Devtools inspector:

Also, I don't see anything from the tablesort-indicator.html.twig when I search for it in the source code.

I really do hope this get's into Drupal8.

@Noe_ I see the same.

Needed to get rid of the template in the classy theme also. See the interdiff.

Addressed failing tests.

#61 looks pretty good. It just works on the Seven theme right?
I changed the theme to Bartik and there it doesn't work.

@lewisnyman Please verify.

I love the way this is looking, less need for preprocess to add an image is a big win IMO.

Template removal should be fine.

We should get some screenshots and Issue Summary Update resolved.

1. +++ b/core/modules/system/templates/tablesort-indicator.html.twig
   @@ -11,8 +11,18 @@
   +<span {{ attributes.addClass(classes) }}>
   

   got an extra space in there before the {{, that can be removed.

2. +++ b/core/modules/system/templates/tablesort-indicator.html.twig
   @@ -11,8 +11,18 @@
   +      {{ 'Sort ascending.'|t }}
   ...
   +      {{ 'Sort descending.'|t }}
   

   Do these need periods? It wasn't before, the capitol is fine though I think(remember to change the click link test if you remove the period.

@joelpittet Please find and review the patch :)

+++ b/core/modules/system/templates/tablesort-indicator.html.twig
@@ -11,8 +11,18 @@
+      {{ 'Sort ascending.'|t }}
...
+      {{ 'Sort descending.'|t }}

+++ b/core/modules/views/src/Tests/Handler/FieldWebTest.php
@@ -74,7 +74,7 @@ public function testClickSorting() {
+    $this->clickLink(t('ID Sort descending.'));

Please remove the periods from these too.

Addressing feedback in #69.

@nlisgo Patch is looks like good... but don't know why testing is failed everytime.
@lewis Can you have some time to look upon.

from myside all (UI) looks good :)

@Manjit.Singh sometimes testbot will fail but it is not due to the code.

See: https://www.drupal.org/automated-testing/troubleshooting

I was not able to reproduce the error locally so I determined that it was an issue with the testbot and simply queued the patch for a retest.

Screenshots looks fine for me !! thanks lewis

+++ b/core/includes/theme.inc
@@ -968,25 +968,6 @@ function template_preprocess_table(&$variables) {
 /**
- * Prepares variables for tablesort indicator templates.
- *
- * Default template: tablesort-indicator.html.twig.
- *
- * @param array $variables
- *   An associative array containing:
- *   - style: Set to either 'asc' or 'desc'. This determines which icon to show.
- */
-function template_preprocess_tablesort_indicator(&$variables) {
-  // Provide the image attributes for an ascending or descending image.
-  if ($variables['style'] == 'asc') {
-    $variables['arrow_asc'] = file_create_url('core/misc/arrow-asc.png');
-  }
-  else {
-    $variables['arrow_desc'] = file_create_url('core/misc/arrow-desc.png');
-  }
-}

This needs a change record

I started a draft CR.

A novice can finish the change record.

I would be nice to show the markup before and after, as well as how someone would override the icons after this patch.

In case you are wondering, this information is needed in the change record.

Removing sprint weekend tag!!
As suggested by @YesCT

Tried to apply the patch, changes to Seven theme YAML file.

I've re-rolled this patch against the latest commit (251ca21).

There was a conflict in the file core/themes/seven/seven.libraries.yml - an additional CSS file was added by this patch and in the main branch. I kept both CSS files.

I've tested the changes and it appears to work as expected.

Just tested the #90 patch--all appears to be in working order, tested under Classy, Bartik, and Stark themes. To be sure that things were OK, a few displays were checked across the site.

Manually tested the #90 patch across different displays in Classy, Stark, and Bartik themes. Also did a review of all lines in the patch, and they appear to be in the scope of the issue (but could stand to have more experienced eye review) of the patch code, and changes appear OK. Switching back to the 8.0.x branch to compare gave further indication the patch is working as advertised.

While going back to make screenshots to document the patch changes, I noticed another issue--In the Bartik theme, the arrows appear to disappear, since they are the same color as the default table heads. This is evident in the highlighted file.

Added remaining tasks to issue summary. Am now going to work on change record.

New screen grabs, to be referenced in the issue summary.

Issue summary updated.

There is a draft change record now--the first I've ever written. Link

I'm going to fix the problem with the Bartik theme that was mentioned in comment #93

I'm checking the beta evaluation

I'm not sure if this is unfrozen because it does change the PHP test and a yml file but both of those look like minor changes.

I have confirmed that it works in every theme except for Bartik.

This was part of a Drupal Association sprint. The sprint is over, so other people are free to take over the issue.

@clacina - what was your plan for Bartik? We can't change the default table heads for Bartik I assume.

I'm assuming we should just create a new twistie-down.svg & twistie-up.svg pair for Bartik that is a lighter shade. I think this is the grey that is closer to what is there #b6b6b6 so we could just create a:
core/themes/bartik/images/b6b6b6/twistie-down.svg

Here's a re-roll because a few things moved around. For bartik, I suggest providing some svg icons with better colors. Because it is there but the color just matches the background. @emma.marie do you have a bartik color palette to work with for those colors?

In the re-roll I moved
core/modules/system/css/components/tablesort.theme.css
and what added to core/modules/system/css/system.module.css
into
core/modules/system/css/components/tablesort.module.css

Hope I got the name correct (barely understand module/theme structure)

+++ b/core/themes/seven/css/components/tablesort-indicator.css
@@ -0,0 +1,26 @@
+[dir="rtl"] .tablesort {
+  float: left;
+}

Oh also added this for RTL.

For bartik added

core/misc/icons/ffffff/twistie-up.svg
core/misc/icons/ffffff/twistie-down.svg and
core/themes/bartik/css/components/tablesort-indicator.css

White icons on gray background. See screnshot how it looks and should be reviewed by theme maintainer.

What's happening in system.libraries.yml? There's 'every_page: true' added on every css component. Is this intended and what does it do?

every_page was removed from core so that shouldn't be there. @avitslv make sure your development copy of Drupal 8 is up to date with the latest head. You should see those every_page entries go away.

Resubmitted patch. Fixed the mixup for system.libraries.yml, thanks @jiv_e @davidhernandez

This time added Desc arrow screenshot for bartik:

I checked out the arrow-asc and arrow-desc in Bartik. They look great! Such a visual improvement for Bartik having the arrows white and the arrows work as expected in all tables. Thanks everyone!

Screenshots for good measure.
 

 

There seems to be a couple change records so removing that tag.

Had a quick glance at Seven theme and it's still good too.

patch #111 needs a reroll

Patch rerolled #111

Shouldn't this patch also remove core/misc/arrow-asc.png core/misc/arrow-desc.png and any references to them?

Patch updated as per #123

Forget to remove reference for arrow-asc and arrow-desc files

The sizes of the patch files for #121, #124 and #125 are significantly smaller than the patch in #111 which was RTBC. I just quickly attempted a re-roll and the size of my patch file is around 12K. So something has gone wrong.

Here is a re-roll of #111 for review.

@nlisgo you beat me to it I agree with a reroll of #111. The patch in #127 contains everything that was in #111.

Setting to RTBC when the patch goes green.

New CI passes. Old testbot has AWS errors?

Setting back to RTBC per #129. The fails were testbottiness.

Markup and templates are yet frozen. And bartik looks much better - nice work everyone. Committed 8bcdbf2 and pushed to 8.0.x. Thanks!