filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
17.13k stars 2.69k forks source link

Accessibility issues #8954

Open caturria opened 10 months ago

caturria commented 10 months ago

Package

filament/filament

Package Version

^3.0-stable

Laravel Version

^10.10

Livewire Version

^3.0

PHP Version

8.2.6

Problem description

Hi,

First and foremost, I just wanted to say what an impressive package this is! As a blind person, this is the easiest way I've found so far to develop UIs without having to worry too much about what it looks like.

I have come across a couple of accessibility issues however. One has to do with form labels and the other involves modals.

When using a screen reader to interact with a Filament form, the screen reader sometimes fails to pick up the label. This is incredibly strange as usually an element is either accessible or it isn't, but here, every time you refresh the exact same form, it appears completely random whether a given form label will be readable or not. Though this might be due at least in part to a bug in either Chromium and/or Microsoft's accessibility stack, the issue appears to stem from the fact that the text of a label is not a direct child of the

This can be solved either by adding aria-label="{{$label}}" to the element directly, or by rendering the "hidden" label unconditionally. In either case, you would then want to mark the visual label aria-hidden="true" to avoid any conflicts.

The other issue has to do with modals. When a modal is opened, the rest of the page contents should be marked using the aria-hidden attribute. It should also have either role="dialog" or role="alertdialog" which cause screen readers to issue a special notification that a dialog has opened. Otherwise, the user can interact with page contents that are supposed to be hidden by the modal. We also don't realize that a modal has opened up unless we manually scroll to the bottom of the page. Please note: I am providing the live demo as a repository link as all of this is demonstrable using the demo.

Thank you for your attention to this.

Kind regards,

Jordan.

Expected behavior

Screen readers should always be able to identify form labels. Screen readers should not be able to see content that's supposed to be hidden by modal dialogs, and should notify users when a dialog has appeared.

Steps to reproduce

1) Navigate to the live demo and log in. 2) Navigate to a create or edit form (such as blog posts). 3) Load up a screen reader (such as Jaws or NVDA on Windows). 4) Read the page using your arrow keys (if you get trapped in an input field, press escape to get out). 5) Refresh the page several times and repeat attempting to read it with your arrow keys. 6) Observe that some form labels are missed by the reader seemingly at random. 7) Navigate to a confirmation modal (such as for deletion of a record) (you can perform this step using mouse navigation). 8) Attempt to read the page with your arrow keys and screen reader. 9) Observe that you are interacting with content that is supposed to be obstructed by the modal. Also note that you were given no indication that the modal appeared.

Reproduction repository

https://github.com/filamentphp/demo

Relevant log output

No response

Donate 💰 to fund this issue

Fund with Polar

caturria commented 10 months ago

Update: I discovered another one. The button to close a modal lacks a "aria-label" as well. This results in something that can almost be described as undefined behaviour; every combination of screen reader, browser and operating system will present this differently. In the case of Windows and the Jaws screen reader, this is announced as "unlabeled 1 button". In other environments it may simply be announced as "button". Others may just ignore it and entirely prevent the user from interacting with it. An aria-label of "Close" or "Okay" would resolve this.

danharrin commented 10 months ago

Hi @caturria, thanks for getting in touch! Accessibility is very important to me, and I'd like to get these issues resolved as quickly as possible.

With regards to aria-label, can it be placed on a non-input element, as long as it wraps the input? For example, we have a few inputs that do not use native browser input elements, can we add the label to the outer-most element of that input?

About the modal, I think that we can probably solve this with Alpine.js's "focus trapping" feature. The problem is that I haven't been able to get it to properly work yet with all of our form elements. However, I haven't tested it with Livewire 3, only Livewire 2, so there is a chance all the issues are fixed now. I will try to find time to test it out soon.

caturria commented 10 months ago

Hi Dan,

Thank you very much for getting back to me.

In the interest of full disclosure, I am neither an expert on WGAC or the Aria spec. However, what I can say for sure is that different assistive technology products tend to interpret things differently, and the rules can be vague at times. Though I cannot find an authoritative statement on aria-label and friends being applied to the target element's parent, I performed the following simple test and it failed spectacularly:

<!DOCTYPE html>
<html>
    <head>
        <title>Nested aria-label test </title>
        </head>

        <body>
            <form action="localhost" method="post">
                <div aria-label="Test input:">
                <span>
                    <div>
                        <input type="text"></input>
                    </div>
                </span>
            </div>
            </form>
        </body>
</html>

I welcome correction if I'm wrong, but my understanding is that aria-labeledby, aria-label and even

caturria commented 10 months ago

Edit: I didn't realize that Github would not escape HTML tags. I do believe that even the "for" attribute of HTML labels are applicable to a single element as opposed to a collection.

danharrin commented 8 months ago

Hello! I'm getting back to this now after a couple of busy months. I've got some more information to you about our core implementations, and it would really help me if you could test a few things for me.

To start with, the modals: The Alpine.js "trap" feature is now active in our modals. I've found this piece of code that sets the aria-hidden attributes on everything outside the modal - https://github.com/alpinejs/alpine/blob/b7bfd88633d95180ffaddfe09afef294420a99ac/packages/focus/src/index.js#L165-L179. If you update Filament, is the issue with the hidden content still there?

For the modal close button, it does actually have a label. The label is inside the <button> element, inside a <span>:

<button type="button">
    <span>
        Close
    </span>
</button>

Please could you test that and let me know if it works with your screenreader?

For the inputs: every input in Filament has a corresponding label that looks like this:

<div>
    <label for="inputId">
        Label here
    </label>

    <div>
        <input type="text" id="inputId" name="inputId" />
    </div>
</div>

Since this is directly assigned to the ID of the input element, I'm unsure why this would not be working. Can you try that example code above with your screenreader to see what happens?

Thank you so much for your patience and support!

caturria commented 4 months ago

Hi,

First of all, I would like to apologize for taking a very long time to respond to this. I am looking into this now and I will have more information for you in the next couple of days.

Thank you.