Skip to content

Fixed #32819 -- Established relationship between form fields and their help text. #16920

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 6, 2023
Merged

Fixed #32819 -- Established relationship between form fields and their help text. #16920

merged 1 commit into from
Jul 6, 2023

Conversation

gregorjerse
Copy link
Contributor

No description provided.

github-actions[bot]
github-actions bot reviewed Jun 1, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@knyghty knyghty requested a review from a team June 1, 2023 15:04
@gregorjerse gregorjerse force-pushed the feature/aria_definedby branch from 64d1d75 to 4d157b0 Compare June 1, 2023 15:15
@felixxm felixxm mentioned this pull request Jun 1, 2023
Closed
smithdc1
smithdc1 reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@smithdc1 smithdc1 left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

Could a test be added for the case when a label has already been set in 'userland'? (@thibaudcolas ™️)

Maybe there's one here already, but I couldn't see it reviewing on mobile.

@gregorjerse gregorjerse force-pushed the feature/aria_definedby branch from 4d157b0 to 5b770c3 Compare June 2, 2023 10:13
@gregorjerse gregorjerse requested a review from smithdc1 June 2, 2023 10:36
@gregorjerse gregorjerse force-pushed the feature/aria_definedby branch from 5b770c3 to 23852b6 Compare June 2, 2023 10:54
@gregorjerse gregorjerse mentioned this pull request Jun 2, 2023
Closed
thibaudcolas
thibaudcolas reviewed Jun 2, 2023
View reviewed changes
@gregorjerse gregorjerse requested a review from thibaudcolas June 2, 2023 15:00
thibaudcolas
thibaudcolas approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

🎉 all working great. Here’s a recording of the behavior in VoiceOver Safari desktop for posterity:

aria-describedby

@smithdc1
Copy link
Member

smithdc1 commented Jun 2, 2023

🎉 all working great. Here’s a recording of the behavior in VoiceOver Safari desktop for posterity:

aria-describedby

I see help text is below the input. I thought that was troublesome? Is it an issue here, and if so do we have a separate ticket for it?

nessita
nessita reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you! Added an initial set of comments.

@thibaudcolas
Copy link
Member

@smithdc1 yes, having help text below the input is also a problem. I’ve been hesitant to raise it for two reasons:

  • It’s "only" a usability issue, not strictly speaking accessibility.
  • It seems like a harder challenge to address than everything else we‘ve been working on with forms, combined (fundamentally incompatible with horizontal form layouts – which also are an anti-pattern)

I’ll happily raise this if you want?

nessita
nessita reviewed Jun 7, 2023
View reviewed changes
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, added a second round of comments and suggestions. I agree this is valuable and we should try to progress it!

@knyghty knyghty mentioned this pull request Jun 8, 2023
Merged
thibaudcolas
thibaudcolas approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@thibaudcolas thibaudcolas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just re-approving because I see my review has been cleared :) There’s some room for small improvements in the docs and code comments formatting based on my feedback and @nessita’s but I suspect they’re small enough that the suggested changes can be made as part of merging the PR. Though smarter handling of custom aria-describedby would be good, this strikes me as a nice-to-have that goes well beyond the fix we’re after here.

@smithdc1
Copy link
Member

I think we need to review the docs to make sure that we're using this new approach. Take this example here -- it is missing the id on the help text, just like what has been added to the form templates in this PR:

django/docs/topics/forms/index.txt

Line 635 in ac09d22

<p class="help">{{ field.help_text|safe }}</p>

Maybe this could be updated to <p class="help" id="{{ field.id_for_label}}_helptext">{{ field.help_text|safe }}</p>?

The other file I could see where a change is required is the release notes for 5.0 (5.0.txt)

@nessita
Copy link
Contributor

nessita commented Jun 14, 2023

I think we need to review the docs to make sure that we're using this new approach. Take this example here -- it is missing the id on the help text, just like what has been added to the form templates in this PR:

django/docs/topics/forms/index.txt

Line 635 in ac09d22

<p class="help">{{ field.help_text|safe }}</p>

Maybe this could be updated to <p class="help" id="{{ field.id_for_label}}_helptext">{{ field.help_text|safe }}</p>?

The other file I could see where a change is required is the release notes for 5.0 (5.0.txt)

Good point @smithdc1!

@gregorjerse would you be available to do the review of the rest of the doc that David mentions? If not, no worries, let me know and I'll do it as part of general tidy up.

@nessita nessita self-requested a review June 14, 2023 13:14
felixxm
felixxm reviewed Jun 15, 2023
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@gregorjerse Thanks 👍 I left suggestions.

@felixxm
Copy link
Member

felixxm commented Jun 21, 2023

@gregorjerse Do you have time to keep working on this? We're really close 🚀

@felixxm
Copy link
Member

felixxm commented Jun 30, 2023 •
edited
Loading

@gregorjerse Do you have time to keep working on this? We're really close rocket

@gregorjerse Do you have time to keep working on this? If not, I'm eager to push final edits.

@felixxm felixxm removed request for nessita and smithdc1 July 6, 2023 04:39
@felixxm felixxm self-assigned this Jul 6, 2023
@felixxm felixxm force-pushed the feature/aria_definedby branch 3 times, most recently from 7438445 to d31bd4b Compare July 6, 2023 05:36
@felixxm felixxm changed the title Fixed #32819 -- Added aria-describedby for help_text. Fixed #32819 -- Established relationship between form fields and their help text. Jul 6, 2023
@gregorjerse @felixxm
Fixed #32819 -- Established relationship between form fields and thei…
966ecdd 
…r help text.

Thanks Nimra for the initial patch.

Thanks Natalia Bidart, Thibaud Colas, David Smith, and Mariusz Felisiak
for reviews.
@felixxm felixxm force-pushed the feature/aria_definedby branch from d31bd4b to 966ecdd Compare July 6, 2023 06:03
@felixxm
Copy link
Member

felixxm commented Jul 6, 2023

@gregorjerse Thanks 👍 Welcome aboard ⛵

I pushed final edits.

@felixxm felixxm added the selenium Apply to have Selenium tests run on a PR label Jul 6, 2023
@felixxm felixxm merged commit 966ecdd into django:main Jul 6, 2023
This was referenced Jul 6, 2023
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nessita nessita nessita left review comments

@felixxm felixxm felixxm left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

+2 more reviewers

@smithdc1 smithdc1 smithdc1 left review comments

@thibaudcolas thibaudcolas thibaudcolas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@felixxm felixxm

Labels

DjangoCon Europe 🏰 selenium Apply to have Selenium tests run on a PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gregorjerse @smithdc1 @thibaudcolas @nessita @felixxm