Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue1047 wcag more semantic headings #1061

Merged
merged 19 commits into from
Sep 17, 2020

Conversation

kouralex
Copy link
Contributor

This PR adds some accessibility headings in order to address issues #1036 #1037 #1038 #1039 #1047

There are still some things left to be done. For example, this PR is missing translations for some translatable strings that were added (and some of them require a bit of tweaking anyway) and there could be better accessibility advises in many places, e.g., converting every -> (arrows) into speech.

Additionally, fixes some small issues that were introduced by #1055

@kouralex kouralex requested a review from joelit September 15, 2020 02:54
@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #1061 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1061   +/-   ##
=========================================
  Coverage     60.32%   60.32%           
  Complexity     1592     1592           
=========================================
  Files            32       32           
  Lines          4429     4429           
=========================================
  Hits           2672     2672           
  Misses         1757     1757           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ab938f...c0f9976. Read the comment docs.

Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tweaks to be done, as mentioned. This is a monumental improvement for the heading structure and makes it feasible to use the h-key with a screen reader.

@@ -97,15 +97,32 @@ $(function() { // DOCUMENT READY
});
});

var active_tab = $('li.active').attr('id');
Copy link
Contributor

@joelit joelit Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (from the next few lines onwards) seems to create a different functionality for group and hierarchy tabs when navigating the tabs with a screen reader on.

When clicking a tab, the screen reader prompts the helper text only for the alphabetical listing and for the list of new concepts. The helper text is not updated when clicking the group listing or the hierarchy listing. Tested on Firefox+orca.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a bit strange. I am unable to reproduce such behavior - maybe I do not have the right tools on my hand (testing everything with Windows Narrator + Edge). The reason why those two are handled differently is based on the fact that their contents are generated on behalf of a jsTree object, which complicates things. To fix some issues regarding this one should do a more thoroughly overhaul of that code base.

But please, as I am unable to reproduce this, I am asking you to subsequently raising on issue on this matter as I am a bit forced to merge this PR anyhow now.

view/changes.twig Outdated Show resolved Hide resolved
view/concept-shared.twig Show resolved Hide resolved
view/group-contents.twig Outdated Show resolved Hide resolved
@@ -21,7 +21,7 @@
<a class="prefLabel conceptlabel" href="{% if "isothes:ConceptGroup" in concept.type %}{{ concept.uri | link_url(concept.vocab, request.lang, 'page') }}{% elseif concept.exvocab is defined%}{{ concept.uri | link_url(concept.exvocab,concept.contentLang) }}{% else %}{{ concept.uri | link_url(concept.vocab,request.lang,'page',concept.contentLang) }}{% endif %}">{{ concept.label(request.contentLang) }}</a>{% if explicit_langcodes or request.queryparam('anylang') %}<span class="versal"> ({{ concept.contentLang }})</span>{% endif %}
{% endif %}
{% endspaceless %}
{% if concept.type == 'skosext:DeprecatedConcept' %}<h2 class="deprecated-alert">{% trans %}deprecated{% endtrans %}</h2>{% endif %}
{%- if concept.type == 'skosext:DeprecatedConcept' %}<span class="versal deprecated-alert">{% trans %}deprecated{% endtrans %}</span>{% endif -%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? I found it strange that the alert heading was downgraded to span-stag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was intentional as I wanted them to follow the correct heading structure (that is, no h2 inside h3 for example). There might be cases where this should be reverted though, I suggest raising issues as one encounters them. Anyway, the heading level should be assessed with care.

view/vocab.twig Outdated Show resolved Hide resolved
view/group-index.twig Outdated Show resolved Hide resolved
view/light.twig Outdated Show resolved Hide resolved
<li id="limit"><p>{% trans "Search options" %}</p></li>
{% endif %}
</ul>
</div>
{% if letters %}
<h4 class="sr-only">{% trans "Choose alphabetical listing letter" %}</h4>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note - it is a shame that the screen reader annotations like these only work when traversing the headers on the page with a H-key, but not when traversing clickable items with a Tab-key. However, I really think this is an issue with the screen reader, not with the Skosmos side bar's structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There are ways to make this work, e.g., using aria-labelledBy or similar, I think. One solution could be changing the element structure, too. I am not an expert on this subject so I would like to learn more of the best practices how to implement such features. One thing to consider is compatibility - ARIAs may not be fully supported by every reader?

@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kouralex
Copy link
Contributor Author

Even though there are some concerns (rightfully) raised by @joelit (btw thank you for a very throughout testing!) I feel like the easiest and especially fastest way to get things on the right tracks now is to merge this and file new issues on any defects pointed out above.

@kouralex kouralex merged commit 4379484 into master Sep 17, 2020
@osma osma added this to the 2.8 milestone Oct 6, 2020
@osma osma deleted the issue1047-wcag-more-semantic-headings branch March 11, 2021 11:18
@osma osma added the WCAG label Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants