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

Fixes Accessibility issues #428

Closed
wants to merge 9 commits into from
Closed

Conversation

funkonaut
Copy link
Contributor

There are some issues in the web interface that affect non-visual access to the web page. My pull request attempts to solve them by labeling form fields for screen readers to announce the input elements and making clickable headings keyboard navigable. There is some redundancy with aria-labels as well to improve access by announcing the help text as well as the name when form fields are focused.

@florianfesti
Copy link
Owner

florianfesti commented Sep 2, 2022

I am all for improving the accessibility. But my HTML foo is barely enough to make the web UI work. So having more detailed explanation what tags/attributes are added and why is really needed in the commit messages. It might also help to compile a list in #427 or here to what needs changing.

@HaSHsss
Copy link
Contributor

HaSHsss commented Sep 4, 2022

This is my income. The proposed corrections are made to comply with the requirements of HTML 1.1.
But they are too superficial, and will not bring real results.

You also need to consider micro markup, structuring, and much more.

@HaSHsss
Copy link
Contributor

HaSHsss commented Sep 4, 2022

I do not want to devalue the work of a person, this is part of the work worth doing, but not at all like that.

@funkonaut
Copy link
Contributor Author

@HaSHsss my apologies I am trying to get the webpage to a point where my students who use screen readers can navigate it as well as my sighted students. Testing my PR with with NVDA, JAWS, VoiceOver, and Windows Narrator all yielded results that
allowed for non visual navigation and use of the web page, I am not sure why you say they would not bring real results?

I would love to learn more about what specifically I did incorrectly, if you could link any resources or sample code I would be happy to update my PR to provide a more robust solution.

@florianfesti Adding specifics in #427 and updating PR to contain more detail commit messages. Will report back here once I finish up the edits.

@@ -241,8 +241,9 @@ class BServer:
continue
if len(group._group_actions) == 1 and isinstance(group._group_actions[0], argparse._HelpAction):
continue
prefix = getattr(group, "prefix", None)
result.append('''<h3 id="h-%s" class="open" onclick="showHide(%s)">%s</h3>\n<table role="presentation" id="%s">\n''' % (groupid, groupid, _(group.title), groupid))
prefix = getattr(group, "prefix", None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds event handler for keyboard enter press and tab index to allow for tab navigation on clickable headings and a more device independent event handler.

@@ -341,7 +342,7 @@ class BServer:
""" ]
for nr, group in enumerate(self.groups):
result.append(f'''
<h3 id="h-{nr}" class="open" onclick="showHide('{nr}')"
<h3 id="h-{nr}" class="open" tabindex="0" onclick="showHide('{nr}')" onkeypress="if(event.keyCode == 13) showHide('{nr}')"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds event handler for keyboard enter press and tab index to allow for tab navigation on clickable headings and a more device independent event handler.

@@ -242,10 +242,10 @@ class BServer:
if len(group._group_actions) == 1 and isinstance(group._group_actions[0], argparse._HelpAction):
continue
prefix = getattr(group, "prefix", None)
result.append('''<h3 id="h-%s" class="open" onclick="showHide(%s)">%s</h3>\n<table id="%s">\n''' % (groupid, groupid, _(group.title), groupid))
result.append('''<h3 id="h-%s" class="open" onclick="showHide(%s)">%s</h3>\n<table role="presentation" id="%s">\n''' % (groupid, groupid, _(group.title), groupid))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds presentation view to layout table. Probably not the best fix as according to W3 WAI best practice is using CSS for visual styling, but gets it more screen reader friendly without requiring a large rewrite of the web ui.

for a in group._group_actions:
if a.dest in ("input", "output"):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This newline doesn't need to be integrated my bad...

@HaSHsss
Copy link
Contributor

HaSHsss commented Sep 4, 2022

I'm sorry, but I did not take into account such a circumstance with the blind.
The fixes you suggested are recommendations for structuring HTML 1.1. In any case, I still think the fixes you suggested are very superficial and we could do better together.
I thought you recommended these Google SEO fixes.
Could you tell us in more detail what functionality your students use.
How would you like to navigate.
I understand how screen reading works, but I've never used it myself.

@HaSHsss
Copy link
Contributor

HaSHsss commented Sep 4, 2022

You simply add labelsfor elements so that they can be voiced by means of the system.
But I suspect that since the structure is not explicitly declared, there will still be problems with quick navigation.

@florianfesti
Copy link
Owner

OK, as there is a whole list of issues lets talk about them one by one.

The patch for headings looks pretty good already and works just fine. What would we need to add to announce that the section is closed or expanded. We can easily add something in the showHide() function or in the self.css sections about h3.

@florianfesti
Copy link
Owner

I am open to replace the tables by proper CSS layout. May be someone can give me a hint on what to try first. Going with the role="presentation" hack is still a lot quicker and what we should do for now. The larger changes can then wait a bit.

@florianfesti
Copy link
Owner

OK, I pushed the later two patches with a bit expanded commit messages. Which mainly leaves the labeling issue. The thing is that these input fields are "labeled" by the text in front of them. I wonder if there is a way to make that accessible to the screen reader users.

@funkonaut
Copy link
Contributor Author

Yeah the commit that wasn't pushed does that with a labelfor tag and id tags, just realized I forget to add it in one place so updating the PR to include everything. Adding comments too.

@@ -45,7 +45,6 @@ class FileChecker(threading.Thread):
super(FileChecker, self).__init__()
self.checkmodules = checkmodules
self.timestamps = {}
self._stopped = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I guess I did not integrate your most recent changes sorry about that :/

@@ -198,7 +198,7 @@ def html(self, name, default, translate):
("""<option value="%s"%s>%s</option>""" %
(e, ' selected="selected"' if e == default else "",
translate("%s %s" % (e, self.names.get(e, "")))) for e in self.edges))
return """<select name="%s" size="1">\n%s</select>\n""" % (name, options) #here
return """<select name="%s" id="%s" aria-labeledby="%s %s" size="1">\n%s</select>\n""" % (name, name, name+"_id", name+"_description", options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this adds and id and aria-labeled by that references the id, name, and description from the boxesserver file this allows for screen readers to announce the label to the left of the input and the description to the right of the input for the combo box

@@ -219,7 +219,7 @@ def html(self, name, default, _):
default = self(default)
return """<input name="%s" type="hidden" value="0">
<input name="%s" id="%s" aria-labeledby="%s %s" type="checkbox" value="1"%s>""" % \
(name, name, name, name+"_id", name+"_description",' checked="checked"' if default else "") #fix here
(name, name, name, name+"_id", name+"_description",' checked="checked"' if default else "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

got rid of the comment

get rid of comment
<input name="%s" type="checkbox" value="1"%s>""" % \
(name, name, ' checked="checked"' if default else "")
<input name="%s" id="%s" aria-labeledby="%s %s" type="checkbox" value="1"%s>""" % \
(name, name, name, name+"_id", name+"_description",' checked="checked"' if default else "") #fix here
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adds associated of label to the left of the input box and description to the right to the input box so screen readers will announce items it does this with an id tag and aria-labeledby tags that reference labelfor tags and id tags there is some redundancy with the aria-labeledby tag referencing the same tag as the label for tag (ie name_id) this is to provide robustness across different screen readers.

@funkonaut
Copy link
Contributor Author

funkonaut commented Sep 5, 2022

All that is left is to get the web page announcing when the clickable headings are expanded and collapsed, usually would use a menu or button element and the aria-expanded tag that would be updated in the showHide function but since they are headings with device handlers we will have to go a different route... Going to experiment with a couple of ideas tomorrow

Role tag added as aria-expanded does not work for clickable headings, this is some what of a work around but will get things accessible for now wihtout requiring too much of a rewrite.  The showHide function now has lines to toggle the aria-expanded tag based on the state of the clickable heading.
keeps initial state of aria-expanded consistent with class
@florianfesti
Copy link
Owner

OK, looks like this should fix all known issues. I will rebase this into two nice commits and add a bit of explanation to the commit message. Or is there anything I should still wait for?

@florianfesti
Copy link
Owner

OK, merged as 691f3f4 and 478d73f

Thanks for all the work!

@funkonaut
Copy link
Contributor Author

funkonaut commented Sep 8, 2022

Awesome thank you so much, my students and I are very much looking forward to the changes going live on festi.info/boxes.py/ !

@florianfesti
Copy link
Owner

Another issue with accessibility is of course the often poor descriptions of the generators (and the parameters). Many generators rely on a sample image to give an idea what they are producing and some are even lacking that.

See #25 and #26
#140 is linked from generators that still lack an image. But those won't help screen reader users a whole lot.

If you or your students want to add descriptions they can be just added as comments in those tickets. PRs are ofc also welcome.

@florianfesti
Copy link
Owner

@funkonaut Can you and your students have a look at the new search functionality on the generator selection page, please. I am pretty sure it will need some tweaking to work well with screen readers.

Not sure if it's worth bothering with the gallery (link in the footer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants