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

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

Merged
merged 1 commit into from
Aug 19, 2016

Conversation

e-kolpakov
Copy link
Contributor

@e-kolpakov e-kolpakov commented Jul 26, 2016

Description: This PR modifies ARIA attributes so that only feedback is read to screen-reader user when an item is dropped into a zone, instead of full "working field"

JIRA: SOL-1806

Testing instructions:
0. Enable Screen Reader + make sure your browser support it (e.g. default Ubuntu screen reader does not work with Chrome, but works with Firefox).
0.a. Get used to it speaking out loud everything + learn to notice important parts of it.

  1. Create Drag and Drop v2 XBlock in a course. Default configuration should be enough.
  2. Open unit with DnDv2 in LMS.
  3. Use keyboard or mouse to drop items into correct zones. Expected behavior: only feedback is read out loud. Previous behavior: almost everything were read out loud (all/some zones and items, feedback)
  4. (Unconfirmed - see concerns below) Use keyboard or mouse to drop items into incorrect zones. Expected behavior: only feedback is read out loud. Previous behavior: almost everything were read out loud (all/some zones and items, feedback)

Sandboxes:
LMS: http://dndv2-sandbox3.opencraft.hosting/
Studio: http://studio-dndv2-sandbox3.opencraft.hosting/

Sandboxes use 07ce4db version

Reviewers

Author concerns:

Default Ubuntu screen reader seems to ignore error feedback popups (despite them being absolutely identical to normal popups). This behavior exists in current master, so I'm not sure if it is a real issue; and if so, should it be fixed in scope of this PR.

@openedx-webhooks
Copy link

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?number=85&repo=edx-solutions%2Fxblock-drag-and-drop-v2

@sstack22
Copy link

@e-kolpakov - FYI the sandbox isn't loading for me

@bradenmacdonald
Copy link
Contributor

@e-kolpakov FYI, when you list the "reviewers" in the PR description, it pings all of them - so probably better to fill that section out once our internal review is completed, and it's ready for review?

@cahrens
Copy link

cahrens commented Jul 27, 2016

Yes, I would appreciate following @bradenmacdonald's suggestion. It would make it clearer when it is ready for TNL review.

@mtyaka
Copy link
Contributor

mtyaka commented Jul 27, 2016

@e-kolpakov I've tested this with VoiceOver in Safari on OS X. I noticed a similar (the same?) problem where incorrect popups are not read out, but that only happens while the board is empty - after you place at least one item in a correct zone, the incorrect popups start working as expected.

I found that the problem was the "Reset problem" button. When you drop the first item onto the board, it shows for a fraction of a second until the item is reverted back to the bank and the button hidden. The button seem to distract VoiceOver which starts reading out "Reset problem" instead of the new contents of the popup.
I replaced display_reset_button: Object.keys(state.items).length > 0 with this code so that the button only shows when at least one item is in correct location:

      var display_reset_button = false;
      for (var key in state.items) {
        if (state.items.hasOwnProperty(key)) {
          if (!state.items[key].submitting_location) {
            display_reset_button = true;
            break;
          }
        }
      }

After I made this change incorrect popups started working fine, but now after placing the first correct item, VoiceOver would start reading out "Reset problem" instead of the contents of the correct popup. The reset button does have aria-live="off", but VoiceOver seems to ignore that for some reason. To fix this problem, I removed aria-live="polite" attribute from the section.feedback wrapper which includes the reset button, the feedback title, and the feedback content, and instead wrapped the feedback title and feedback content in an extra div with aria-live="polite".

After this change, all popups work correctly for me, except in the case where you drag the same item to two wrong locations successively (the first time you drop it into the wrong location, VoiceOver reads out "The item does not belong here", but after you drag it again and drop it on another (wrong) region, the feedback message doesn't change, so VoiceOver doesn't read it out). This would be hard to fix and since this problem was already present prior to this PR, I think we can ignore it for now.

@e-kolpakov
Copy link
Contributor Author

@mtyaka Implemented the suggested fixes; however, my SR still does not read "error" feedback. Instead it reads zone description twice. Probably a bug in Ubuntu screen reader + Firefox

h('p.popup-content', {innerHTML: ctx.popup_html}),
]
),
popupTemplate(ctx),
Copy link
Contributor

Choose a reason for hiding this comment

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

@e-kolpakov Nice, this looks much cleaner now.

@mtyaka
Copy link
Contributor

mtyaka commented Jul 27, 2016

👍

  • I tested this with VoiceOver on Safari
  • I read through the code
  • this change does not require documentation

h(
'button.reset-button.unbutton.link-button',
{ style: { display: reset_button_display }, attributes: { tabindex: 0 }, 'aria-live': 'off'},
{ style: { display: reset_button_display }, attributes: { tabindex: 0, 'aria-live': 'off' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch. I tested whether moving arial-live="polite" back to the section.feedback container makes any difference with this change, but it looks like it doesn't. VoiceOver still reads out "Reset problem" the first time it appears, even though aria-live="off" is set correctly, so moving the button out of aria-live="polite" aria as you did here still makes sense.

@staubina
Copy link
Contributor

The code changes seem straight forward, but I am having trouble recreating this on the Sandbox. Can you set up the Drag and drop problem you used for testing?

@e-kolpakov
Copy link
Contributor Author

@staubina I've updated the course with the default instance of DnDv2. Note there's a stock "Drag and Drop" CAPA problem - it is not the one we're working with here.

@cahrens
Copy link

cahrens commented Jul 28, 2016

I've read through the code and will defer to a11y review on this one.

@staubina
Copy link
Contributor

I again tried to parse through this with a screen reader, but having not used it often, I am going to also defer to accessibility on this PR.

@e-kolpakov
Copy link
Contributor Author

@cptvitamin Friendly reminder to review this PR.

@e-kolpakov
Copy link
Contributor Author

e-kolpakov commented Aug 16, 2016

@sstack22 FYI - waits for your and a11y review. This PR is about a11y as well, so I believe we should not merge it withot a11y review. Might be a good idea to include reviewing this PR and #73 to upcoming UX/RTL/a11y task.

UPD: I've added this PR to acessibility review task - do you think we can merge it now and address a11y review notes later, when they arrive?

@bradenmacdonald
Copy link
Contributor

@e-kolpakov Per @cptvitamin, this can merge without his review for now; I'm waiting to find out if @sstack22 wants to review.

Please rebase and get a green build in preparation for merge.

e-kolpakov pushed a commit to open-craft/xblock-drag-and-drop-v2 that referenced this pull request Aug 18, 2016
@bradenmacdonald
Copy link
Contributor

Rebased from 71796ad and squashed to 1cb7b9d . There were some rebase conflicts, so I did a quick post-rebase manual test of both standard + assessment mode using VoiceOver, and everything seems good. Merging now.

@bradenmacdonald bradenmacdonald merged commit ef6d2e2 into openedx:master Aug 19, 2016
@bradenmacdonald bradenmacdonald deleted the ekolpakov/sol-1806 branch August 19, 2016 01:13
haikuginger pushed a commit to open-craft/xblock-drag-and-drop-v2 that referenced this pull request Aug 25, 2016
itsjeyd pushed a commit to open-craft/xblock-drag-and-drop-v2 that referenced this pull request Aug 29, 2016
e-kolpakov pushed a commit to open-craft/xblock-drag-and-drop-v2 that referenced this pull request Aug 31, 2016
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.

7 participants