From ef96eb02fb3998f8e1cebe8657cffbbfbefe6b2b Mon Sep 17 00:00:00 2001
From: Damyon Wiese <damyon@moodle.com>
Date: Thu, 10 Jan 2019 10:22:48 +0800
Subject: [PATCH] MDL-62680 output: Only hide icons with no label

For accessibility we don't want to read an icon with a label immediately next to the label,
but in this case it's clearer for the icon to have no alt text / title for both
screen readers and non-screen readers. Worse is not reading important information just
because it's displayed as an icon.
---
 .../output/icon_system_fontawesome.php        |  3 ++
 lib/outputcomponents.php                      |  5 ++++
 lib/outputrenderers.php                       |  2 +-
 lib/templates/pix_icon_fontawesome.mustache   |  2 +-
 lib/tests/outputcomponents_test.php           | 29 +++++++++++++++++++
 lib/upgrade.txt                               |  1 +
 6 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/lib/classes/output/icon_system_fontawesome.php b/lib/classes/output/icon_system_fontawesome.php
index 1530895f952..c65af9d2d10 100644
--- a/lib/classes/output/icon_system_fontawesome.php
+++ b/lib/classes/output/icon_system_fontawesome.php
@@ -444,6 +444,9 @@ class icon_system_fontawesome extends icon_system_font {
         if (!$subpix->is_mapped()) {
             $data['unmappedIcon'] = $icon->export_for_template($output);
         }
+        if (isset($icon->attributes['aria-hidden'])) {
+            $data['aria-hidden'] = $icon->attributes['aria-hidden'];
+        }
         return $output->render_from_template('core/pix_icon_fontawesome', $data);
     }
 
diff --git a/lib/outputcomponents.php b/lib/outputcomponents.php
index 772385e2f43..7824d23915c 100644
--- a/lib/outputcomponents.php
+++ b/lib/outputcomponents.php
@@ -694,6 +694,11 @@ class pix_icon implements renderable, templatable {
             // and some browsers might overwrite it with an empty title.
             unset($this->attributes['title']);
         }
+
+        // Hide icons from screen readers that have no alt.
+        if (empty($this->attributes['alt'])) {
+            $this->attributes['aria-hidden'] = 'true';
+        }
     }
 
     /**
diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php
index 6ccfa79d09d..a3d110e319e 100644
--- a/lib/outputrenderers.php
+++ b/lib/outputrenderers.php
@@ -3487,7 +3487,7 @@ EOD;
                         // Process this as a link item.
                         $pix = null;
                         if (isset($value->pix) && !empty($value->pix)) {
-                            $pix = new pix_icon($value->pix, $value->title, null, array('class' => 'iconsmall'));
+                            $pix = new pix_icon($value->pix, '', null, array('class' => 'iconsmall'));
                         } else if (isset($value->imgsrc) && !empty($value->imgsrc)) {
                             $value->title = html_writer::img(
                                 $value->imgsrc,
diff --git a/lib/templates/pix_icon_fontawesome.mustache b/lib/templates/pix_icon_fontawesome.mustache
index 7f95b6c292d..34e74bda01d 100644
--- a/lib/templates/pix_icon_fontawesome.mustache
+++ b/lib/templates/pix_icon_fontawesome.mustache
@@ -43,7 +43,7 @@
 
 }}
 {{^unmappedIcon}}
-<i class="icon fa {{key}} fa-fw {{extraclasses}}" aria-hidden="true" {{#title}}title="{{title}}"{{/title}} aria-label="{{alt}}"></i>
+<i class="icon fa {{key}} fa-fw {{extraclasses}}" {{#aria-hidden}}aria-hidden="true"{{/aria-hidden}} {{#title}}title="{{title}}"{{/title}} {{#alt}}aria-label="{{alt}}"{{/alt}}></i>
 {{/unmappedIcon}}
 {{#unmappedIcon}}
 {{! We cannot include the pix_icon template directly here because we don't have all the mustache helpers loaded. }}
diff --git a/lib/tests/outputcomponents_test.php b/lib/tests/outputcomponents_test.php
index de5ec1ebd4b..67e2c0cd0fc 100644
--- a/lib/tests/outputcomponents_test.php
+++ b/lib/tests/outputcomponents_test.php
@@ -436,4 +436,33 @@ EOF;
         $this->assertEquals($expecteda, $pbara->pagelinks);
         $this->assertEquals($expectedb, $pbarb->pagelinks);
     }
+
+    public function test_pix_icon() {
+        $this->resetAfterTest();
+
+        $page = new moodle_page();
+
+        set_config('theme', 'boost');
+        // Need to reset after changing theme.
+        $page->reset_theme_and_output();
+        $renderer = $page->get_renderer('core');
+
+        $reason = 'An icon with no alt text is hidden from screenreaders.';
+        $this->assertContains('aria-hidden="true"', $renderer->pix_icon('t/print', ''), $reason);
+
+        $reason = 'An icon with alt text is not hidden from screenreaders.';
+        $this->assertNotContains('aria-hidden="true"', $renderer->pix_icon('t/print', 'Print'), $reason);
+
+        // Test another theme with a different icon system.
+        set_config('theme', 'clean');
+        // Need to reset after changing theme.
+        $page->reset_theme_and_output();
+        $renderer = $page->get_renderer('core');
+
+        $reason = 'An icon with no alt text is hidden from screenreaders.';
+        $this->assertContains('aria-hidden="true"', $renderer->pix_icon('t/print', ''), $reason);
+
+        $reason = 'An icon with alt text is not hidden from screenreaders.';
+        $this->assertNotContains('aria-hidden="true"', $renderer->pix_icon('t/print', 'Print'), $reason);
+    }
 }
diff --git a/lib/upgrade.txt b/lib/upgrade.txt
index 1c2f473b55a..4a4aa5e6bb6 100644
--- a/lib/upgrade.txt
+++ b/lib/upgrade.txt
@@ -4,6 +4,7 @@ information provided here is intended especially for developers.
 === 3.7 ===
 
 * The method core_user::is_real_user() now returns false for userid = 0 parameter
+* Icons are displayed for screen readers unless they have empty alt text (aria-hidden). Do not provide an icon with alt text immediately beside an element with exactly the same text.
 
 === 3.6 ===