Adding usability for keyboard controls (SOL-1807) #73

dovinmu · 2016-06-23T18:10:50Z

Edit: this pull request is covered by the OpenCraft contributor agreement. @antoviaque

This should add in the following functionality:

'When tabbing through the focusable controls, the Windows user is never exposed to the alt text for the background image, which is critical to understanding the context of the problem. I think a reasonable solution would be to add an ID to this element and then reference it via aria-describedby on all the dropzones. The screen reader user would move focus to the dropzone, hear its description, and then hear the description of its meaning in the greater context of the problem.'
'In certain screen readers, the keyboard controls are not exposed. On Windows and NVDA< only the CTRL + M keycombo works to drag and drop the items (many users will not be aware of this combination). It would be very cool to add an additional aria-describedby that references that bit of help text that is available in the Keyboard help modal: "Press "Enter", "Space", "Ctrl-m", or "⌘-m" on an item to select it for dropping, then navigate to the zone you want to drop it on." on the draggable items and "Press "Enter", "Space", "Ctrl-m", or "⌘-m" to drop the item on the current zone." on the dropzones.'
'when I went to a dropzone that had an item in it, it was not read back to me, so I had no idea if a dropzone had any items in it or not.'

Sandbox URLs

Studio: http://studio-dndv2-sandbox7.opencraft.hosting/
LMS: http://dndv2-sandbox7.opencraft.hosting/

The sandbox is at version de47111

openedx-webhooks · 2016-06-23T18:11:04Z

Thanks for the pull request, @dovinmu! I've created OSPR-1308 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

nedbat · 2016-06-23T18:31:38Z

@dovinmu Thanks! We'll need a contributor agreement from you, or let us know if you are covered by someone else's.

dovinmu · 2016-06-23T19:23:52Z

@nedbat Okay, just sent it in.

antoviaque · 2016-06-26T13:00:04Z

@dovinmu Could you re-read the instructions I had given you for the contributor agreement and opening the PR? You haven't followed them - could you do that now?

@nedbat Apologies for the confusion - this is a recruitment task, and @dovinmu would normally be covered by the OpenCraft agreement. I still need a signature from him though before it could actually be the case - so if you have already got his contributor agreement, we can just have him covered by the individual contributor agreement in this case, and only switch to ours if he is hired.

dovinmu · 2016-06-26T20:01:02Z

@antoviaque Sorry for the confusion. You linked for me to post a pull request to edx-platform, but I don't think I can do that since this is the repo I've worked in. I don't see how it would make sense to merge this code into the edx-platform code, but is there something that I'm missing?

I'll edit the post to reflect the fact that this is under OpenCraft (and you should have my form now).

antoviaque · 2016-06-27T08:00:17Z

@dovinmu Thanks! And no worries. For the edx-platform PR, you will eventually need one there to update the requirements: https://github.com/edx/edx-platform/blob/35267e47774d939c8858782a56c6e3ea7a7574e5/requirements/edx/github.txt#L98 - but yes, for now the PR is on this repository, sorry if that wasn't clear. The requirements for the contributor agreement are the same on this repository and edx-platform.

nedbat · 2016-06-27T17:07:06Z

@antoviaque @dovinmu We have Rowan's individual signed agreement, so we are good from a legal standpoint.

dovinmu · 2016-07-06T22:04:34Z

@antoviaque @nedbat Any estimate for when this could be reviewed? Anything else I can do to help it along?

antoviaque · 2016-07-07T06:48:09Z

@dovinmu Thank you for the ping - since it's DnDv2, I would imagine OpenCraft would do at least one of the reviews. @bradenmacdonald Did https://openedx.atlassian.net/browse/OSPR-1308 came up in the sprint scheduling for the DnDv2 work? If not can we schedule a review in an upcoming sprint?

e-kolpakov · 2016-07-07T11:52:32Z

@dovinmu there are some code quality issues that failed CI build (that red cross with "continuous-integration/travis-ci/pr — The Travis CI build failed" down at the bottom :)) - at the very minimum you should resolve them.

Hint: clicking on Details next to that "Travis CI build failed" should bring you to Travis CI build page - there are some useful insights on what needs to be fixed.

e-kolpakov · 2016-07-07T12:00:35Z

tests/integration/test_interaction.py

@@ -284,6 +295,62 @@ def get_locations():
            self.assertDictEqual(locations_after_reset[item_key], initial_locations[item_key])
            self.assert_reverted_item(item_key)

+    def alt_text_image_displayed(self, items_map):
+        #Verify that the alt text for the underlying image appears when the user navigates to it with the keyboard.


@dovinmu this comment better be a docstring

bradenmacdonald · 2016-07-07T18:40:36Z

@antoviaque FYI, the original ticket was SOL-1807, and we have OC-1668 to track it.

antoviaque · 2016-07-08T06:50:11Z

@bradenmacdonald Perfect, thanks - and to @e-kolpakov for the review.

dovinmu · 2016-07-08T20:20:19Z

tests/integration/test_interaction.py

-        popup = self._get_popup()
-        feedback_popup_content = self._get_popup_content()
+        self._get_popup()
+        self._get_popup_content()


There's a strange situation where this code is either failing the run_tests.py (for an aria-describedby being None) or pylint (for unused variables) tests, while the only thing I'm changing is this. I'm looking into the aria-describedby being none, but any insight as to why changing whether I'm assigning these objects to variables might affect things?

@dovinmu That assignment (or not) should not have any effect, I agree. Some tests are just flaky, which means that they occasionally randomly fail due to some factors like timing, network connection speed, etc.

e-kolpakov · 2016-07-11T09:32:11Z

@dovinmu almost there - just fix the last issue, squash commits, make sure the CI build is green and it will be good to go.

dovinmu · 2016-07-11T19:37:56Z

@e-kolpakov All right, everything looks good. Want me to pull the recent changes so this pull request is up-to-date?

e-kolpakov · 2016-07-12T09:49:17Z

@dovinmu This branch does not have conflicts, so rebasing on top of latest master is optional.

However, there're two small outstanding issues about - I've just commented on those threads again. Sorry for not spotting it earlier.

e-kolpakov · 2016-07-14T07:12:36Z

@dovinmu 👍

@bradenmacdonald No upstream reviewer assigned as far as I can tell.

bradenmacdonald · 2016-07-14T16:42:17Z

@cptvitamin this is one of the dndv2.1 stories, ready for your review. Can you please take a look?

@nedbat FYI, this is a Solutions project, SOL-1807, though OpenCraft is covering the time since we're using it as a recruitment test task.

dovinmu · 2016-07-20T16:20:55Z

Hey @cptvitamin, I think this is still waiting on your review.

bradenmacdonald · 2016-07-21T20:37:10Z

@dovinmu This will also need a rebase.

dovinmu · 2016-07-25T18:13:47Z

@bradenmacdonald Okay, should be good now.

cptvitamin · 2016-07-28T15:36:24Z

drag_and_drop_v2/public/js/drag_and_drop.js

@@ -45,7 +45,8 @@ function DragNDropTemplates(url_name) {
            'draggable': !item.drag_disabled,
            'aria-grabbed': item.grabbed,
            'data-value': item.value,
-            'data-drag-disabled': item.drag_disabled
+            'data-drag-disabled': item.drag_disabled,
+            'aria-describedby': gettext('Press "Enter", "Space", "Ctrl-m", or "⌘-m" on an item to select it for dropping, then navigate to the zone you want to drop it on.'),


@dovinmu aria-describedby takes an ID value, not a string. My suggestion was to add a unique ID attribute to this piece of text (already exists in the Keyboard help modal) and reference that ID here, via aria-describedby.

Ah, that's true. I'm looking into this.

@cptvitamin So currently there is a describedby for when the item is correctly placed. So then I assume we'd want the default describedby to be present in all other cases, right? Doing that works as long as I change a few lines in assert_reverted_item() in tests_integration.py.

@dovinmu yeah, that makes sense to me. aria-describedby can take a space separated list of IDrefs, so you might be able to just add/remove IDrefs as necessary. They are read to the user in order, so I think the keyboard usage should be at the end, that way they get the most relevant information first (after a few uses they will be aware of how to use with a keyboard.) Make sense?

@cptvitamin What's the advantage of doing it that way, versus just making an sr div for the describedby as needed (i.e. only put the "Correctly placed" message in if the item has been placed)?

@dovinmu I'm not actually sure there is one, just wanted you to be aware of that technique in case it was a viable solution. I don't want to make any suggestions for implementation techniques until some manual testing is performed on this. Much of accessibility is pretty straightforward, but we are getting into an area that has very poor implementation support, so a lot of testing and revision work is required. I'm hoping to find the time to take a close look at this soon. I don't want to test this though until we at least get an IDref in there as the value of that aria-describedby attribute.

@cptvitamin Alright, the latest code should have fixed that ID issue now.

cptvitamin · 2016-07-28T15:38:01Z

@dovinmu thanks for doing this work. I'm ready to review this and eager to get it into the platform. Can we get a sandbox up and running for this? Thanks.

bradenmacdonald · 2016-07-28T17:49:58Z

@itsjeyd Can you please set up a sandbox for this, for @dovinmu ?

bradenmacdonald · 2016-07-28T17:51:21Z

Sorry for the lack of a sandbox @cptvitamin; this PR was done as a recruitment test task so isn't following the same process as the rest of this epic, and that was an oversight on my part.

itsjeyd · 2016-07-29T12:40:34Z

@cptvitamin The sandbox is up and running, see the PR description for Studio and LMS URLs. I put OpenCraft's default demo course for DnDv2 on it for testing.

CC @bradenmacdonald @dovinmu

e-kolpakov · 2016-08-16T07:37:36Z

@bradenmacdonald @itsjeyd Looks like this PR is a bit stuck - is there anything we could do about it? I believe this is all about a11y, but a11y team is busy.

CC @sstack22 - FYI

bradenmacdonald · 2016-08-17T17:52:20Z

@e-kolpakov, Mark has asked us to merge this and allow him to do a final combined a11y review.

As the reviewer, can you please quickly check that this PR is going to merge OK with the latest assessment mode code, confirm that you have manually tested (with keyboard controls and a screen reader), ping TNL team for a second [set of] +1, then merge?

bradenmacdonald · 2016-08-19T01:18:59Z

@dovinmu Sorry, but this needs a rebase again, and we don't have the ability to rebase it since it's on your GitHub branch. Can you please rebase it, then we'll do a final review+test by another member of our team and merge? Thanks so much for your continued work on this PR.

dovinmu · 2016-08-19T17:13:50Z

@bradenmacdonald Looks like one of the test modules is now too many lines, probably too many to edit down with minor changes. What would you suggest?

bradenmacdonald · 2016-08-19T17:17:50Z

@dovinmu Normally, I would say just to move some of the tests in that module to a different module; but because we have several open PRs right now, that's going to create tons of merge conflicts and rebasing for everyone. So maybe just add a # pylint: disable=too-many-lines to the top of that file for now.

dovinmu · 2016-08-20T05:20:39Z

@bradenmacdonald Okay, third time's the charm then hopefully :)

bradenmacdonald · 2016-08-23T01:00:59Z

Did a quick test + final review; added 06871d9 to fix some duplicate HTML IDs (part of the ID that was supposed to be unique was undefined) and remove a time.sleep() from the tests.

The keyboard control help seems a bit awkward since there's no pause between the item text and the keyboard help. I quickly tried what @cptvitamin had suggested: using an aria-describedby pointer to the ID of the li text in the keyboard help dialog instead of including the help within the element; however, when I tried that with VoiceOver/Chrome, that help text then never got read at all. So I think this is reasonable for now and any follow-up will be done in the comprehensive a11y review we have planned.

Merging now.

cptvitamin · 2016-08-25T21:22:04Z

@bradenmacdonald just getting caught up on a bunch of mentions I haven't gotten to. I wanted to let you know that you should never test Voiceover with Chrome. You will get unexpected and buggy results. Because Apple has a proprietary accessibility API, it Voiceover only works with Safari.

bradenmacdonald · 2016-08-25T21:49:36Z

@cptvitamin Ah, that explains it :) Thanks.

e-kolpakov reviewed Jul 7, 2016
View reviewed changes

dovinmu reviewed Jul 8, 2016
View reviewed changes

cptvitamin reviewed Jul 28, 2016
View reviewed changes

e-kolpakov mentioned this pull request Aug 16, 2016

[SOL-1806] Moved aria-live attributes to popup wrapper #85

Merged

4 tasks

Changes to usability for keyboard controls (SOL-1807)

de47111

bradenmacdonald merged commit de47111 into openedx:master Aug 23, 2016

bradenmacdonald added a commit that referenced this pull request Aug 23, 2016

Merge pull request #73 Add Usability for Keyboard Controls

6b6574e

Adding usability for keyboard controls (SOL-1807) #73

Adding usability for keyboard controls (SOL-1807) #73

Conversation

dovinmu commented Jun 23, 2016 • edited by bradenmacdonald Loading

openedx-webhooks commented Jun 23, 2016

nedbat commented Jun 23, 2016

dovinmu commented Jun 23, 2016

antoviaque commented Jun 26, 2016 • edited Loading

dovinmu commented Jun 26, 2016

antoviaque commented Jun 27, 2016

nedbat commented Jun 27, 2016

dovinmu commented Jul 6, 2016

antoviaque commented Jul 7, 2016

e-kolpakov commented Jul 7, 2016

Choose a reason for hiding this comment

bradenmacdonald commented Jul 7, 2016

antoviaque commented Jul 8, 2016

dovinmu Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-kolpakov commented Jul 11, 2016

dovinmu commented Jul 11, 2016

e-kolpakov commented Jul 12, 2016

e-kolpakov commented Jul 14, 2016

bradenmacdonald commented Jul 14, 2016

dovinmu commented Jul 20, 2016

bradenmacdonald commented Jul 21, 2016

dovinmu commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cptvitamin commented Jul 28, 2016

bradenmacdonald commented Jul 28, 2016

bradenmacdonald commented Jul 28, 2016

itsjeyd commented Jul 29, 2016

e-kolpakov commented Aug 16, 2016

bradenmacdonald commented Aug 17, 2016

bradenmacdonald commented Aug 19, 2016

dovinmu commented Aug 19, 2016

bradenmacdonald commented Aug 19, 2016

dovinmu commented Aug 20, 2016

bradenmacdonald commented Aug 23, 2016

cptvitamin commented Aug 25, 2016

bradenmacdonald commented Aug 25, 2016

dovinmu commented Jun 23, 2016 •

edited by bradenmacdonald

Loading

antoviaque commented Jun 26, 2016 •

edited

Loading

dovinmu Jul 8, 2016 •

edited

Loading