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

fix: link form-item label to field via aria-labelledby #3088

Merged
merged 13 commits into from
Nov 25, 2021

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Nov 19, 2021

Description

The PR makes screen readers announce the form-item label when tabbing to the first field inside the item. This is achieved by linking the label with the field via the aria-labelledby attribute on:

  • the ARIA target element when the field specifies the ariaTarget property (that typically refers to a slotted input)
  • the field element itself otherwise

Fixes #1145

Type of change

  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 24f17a0 to 2de5723 Compare November 19, 2021 08:12
@vursen vursen changed the title feat: link label to field via aria-labelledby feat: link form-item label with field via aria-labelledby Nov 19, 2021
@vursen vursen changed the title feat: link form-item label with field via aria-labelledby feat: link form-item label to field via aria-labelledby Nov 19, 2021
@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 2de5723 to b9e6427 Compare November 22, 2021 09:29
@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 888dd9e to 280b7a9 Compare November 22, 2021 14:48
@vursen vursen marked this pull request as ready for review November 22, 2021 15:16
packages/form-layout/package.json Outdated Show resolved Hide resolved
packages/form-layout/src/vaadin-form-item.js Outdated Show resolved Hide resolved
packages/form-layout/test/form-item.test.js Outdated Show resolved Hide resolved
packages/form-layout/test/form-item.test.js Outdated Show resolved Hide resolved
@vursen
Copy link
Contributor Author

vursen commented Nov 23, 2021

I realized that the feature is not fully completed yet. In the Designer, it is possible to dynamically replace the label with a new one as well as fields can be re-arranged or replaced so that the label id should be properly unlinked in the case of the order change or the removal of a field. All this will presumably require more observers.

@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 2e7adcb to ea59696 Compare November 23, 2021 15:13
@vursen
Copy link
Contributor Author

vursen commented Nov 24, 2021

I realized that the feature is not fully completed yet. In the Designer, it is possible to dynamically replace the label with a new one as well as fields can be re-arranged or replaced so that the label id should be properly unlinked in the case of the order change or the removal of a field. All this will presumably require more observers.

On the other hand, this is probably not a significant problem for the user since it turns out that the form-item label accessibility can be only violated temporarily, as long as constructing a View in the Designer. Note, as a result of the Designer, there is always a static View where every field has its static place and nothing changes dynamically.

From that perspective, the dynamic re-arranging of fields is probably not necessary to be supported because the feature targets only single field form-items, while supporting dynamic adding a label still seems useful.

@web-padawan
Copy link
Member

From that perspective, the dynamic re-arranging of fields is probably not necessary to support

I agree it might bring unnecessary complexity. Let's not aim to cover all the possible edge cases.
Please revert the logic in a separate commit so we can compare the diff.

@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 1b41aeb to 69889a5 Compare November 24, 2021 09:41
@vursen
Copy link
Contributor Author

vursen commented Nov 24, 2021

Pushed b557ead where I simplified the form-item a little bit in a way that it doesn't care anymore about cleaning the label id and aria-labelledby for removed nodes since I considered such a behavior worthless for the real applications: it can possibly violate the accessibility only in the Designer and only for as long as a View is being constructed.

@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 69889a5 to 35c09de Compare November 24, 2021 10:57
@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 35c09de to b557ead Compare November 24, 2021 10:58
packages/field-base/src/utils.d.ts Show resolved Hide resolved
packages/field-base/src/utils.js Show resolved Hide resolved
packages/field-base/src/utils.js Show resolved Hide resolved
packages/form-layout/src/vaadin-form-item.js Outdated Show resolved Hide resolved
@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from 8603566 to ab3e4fa Compare November 25, 2021 10:20
@vursen vursen force-pushed the feat/form-item/link-label-to-field branch from ab3e4fa to 2404be4 Compare November 25, 2021 10:25
@web-padawan web-padawan changed the title feat: link form-item label to field via aria-labelledby fix: link form-item label to field via aria-labelledby Nov 25, 2021
@sonarcloud
Copy link

sonarcloud bot commented Nov 25, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@web-padawan web-padawan merged commit ffe5d0d into master Nov 25, 2021
@web-padawan web-padawan deleted the feat/form-item/link-label-to-field branch November 25, 2021 11:11
@vaadin-bot
Copy link
Collaborator

Hi @vursen , this commit cannot be picked to 22.0 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick ffe5d0d
error: could not apply ffe5d0d... fix: link form-item label to field via aria-labelledby (#3088)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 23.0.0.alpha1 and is also targeting the upcoming stable 23.0.0 version.

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

Successfully merging this pull request may close these issues.

[form-layout] Update <vaadin-form-item> to link fields to its own label using aria-labelledby
4 participants