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

Player accessibility #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SimonRothUCF
Copy link
Contributor

Makes the widget fully keyboard accessible and adds screen reader support.

Closes #33

Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

Overall this is functional, nice work. Most of my feedback is about missing or unclear element roles or how the screen reader reports game state. Remember, a non-sighted user needs to be aware of what they're interacting with and why.

I mentioned it in in-line feedback below, but I think it'd be valuable to have a second live region, using the assertive property, to pipe high-priority messages to. Doing so would require defining a second function to update the value of the second live region independently of the first. So in addition to liveRegionUpdate you could have assertiveLiveRegionUpdate or liveRegionAlert to differentiate the two.

@@ -42,8 +43,8 @@ <h1>Guess the Phrase</h1>
<div>Loading...</div>
</center>
</div>
<div id="start" ng-show="!loading && !gameDone" class="start">Start</div>
<div ng-show="!loading && gameDone" class="finished"><span>FINISH</span></div>
<div id="start" ng-show="!loading && !gameDone" class="start" tabindex="0">Start</div>
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a div, it's not immediately clear to the user that they are indeed focusing a button and need to interact with it to continue. Aside from changing the actual HTML element from div to button which may cause some unintended styling or behavioral changes, you can alternatively apply role="button" to indicate its purpose. ARIA role attributes are always secondary in preference to using semantic HTML, but for this widget it's acceptable.

<div id="start" ng-show="!loading && !gameDone" class="start">Start</div>
<div ng-show="!loading && gameDone" class="finished"><span>FINISH</span></div>
<div id="start" ng-show="!loading && !gameDone" class="start" tabindex="0">Start</div>
<div ng-show="!loading && gameDone" class="finished" tabindex="0"><span>FINISH</span></div>
Copy link
Member

Choose a reason for hiding this comment

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

Same as the above. This should be given a button role to indicate its purpose as a focusable input.

@@ -52,10 +53,10 @@ <h1>Guess the Phrase</h1>
<img src="assets/img/anvil.png" aria-hidden="true" class="anvil">
</div>
<div aria-hidden="true" class="podium"></div>
<div class="title"></div>
<div class="title" tabindex="0" aria-label="{{focusTitleMessage}}"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Generally, anything in the tab sequence should have a clear indication of its purpose, and why it's focusable (as tab stops are normally tied to inputs). It might help to have a banner, or similar ARIA role, applied to this element. I might also consider prepending something to the focusTitleMessage string to indicate why the element is focusable. Maybe "Game status:" or something?

<div class="question-num">{{ curItem+1 }}</div>
<div class="total-questions"></div>
<div ng-class="{transition: inQues}" class="answer">
<div ng-class="{transition: inQues}" class="answer" tabindex="0" aria-label="{{focusAnswerMessage}}">
Copy link
Member

Choose a reason for hiding this comment

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

A thought: the focusAnswerMessage string is good here, but it's not immediately clear that the "current answer" mentioned in the string is associated with this element specifically, and why. It might help to replace "current answer" with a concise explanation of the focused element: "Your current answer is displayed here. It is currently: blank blank blank blank", that sort of thing. It might also help to append brief instructions after the current answer is relayed. "Guess another letter by pressing its corresponding key on the keyboard", etc.

@@ -82,7 +85,7 @@ <h1>Guess the Phrase</h1>
class="icon-close">
</span>
</div>
<div aria-hidden="true" class="keyboard-bg"></div>
<div aria-hidden="true" class="keyboard-bg" tabindex="0" aria-label="{{focusKeyboardMessage}}"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Be careful here, because your tabindex="0" attributes and aria-hidden="true" attributes are cancelling each other out. This element is never actually focusable because the aria-hidden attribute takes precedence. Similar to the answer element, I might also consider appending some directions after focusKeyboardMessage. "You must guess all the letters in the answer or run out of guesses before moving on to the next question", or something along those lines.


# User entered a correct guess
else
$scope.answer.guessed = Input.correct matches, input, $scope.answer.guessed
liveRegionUpdate(input + " is correct! Current answer: " + addBlanksForLiveRegion($scope.answer.guessed))
Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  1. I think providing the current answer after each letter input might be kinda information overload. The answer is always available when focusing the answer element - I might just have this report whether the selection was correct or incorrect.
  2. Entering a letter (and getting validation on whether it was correct or incorrect) is a high-priority interaction. I would consider adding a second live region with the assertive property, and having these updates (correct or incorrect) report to that assertive live region instead. Those will interrupt whatever is currently being said, and also won't repeat the way polite ones do (which is a known bug).

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.

Player accessibility
2 participants