search

reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.37k stars 808 forks source link

VoiceOver fails on aria-modal="true" #611

Open MaksimKniazeu opened 6 years ago

MaksimKniazeu commented 6 years ago

Summary:

In current moment dialogs that wrap up with aria-modal perfectly lock NVDA user from navigate outside a dialog but for VoiceOver users it is make unreachable some static content.

Steps to reproduce:

  1. Apply react-modal approach
  2. Fill mentioned dialog with static text in 3

    tag

  3. Try to reach it in VO

Expected behavior:

Content is reachable

Additional notes:

https://bugs.webkit.org/show_bug.cgi?id=174667

Suggestion:

Remove aria-modal till webkit bug will resolve

diasbruno commented 6 years ago

Hi @MaksimKniazeu, the webkit nightly build is affected (don't know if this gets to the end-user). So I don't know if we need to take action now. Can you please post more information about this?

If we have to do something we can make a hack to check for iPad and iPhone and disable only for this devices.

MaksimKniazeu commented 6 years ago

Hi @diasbruno, initially I faced with this issue on macOS (safari stable + voiceOver) about 6 month ago, hopefully, react-modal allows me to set aria attributes that I find necessary. In current moment we switch to 2.4.1 version. Want to highlight, that in NVDA it`s work perfectly, but MAC users blocked.

I act as a accessibility developer for 3 years and realize that best approach for dialogs that works everywhere and make it really modal for screen reader is set aria-hidden="true" for all content except dialog and his parent DOM chein when dialog is show.

leonascimento commented 6 years ago

@MaksimKniazeu can you confirm if this bug happens just on iPad and iPhone?

@diasbruno @MaksimKniazeu can you make a code review please?

MaksimKniazeu commented 6 years ago

@leonascimento false, bug is true for macOS too. So devices such as macBook, macMini, iMac etc included. Therefore you need to extend your code for "Mac OS" condition (see comment for your commit).

diasbruno commented 6 years ago

We had some problems using aria-hidden (not sure if it was confusing or just a wrong a implementation).

I'll need to have a look on the specs for those attributes (aria-modal and aria-hidden), but if you can elaborate/advise/plan a better implementation, I could help writing a patch to improve our a11y.

leonascimento commented 6 years ago

Hi guys, @diasbruno @MaksimKniazeu. I figure that we need to remove this aria-modal just to this specific version that happens it. I'm thinking that we apply this patch to all webkit it is not right.

MaksimKniazeu commented 6 years ago

Hi @leonascimento as my testing show, all current versions of webkit and version below deal bad with aria-modal. So it is OK to remove aria-modal for all webkit.

afercia commented 6 years ago

I guess part of the issue here is that aria-modal shouldn't be set on the modal overlay. Instead, this attribute is meant to be applied on the same element that has a role="dialog". See:

ARIA 1.1 https://www.w3.org/TR/wai-aria-1.1/#aria-modal Inherits into Roles: alertdialog, dialog

WAI-ARIA Authoring Practices 1.1 https://www.w3.org/TR/wai-aria-practices/#dialog_modal See the working example: https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

<div role="dialog" id="dialog1" aria-labelledby="dialog1_label" aria-modal="true" class="default_dialog">