report: add aria-labels to tools button and metric toggle #9201
Conversation
| const metricAuditsEl = this.renderAuditGroup(groups.metrics); | ||
|
|
||
| // Metric descriptions toggle. | ||
| const toggleTmpl = this.dom.cloneTemplate('#tmpl-lh-metrics-toggle', this.templateContext); | ||
| const toggleEl = this.dom.find('.lh-metrics-toggle', toggleTmpl); | ||
| metricAuditsEl.prepend(...toggleEl.childNodes); | ||
| metricAuditsEl.append(...toggleEl.childNodes); |
There was a problem hiding this comment.
prepend() made the screen reader version of the category really confusing.
Maybe prepending was done because the code reads at first glance like the toggle needs to be prepended to come before the rendered audits, but renderAuditGroup() only renders the container and the group header, not the audits themselves, so the toggle should be the last child of metricAuditsEl at this point.
...or maybe the code just changed over time and this was a holdover of a different version :) Moved the metricAudits filter statement to try to make the order of things clearer, regardless.
| @@ -366,7 +366,7 @@ | |||
| <a href="" class="lh-topbar__url" target="_blank" rel="noopener"></a> | |||
|
|
|||
| <div class="lh-tools"> | |||
| <button class="report-icon report-icon--share lh-tools__button" title="Export report"> | |||
| <button class="report-icon report-icon--share lh-tools__button" title="Tools menu" aria-label="Toggle report tools menu"> | |||
There was a problem hiding this comment.
at what point does i18n creep into here? :)
There was a problem hiding this comment.
at what point does i18n creep into here? :)
😬
(probably should have a while ago)
There was a problem hiding this comment.
this be the point: #9166
Two easy fixes from aXe. Neither of these were getting good labels in a screen reader (including the actual ` for the metrics toggle, which had no text).
There was also a warning about tables with column headers but only empty cells in the column (https://dequeuniversity.com/rules/axe/3.2/th-has-data-cells), but from discussions I've found out there, AFAICT screen readers actually handle that case pretty well (e.g. VoiceOver says "empty"), so I don't think that actually needs fixing in the report.