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

Accessibility issues #8954

Open
caturria opened this issue Oct 7, 2023 · 8 comments
Open

Accessibility issues #8954

caturria opened this issue Oct 7, 2023 · 8 comments
Milestone

Comments

@caturria
Copy link

caturria commented Oct 7, 2023

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 element itself.

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

  • You can donate funding to this issue. We receive the money once the issue is completed & confirmed by you.
  • 100% of the funding will be distributed between the Filament core team to run all aspects of the project.
  • Thank you in advance for helping us make maintenance sustainable!
Fund with Polar
@caturria caturria added bug Something isn't working low priority unconfirmed labels Oct 7, 2023
@github-project-automation github-project-automation bot moved this to Todo in Roadmap Oct 7, 2023
@zepfietje zepfietje added this to the v3 milestone Oct 8, 2023
@zepfietje zepfietje changed the title A couple of accessibility issues Accessibility issues Oct 8, 2023
@caturria
Copy link
Author

caturria commented Oct 9, 2023

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
Copy link
Member

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
Copy link
Author

caturria commented Oct 9, 2023

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 do not wrap collections; they rather apply to individual elements.

@caturria
Copy link
Author

caturria commented Oct 9, 2023

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
Copy link
Member

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
Copy link
Author

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.

@polar-sh polar-sh bot added the Fund label Jun 3, 2024
@danharrin danharrin removed the fund label Jun 4, 2024
@Jamiewarb
Copy link

@caturria Were you able to look at this at all?

@caturria
Copy link
Author

Hi all,

I sincerely apologize for taking so long to get back to you.
I have an upcoming call with @zepfietje this coming Tuesday morning, and I will be using Filament for a couple of projects in the coming months. So I will definitely be able to let you know what's working and what isn't and perform a great deal more testing than before.

What I do know so far is this:

  • The navbar being at the bottom of the DOM (which as I understand from code comments, was done to work around a bug with charts) presents strange UX for screen reader users. With few exceptions, screen readers present content in DOM order, so the nav appears at the bottom of the page. Is there possibly another solution to the chart issue?

  • On the demo site, if I navigate to 'New blog post', the dropdowns for 'Category' and 'Author' are entirely unreadable.

  • The categories table is a bit confusing:

    • In the header, the 'Sort by Ascending' button appears to be duplicated. If you have an Aria label on one element and a visible label in another, this would result in the label being read twice.

    • Having the sort options as header columns isn't ideal, because then the screen reader thinks these are column names that are also relevant to subsequent rows.

    • Whatever the 3rd column is supposed to be presents as an unlabeled button.

  • The table that's presented on the 'Authors' page is not presented as a table at all. The sort UX is much better, but because the rest of it does not carry the role of a table/ grid and corresponding rows, each row reads as one long line of text.

  • On a much more positive note, the modals now prevent access to what's beneath them, which is exactly what they should do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

4 participants