Drag and Drop v2 - Review for release on edx.org

[bradenmacdonald]

This is the main PR for the SOL-1403 DnDv2 on edx.org epic.

Recent reviews have taken place on the following constituent PRs:

• Allow authors to set background and text color of draggable items #33 Allow authors to set background and text color of draggable items: SOL-1406, SOL-1409
• Add License (AGPLv3) #35 Add License (AGPLv3): SOL-1420
• Move draggable items above drop image area #38 Move draggable items above drop image area: SOL-1404
• Make the image resize to fit the container, make item placement relative open-craft/xblock-drag-and-drop-v2#4 Make the image resize to fit the container: SOL-1450, Allow problem reset after the first interaction: SOL-1408
• Support for multiple DnD blocks on the same page #39 Support for multiple DnD blocks on the same page: SOL-1448
• Styling changes from ux #40 Styling changes from UX review: SOL-1410
• Improve default settings for DnDv2 blocks  open-craft/xblock-drag-and-drop-v2#5 Improve default settings and explain settings more clearly: SOL-1451
• Make DnDv2 accessible #41 Make DnDv2 accessible: SOL-1412
• Ajax loader spinner #42 DnDv2 - Show an indicator/spinner while AJAX requests are in progress: SOL-1452
• Make DnDv2 keyboard-accessible #44 DnDv2 - Make the block keyboard navigable: SOL-1462
• Add a checkbox to the zone tab to control visibility of zone borders.… #45 DnDv2 - Allow authors to show the borders on drop zones: SOL-1405
• Flaky tests fix #46 Fix flaky tests: SOL-1449
• Update and refine analytics events and their documentation. #47 Update and refine analytics events and their documentation: SOL-1473
• Make DnDv2 themable #48 Add theming: SOL-1411
• Update screenshots #52 Re-create all of the documentation screenshots: SOL-1453
• Mobile support: Improve draggable width options for more flexible authoring #49 Improve usability on mobile: SOL-1474

TODO (during review):

• a11y: Check if the grey color of the focus outline on the drop targets is OK, or if there's a better way to make it visible against varied backgrounds
• a11y: Check if the grey dotted borders around drop zones when authors turn on "show drop zones borders" are OK

Testing

We have a sandbox available for testing DnDv2: Studio, LMS.

Reviewers

• a11y: @cptvitamin
• UX: @talbs
• Strings: @pdesjardins
• TNL: @dianakhuang

[Kelketek]

@clytwynec I've pushed changes to make the links into buttons.

Yes, you can specify in the studio alt text for draggable images.

[clytwynec]

@Kelketek Thanks for making those updates. I had a chance to review this a bit more this afternoon and found a couple other issues.

• color contrast is too low on items that have been placed correctly in the drop zones.
• keyboard help/reset problem buttons have no focus indicator in Firefox.
• Multiple drag-and-drop problems on a page results in duplicate IDs (‘#-zone-1’, etc.). There shouldn’t be more than one instance of an ID on a page.
• Mark mentioned before that role=“application” needed to go around the “draggable” region. It looks like it also needs to the container with the drop zones also needs to have role=“application”, or instead put it on the section.drag-container, since that contains both the draggable container and drop zone container. I was able to pick up an item, but the keystrokes for dropping an item in the drop zones were being intercepted by screen readers. (Once the role="application" was added, this work really well with the windows screenreaders.)
• Some issues that only seem to be happening in voiceover:
  • The drop zones aren’t being recognized as focusable by voiceover. I’m not sure why. For now, I don’t know how you can fix this, but I wanted to mention that the issue exists.
  • When dropping an item into a drop zone, I am unable to access the feedback (the black/red boxes in the triangle example) using voiceover on mac. aria-live regions that are loaded after the initial DOM load don’t always work in voiceover/safari. Since this seems to be an issue with the way a specific screenreader interprets valid markup, I’m not sure it is necessary to put in a workaround. However, one possible solution is to send focus to the feedback element instead of having it work as an aria-live region.

[Kelketek]

@clytwynec I think I've hit everything you've listed there now. Please take another look.

[clytwynec]

@Kelketek Those changes look good to me.

• With multiple problems on a page, there are still some duplicate IDs. The one I see now is #item-n-description in the case where both problems have their nth item correctly dropped in a target. Can you take a look at any IDs that are being used in this block and make sure there isn’t a case where there could be multiple of the same ID on the page?
• I think the individual drop zones also need to have role="button" (much like the draggables). Without it the screenreader isn’t providing useful context to the user.

[cahrens]

To find simple issues like multiple IDs or color contrast, I HIGHLY recommend that you add an accessibility bok choy test. It's super easy and useful-- all you have to do is navigate to a courseware page with 2 drag and drop xblocks on it and call a single method!

[Kelketek]

@cahrens In this case it'd require a few extra steps, since those IDs don't get created until some interaction happens. As we're short on time I'd like to avoid introducing extra variables.

However I'm curious about this technique. Is there documentation for it? I'd like to use it in the future if I can.

@clytwynec Please take a look at the latest updates and see if they perform as you expect.

[cahrens]

@Kelketek See https://openedx.atlassian.net/wiki/display/TE/Automated+Accessibility+Tests#AutomatedAccessibilityTests-WritinganAccessibilityTest and/or search in the edx-platform acceptance test directory for "check_for_accessibility_errors".

@clytwynec is the one who introduced the functionality, and the TNL team has found it very useful and simple to write.

[clytwynec]

@Kelketek

When dropping an item into a drop zone, I am unable to access the feedback (the black/red boxes in the triangle example) using voiceover on mac. aria-live regions that are loaded after the initial DOM load don’t always work in voiceover/safari. Since this seems to be an issue with the way a specific screenreader interprets valid markup, I’m not sure it is necessary to put in a workaround. However, one possible solution is to send focus to the feedback element instead of having it work as an aria-live region.

I think you made the change I suggested here, but it seems to have made the feedback regions no longer work in windows screenreaders and not to have fixed the issue in safari/voicover. At this point, it seems better to leave it as it was (just using the aria-live region, not sending focus). It will leave it without the feedback read aloud in voiceover without navigating to it, but allow it to work properly elsewhere.

I think there is still one outstanding question that Mark had.

Is there a specific use case for making the display of the title optional (checkbox in studio)? [...]

We recently added this functionality for one of our clients, cf. #32, so we can't remove it completely (at least not without having to maintain a separate version of DnDv2 that includes it). Please let us know if you have any ideas on how to discourage course authors from making use of this feature more actively.

I'm still not sure I understand what the use case for excluding the heading would be. What was the clients reason for not wanting it included?

I think they just thought it was ugly or redundant in some layouts. @antoviaque can you give some insight here?

Otherwise this is working really well.

[antoviaque]

I'm still not sure I understand what the use case for excluding the heading would be. What was the clients reason for not wanting it included?
I think they just thought it was ugly or redundant in some layouts. @antoviaque can you give some insight here?

@clytwynec Sorry that I had missed the question before - the reason they wanted the option to hide the title is that they sometimes want to be able to make titles themselves, with a different heading/formatting than the default one from the XBlock.

[Kelketek]

@clytwynec I've reverted the changes to the feedback popup. Could you take another look?

[clytwynec]

@Kelketek This looks good to me except that the popup feedback still isn't being recognized by any screenreaders. I'm out of ideas for how to get it working, since it appears to have the right markup and, looking at the code, looks like it is being rendered at the same time as other aria-live regions that are working. Since @cptvitamin is out until Monday, @clrux do you have any ideas what might be going on here?

[downzer0]

@Kelketek Remove aria-live from the injected popup and instead place it on the parent (target) like so:

<div class="target" aria-live="polite" aria-atomic="true" aria-relevant="additions">

[Kelketek]

@clrux @clytwynec Fixed.

[clytwynec]

@Kelketek That works great! Thanks for making these changes.

[Kelketek]

@clytwynec Could I get your explicit +1 on this?

[clytwynec]

👍