From 2c93a1f53c638d2c6f96ccc6e97dd1cfd5298b1a Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 25 Sep 2020 21:07:10 +1000 Subject: [PATCH 1/2] MDL-69649 backup: Fix missing labels - The backup details page uses a table to show a sumary of the backup content. Used role attribute to denote the tabular format of the summary. - The backup details page displays activity name next to each activity icon. Therefore the icons are only decorative and do not need to have any title or even alt text. - Form labels should be associated with form controls. A div element is not a form control. - The from attribute of the form labels should be equal to the id attribute of an element. Therefore, we first create a label and an input elements and associate them to each other, and then pass them to backup_detail_pair() when a label is needed. --- backup/util/ui/renderer.php | 104 ++++++++++++++++++++++++------------ lang/en/moodle.php | 1 + 2 files changed, 71 insertions(+), 34 deletions(-) diff --git a/backup/util/ui/renderer.php b/backup/util/ui/renderer.php index 4a1f187f921..d4041266fac 100644 --- a/backup/util/ui/renderer.php +++ b/backup/util/ui/renderer.php @@ -126,8 +126,9 @@ class core_backup_renderer extends plugin_renderer_base { $html = html_writer::start_tag('div', array('class' => 'backup-restore')); - $html .= html_writer::start_tag('div', array('class' => 'backup-section')); - $html .= $this->output->heading(get_string('backupdetails', 'backup'), 2, array('class' => 'header')); + $html .= html_writer::start_tag('div', ['class' => 'backup-section', + 'role' => 'table', 'aria-labelledby' => 'backupdetailsheader']); + $html .= $this->output->heading(get_string('backupdetails', 'backup'), 2, 'header', 'backupdetailsheader'); $html .= $this->backup_detail_pair(get_string('backuptype', 'backup'), get_string('backuptype'.$details->type, 'backup')); $html .= $this->backup_detail_pair(get_string('backupformat', 'backup'), get_string('backupformat'.$details->format, 'backup')); $html .= $this->backup_detail_pair(get_string('backupmode', 'backup'), get_string('backupmode'.$details->mode, 'backup')); @@ -153,8 +154,9 @@ class core_backup_renderer extends plugin_renderer_base { $html .= html_writer::end_tag('div'); - $html .= html_writer::start_tag('div', array('class' => 'backup-section settings-section')); - $html .= $this->output->heading(get_string('backupsettings', 'backup'), 2, array('class' => 'header')); + $html .= html_writer::start_tag('div', ['class' => 'backup-section settings-section', + 'role' => 'table', 'aria-labelledby' => 'backupsettingsheader']); + $html .= $this->output->heading(get_string('backupsettings', 'backup'), 2, 'header', 'backupsettingsheader'); foreach ($details->root_settings as $label => $value) { if ($label == 'filename' or $label == 'user_files') { continue; @@ -164,8 +166,9 @@ class core_backup_renderer extends plugin_renderer_base { $html .= html_writer::end_tag('div'); if ($details->type === 'course') { - $html .= html_writer::start_tag('div', array('class' => 'backup-section')); - $html .= $this->output->heading(get_string('backupcoursedetails', 'backup'), 2, array('class' => 'header')); + $html .= html_writer::start_tag('div', ['class' => 'backup-section', + 'role' => 'table', 'aria-labelledby' => 'backupcoursedetailsheader']); + $html .= $this->output->heading(get_string('backupcoursedetails', 'backup'), 2, 'header', 'backupcoursedetailsheader'); $html .= $this->backup_detail_pair(get_string('coursetitle', 'backup'), $details->course->title); $html .= $this->backup_detail_pair(get_string('courseid', 'backup'), $details->course->courseid); @@ -200,7 +203,7 @@ class core_backup_renderer extends plugin_renderer_base { $table->data = array(); } $name = get_string('pluginname', $activity->modulename); - $icon = new image_icon('icon', $name, $activity->modulename, array('class' => 'iconlarge icon-pre')); + $icon = new image_icon('icon', '', $activity->modulename, ['class' => 'iconlarge icon-pre']); $table->data[] = array( $this->output->render($icon).$name, $activity->title, @@ -424,13 +427,25 @@ class core_backup_renderer extends plugin_renderer_base { protected function backup_detail_pair($label, $value) { static $count = 0; $count ++; - $html = html_writer::start_tag('div', array('class' => 'detail-pair')); - $html .= html_writer::tag('label', $label, array('class' => 'detail-pair-label', 'for' => 'detail-pair-value-'.$count)); - $html .= html_writer::tag('div', $value, array('class' => 'detail-pair-value pl-2', 'name' => 'detail-pair-value-'.$count)); + $html = html_writer::start_tag('div', ['class' => 'detail-pair', 'role' => 'row']); + $html .= html_writer::tag('div', $label, ['class' => 'detail-pair-label mb-2', 'role' => 'cell']); + $html .= html_writer::tag('div', $value, ['class' => 'detail-pair-value pl-2', 'role' => 'cell']); $html .= html_writer::end_tag('div'); return $html; } + /** + * Creates a unique id string by appending an incremental number to the prefix. + * + * @param string $prefix To be used as the left part of the id string. + * @return string + */ + protected function make_unique_id(string $prefix): string { + static $count = 0; + + return $prefix . '-' . $count++; + } + /** * Created a detailed pairing with an input * @@ -448,9 +463,11 @@ class core_backup_renderer extends plugin_renderer_base { } else { $description = ''; } + $id = $this->make_unique_id('detail-pair-value'); return $this->backup_detail_pair( - $label, - html_writer::empty_tag('input', $attributes + array('name' => $name, 'type' => $type, 'value' => $value)) . $description + html_writer::label($label, $id), + html_writer::empty_tag('input', $attributes + ['id' => $id, 'name' => $name, 'type' => $type, 'value' => $value]) . + $description ); } @@ -718,8 +735,6 @@ class core_backup_renderer extends plugin_renderer_base { * @return string */ public function render_restore_course_search(restore_course_search $component) { - $url = $component->get_url(); - $output = html_writer::start_tag('div', array('class' => 'restore-course-search mb-1')); $output .= html_writer::start_tag('div', array('class' => 'rcs-results table-sm w-75')); @@ -733,11 +748,18 @@ class core_backup_renderer extends plugin_renderer_base { if (!$course->visible) { $row->attributes['class'] .= ' dimmed'; } - $row->cells = array( - html_writer::empty_tag('input', array('type' => 'radio', 'name' => 'targetid', 'value' => $course->id)), - format_string($course->shortname, true, array('context' => context_course::instance($course->id))), - format_string($course->fullname, true, array('context' => context_course::instance($course->id))) - ); + $id = $this->make_unique_id('restore-course'); + $row->cells = [ + html_writer::empty_tag('input', ['type' => 'radio', 'name' => 'targetid', 'value' => $course->id, + 'id' => $id]), + html_writer::label( + format_string($course->shortname, true, ['context' => context_course::instance($course->id)]), + $id, + true, + ['class' => 'd-block'] + ), + format_string($course->fullname, true, ['context' => context_course::instance($course->id)]) + ]; $table->data[] = $row; } if ($component->has_more_results()) { @@ -779,8 +801,6 @@ class core_backup_renderer extends plugin_renderer_base { * @return string */ public function render_import_course_search(import_course_search $component) { - $url = $component->get_url(); - $output = html_writer::start_tag('div', array('class' => 'import-course-search')); if ($component->get_count() === 0) { $output .= $this->output->notification(get_string('nomatchingcourses', 'backup')); @@ -790,6 +810,8 @@ class core_backup_renderer extends plugin_renderer_base { 'type' => 'text', 'name' => restore_course_search::$VAR_SEARCH, 'value' => $component->get_search(), + 'aria-label' => get_string('searchcourses'), + 'placeholder' => get_string('searchcourses'), 'class' => 'form-control' ); $output .= html_writer::empty_tag('input', $attrs); @@ -825,11 +847,18 @@ class core_backup_renderer extends plugin_renderer_base { if (!$course->visible) { $row->attributes['class'] .= ' dimmed'; } - $row->cells = array( - html_writer::empty_tag('input', array('type' => 'radio', 'name' => 'importid', 'value' => $course->id)), - format_string($course->shortname, true, array('context' => context_course::instance($course->id))), - format_string($course->fullname, true, array('context' => context_course::instance($course->id))) - ); + $id = $this->make_unique_id('import-course'); + $row->cells = [ + html_writer::empty_tag('input', ['type' => 'radio', 'name' => 'importid', 'value' => $course->id, + 'id' => $id]), + html_writer::label( + format_string($course->shortname, true, ['context' => context_course::instance($course->id)]), + $id, + true, + ['class' => 'd-block'] + ), + format_string($course->fullname, true, ['context' => context_course::instance($course->id)]) + ]; $table->data[] = $row; } if ($component->has_more_results()) { @@ -848,6 +877,8 @@ class core_backup_renderer extends plugin_renderer_base { 'type' => 'text', 'name' => restore_course_search::$VAR_SEARCH, 'value' => $component->get_search(), + 'aria-label' => get_string('searchcourses'), + 'placeholder' => get_string('searchcourses'), 'class' => 'form-control'); $output .= html_writer::empty_tag('input', $attrs); $attrs = array( @@ -870,8 +901,6 @@ class core_backup_renderer extends plugin_renderer_base { * @return string */ public function render_restore_category_search(restore_category_search $component) { - $url = $component->get_url(); - $output = html_writer::start_tag('div', array('class' => 'restore-course-search mb-1')); $output .= html_writer::start_tag('div', array('class' => 'rcs-results table-sm w-75')); @@ -887,12 +916,19 @@ class core_backup_renderer extends plugin_renderer_base { $row->attributes['class'] .= ' dimmed'; } $context = context_coursecat::instance($category->id); - $row->cells = array( - html_writer::empty_tag('input', array('type' => 'radio', 'name' => 'targetid', 'value' => $category->id)), - format_string($category->name, true, array('context' => context_coursecat::instance($category->id))), + $id = $this->make_unique_id('restore-category'); + $row->cells = [ + html_writer::empty_tag('input', ['type' => 'radio', 'name' => 'targetid', 'value' => $category->id, + 'id' => $id]), + html_writer::label( + format_string($category->name, true, ['context' => context_coursecat::instance($category->id)]), + $id, + true, + ['class' => 'd-block'] + ), format_text(file_rewrite_pluginfile_urls($category->description, 'pluginfile.php', $context->id, - 'coursecat', 'description', null), $category->descriptionformat, array('overflowdiv' => true)) - ); + 'coursecat', 'description', null), $category->descriptionformat, ['overflowdiv' => true]) + ]; $table->data[] = $row; } if ($component->has_more_results()) { @@ -918,7 +954,7 @@ class core_backup_renderer extends plugin_renderer_base { 'inform' => true, 'extraclasses' => 'rcs-search mb-3 w-25', 'inputname' => restore_category_search::$VAR_SEARCH, - 'searchstring' => get_string('search'), + 'searchstring' => get_string('searchcoursecategories'), 'query' => $component->get_search(), ]; $output .= $this->output->render_from_template('core/search_input', $data); diff --git a/lang/en/moodle.php b/lang/en/moodle.php index f5a35c18d4a..05721c7a5f0 100644 --- a/lang/en/moodle.php +++ b/lang/en/moodle.php @@ -1811,6 +1811,7 @@ $string['searchagain'] = 'Search again'; $string['searchactivities'] = 'Search for activities by name or description'; $string['searchbyemail'] = 'Search by email address'; $string['searchbyusername'] = 'Search by username'; +$string['searchcoursecategories'] = 'Search categories'; $string['searchcourses'] = 'Search courses'; $string['searchoptions'] = 'Search options'; $string['searchresults'] = 'Search results'; From 424e61d5593ac9d8058b3571dba4b7e21b6a4381 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 25 Sep 2020 23:11:32 +1000 Subject: [PATCH 2/2] MDL-69649 form: Fix labels for defaultcustom form elements --- .../templates/element-defaultcustom.mustache | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/form/templates/element-defaultcustom.mustache b/lib/form/templates/element-defaultcustom.mustache index 3c6fbda28db..af33ffc27e4 100644 --- a/lib/form/templates/element-defaultcustom.mustache +++ b/lib/form/templates/element-defaultcustom.mustache @@ -31,13 +31,18 @@ } } }} -{{< core_form/element-template }} +{{< core_form/element-group }} {{$element}} - - {{#element.elements}} - {{{separator}}} - {{{html}}} - {{/element.elements}} - +
+ {{label}} +
+ + {{#element.elements}} + {{{separator}}} + {{{html}}} + {{/element.elements}} + +
+
{{/element}} -{{/ core_form/element-template }} +{{/ core_form/element-group }}