-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
There was a problem hiding this 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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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}}"> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- 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.
- 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).
These commits have been incorporated in #43, which now supersedes this PR. Closing. |
Makes the widget fully keyboard accessible and adds screen reader support.
Closes #33