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

Improve accessibility by associating for with id instead of name #230

Merged
merged 3 commits into from
Dec 14, 2023

Conversation

plocket
Copy link
Collaborator

@plocket plocket commented Dec 12, 2023

Closes #214. No rush on my part. I've tested it a bit myself to make sure basic behavior hasn't changed, though I don't think I have the tools I need to test the accessibility properly.

Not sure if we want to increment those package versions.

Copy link
Member

@nonprofittechy nonprofittechy left a comment

Choose a reason for hiding this comment

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

Just need to fix formatting; I'll merge that PR

@plocket
Copy link
Collaborator Author

plocket commented Dec 12, 2023

I think Bryce wanted to test some things before merging this

@nonprofittechy
Copy link
Member

I think Bryce wanted to test some things before merging this

I meant merge the PR from actions/Black, but that doesn't exist. Looks like we need to point to Bryce's new action in ALActions for formatting; the one this repo points to is broken.

Copy link
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

Got to testing it out, and it does seem to associate the "Month" / "Day" / "Year" labels better with their inputs, so success! However, I think the conversation we started having in #164 comes into play too, as there are some issues with the original field label.

In Chromium, I get this error:

The label's for attribute doesn't match any element id. This might prevent the
browser from correctly autofilling the form and accessibility tools from working correctly.

To fix this issue, make sure the label's for attribute references the correct id of a form field.

which happens because we set that input to be hidden, so most accessibility tools won't include it as a part of the page's content, and will assume that the label points to nothing.

I think that we could make the whole widget a fieldset, with that label becoming the legend. I manually edited that HTML in the browser tools, and it's more clearly associated with the inputs, which is great, essentially just changing the div.da-field-container-datatype-ThreePartsDate to a fieldset, and changing the label tag to a legend. My HTML looks like this:

<fieldset class="da-container da-form-group row da-field-container da-field-container-datatype-ThreePartsDate">
    <legend for="ZGF0ZV9vZl9ncmFkdWF0aW9u" class="col-md-4 col-form-label da-form-label datext-right">
        When will your high school graduation be?
    </legend>
    <div class="col-md-8 dafieldpart">
        (unchanged)
        ...
</fieldset>

I'm not sure how much harder these changes would be; if you think they're a lot harder, we can merge this and take another shot at them later (and to be extra clear, these issues are also present in main, so there's nothing wrong with this PR). But if you think that it'd be easy to add while this is fresh in our minds, I think it'd be a significant improvement.

docassemble/ALToolbox/ThreePartsDate.py Outdated Show resolved Hide resolved
@plocket
Copy link
Collaborator Author

plocket commented Dec 14, 2023

I'm not sure how much harder these [fieldset] changes would be; if you think they're a lot harder, we can merge this and take another shot at them later

I do believe the fieldset conversation is a more complex one and added to the discussion in #164. We should probably save it for a separate discussion and PR. Maybe a deep dive? I've added it to the list in the doc for whenever the next deep dive happens.

Copy link
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

Let's get these improvements in while we discuss further in #164

@BryceStevenWilley BryceStevenWilley merged commit a0e6bf7 into main Dec 14, 2023
4 checks passed
@BryceStevenWilley BryceStevenWilley deleted the 214_a11y_id branch December 14, 2023 16:22
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.

for/id associations for ThreePartDate
3 participants