-
Notifications
You must be signed in to change notification settings - Fork 70
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
fix: set go to beginning button to hidden #408
fix: set go to beginning button to hidden #408
Conversation
Thanks for the pull request, @DanielVZ96! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Thanks for the pull request, @DanielVZ96! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate. |
a3a0299
to
3e87e57
Compare
0145cb7
to
739d074
Compare
@DanielVZ96 Thanks for these changes! I'll mark them as ready for review once OpenCraft internal review is done and they're ready for upstream review. Let me know. |
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.
I now re-read the feedback while reviewing the PR with the VO and see the following problems mentioned there:
When I am placing items in drop zones, focus is never automatically moved off any of them, once I'm done with them I can press Tab or use VO-right arrow to navigate to the Submit button.
I believe we should move the focus to the first item in the item bank once the item is placed in a drop zone. If no more items are in the item bank, we should move the focus to the submit button.
If you use VO right on the disabled go-to-beginning button, the cursor moves to the Actions group instead of the next Action
From what I understood, the problem here is the inconsistent behavior between the Tab button and the VO-right arrow. Though moving the focus directly to the action button should be a good improvement.
I think you were right about this one - the VO right should move to the next action to match the behavior of the Tab button.
Regarding the "Go to the beginning" - I think it is still useful, but maybe we could use the display: none
when it's unavailable (i.e. when there are no more attempts left in the "Assessment" mode). Also, please note that using hidden
makes the button visible in Studio, which does not seem to be intended.
This PRs covers it because it only focuses on the feedback text once it's available, and that coincides when there are no more items to add.
The problem with this is that there's no submit button for drag and drop. If the focus is moved to the submit button it may skip any other problems after the drag and drop block.
good point. will work on that! Edit: so.. after reviewing this for a bit I noticed that .hidden applies display: none. so we should fine with that last point
|
These points are incorrect for the "Assessment" mode. You can enable it in the XBlock settings (the "Edit" button) in Studio.
I'm seeing this in Studio:
A quick reminder about this note. |
@itsjeyd, I planned to merge this as a maintainer because this is an a11y fix. Will we need another review round for this? |
@Agrendalath Ah no, it should be fine to merge without a second review. Sorry for the confusion. CC @DanielVZ96 |
739d074
to
d50b3b9
Compare
@Agrendalath I updated the PR with the changes you requested! |
@DanielVZ96, the VO right still moves the focus to the action wrapper instead of the first action button. This is inconsistent with the Tab behavior. |
@Agrendalath I'm actually not that sure about that one. It's due to the role=group and aria label of the actions wrapper div. I'm thinking that maybe that's there for a reason. |
We added this in #280 to match the HTML nodes in the edx-platform's Problem XBlock. This is no longer the case, so we can remove it. |
@Agrendalath done! |
@DanielVZ96, thank you; the changes look good to me. I'll wait for the client's final QA before merging this. |
@DanielVZ96 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
1 similar comment
@DanielVZ96 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@itsjeyd, does the GH bot automatically merge the "ready to merge" PRs even when they don't have official approval (green checkmark)? It's fine in this case, but it would be nice to squash the commits first. |
@Agrendalath Good question, I'm not sure if it's supposed to 🤔 After reading your comment above it definitely wasn't my intention to trigger a merge 😅 If you're coming to the maintenance working group meeting later, maybe bring up the question there? |
Description
This PR addresses a few accessibility issues when using the xblock with keyboard and Voice Over controls.
When submitting the problem:
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
(Non-Assessment)
(Assessment)
Private-ref: https://tasks.opencraft.com/browse/BB-8848