KEMBAR78
fix: reflect row aria-expanded state for toggle cells by ugur-vaadin · Pull Request #10156 · vaadin/web-components · GitHub
Skip to content

fix: reflect row aria-expanded state for toggle cells #10156

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 4 commits into from
Sep 16, 2025
Merged

fix: reflect row aria-expanded state for toggle cells #10156

merged 4 commits into from
Sep 16, 2025

Conversation

@ugur-vaadin
Copy link
Contributor

Description

When cell focus mode is active and the row is expanded/collapsed via Space key, the state change is not announced for NVDA and VoiceOver.

This PR reflects the row aria-expanded state to the toggle cells so that the announcements in cell focus mode works correctly.

Fixes #5791

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/contributing/pr
  • 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 reviewed Sep 15, 2025
View reviewed changes
packages/grid/src/vaadin-grid-a11y-mixin.js Outdated
* @return {HTMLElement|null}
* @private
*/
__a11yFindTreeToggleCell(row) {
Copy link
Contributor

@vursen vursen Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's put this helper function into vaadin-grid-helpers.js since it doesn't depend on the class logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted the function.

packages/grid/src/vaadin-grid-a11y-mixin.js Outdated
_a11yUpdateRowExpanded(row) {
if (this.__isRowExpandable(row)) {
row.setAttribute('aria-expanded', 'false');
const toggleCell = this.__a11yFindTreeToggleCell(row);
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable could be created outside the branches to avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted the line.

vursen
vursen reviewed Sep 16, 2025
View reviewed changes
@ugur-vaadin @vursen
Update packages/grid/src/vaadin-grid-helpers.js
c96eb31 
Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

vursen
vursen approved these changes Sep 16, 2025
View reviewed changes
Copy link
Contributor

@vursen vursen left a comment

Choose a reason for hiding this comment

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

Tested on Windows with NVDA – works correctly.

@DiegoCardoso
Copy link
Contributor

There's a behavior that I first thought was getting introduced by these changes, but it seems that it is the same when the changes are reverted (I simply made the helper function return null to make it easier to test). I tried to record it in the video below. Basically, after expanding one row, it starts to announce the other rows' expanded state. I don't know if I am not testing it properly or if this is the expected behavior. Could you guys double-check it? Tested on Safari + VO:

row-a11y.mp4

@vursen
Copy link
Contributor

vursen commented Sep 16, 2025 •
edited
Loading

Basically, after expanding one row, it starts to announce the other rows' expanded state. I don't know if I am not testing it properly or if this is the expected behavior

I can confirm that as well.

@ugur-vaadin
Copy link
Contributor Author

It seems like that issue is browser-specific and does not reproduce on Chrome. In any case, it looks like a separate issue. I will create another one for that and merge this one if it makes sense @DiegoCardoso @vursen

@DiegoCardoso
Copy link
Contributor

Sounds good to me, especially if it's not related to this change. I wanted to mention it to confirm this finding.

@ugur-vaadin ugur-vaadin merged commit d70d60d into main Sep 16, 2025
8 of 9 checks passed
@ugur-vaadin ugur-vaadin deleted the refactor-reflect-row-aria-expanded-state-for-toggle-cells branch September 16, 2025 12:00
vaadin-bot pushed a commit that referenced this pull request Sep 16, 2025
@ugur-vaadin @vursen @vaadin-bot
fix: reflect row aria-expanded state for toggle cells (#10156)
90ebe30 
* refactor: reflect row aria-expanded state for toggle cells

* refactor: get toggle cell once to avoid duplication

* refactor: extract logic for finding toggle cell into helper

* Update packages/grid/src/vaadin-grid-helpers.js

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>

---------

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
vaadin-bot pushed a commit that referenced this pull request Sep 16, 2025
@ugur-vaadin @vursen @vaadin-bot
fix: reflect row aria-expanded state for toggle cells (#10156)
09dacb6 
* refactor: reflect row aria-expanded state for toggle cells

* refactor: get toggle cell once to avoid duplication

* refactor: extract logic for finding toggle cell into helper

* Update packages/grid/src/vaadin-grid-helpers.js

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>

---------

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
@ugur-vaadin ugur-vaadin mentioned this pull request Sep 16, 2025
Open
10 tasks
web-padawan pushed a commit that referenced this pull request Sep 17, 2025
@vaadin-bot @ugur-vaadin @vursen
fix: reflect row aria-expanded state for toggle cells (#10156) (#10184)
d117fba 
Co-authored-by: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com>
Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
web-padawan pushed a commit that referenced this pull request Sep 17, 2025
@vaadin-bot @ugur-vaadin @vursen
fix: reflect row aria-expanded state for toggle cells (#10156) (#10185)
358e797 
Co-authored-by: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com>
Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 25.0.0-alpha12.

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

Reviewers

@vursen vursen vursen approved these changes

@DiegoCardoso DiegoCardoso Awaiting requested review from DiegoCardoso

Assignees

No one assigned

Labels

cherry-picked-24.8 cherry-picked-24.9 Released with Vaadin 25.0.0-alpha12 target/24.8 target/24.9

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TreeGrid expanded/collapsed status not announced by NVDA

4 participants

@ugur-vaadin @DiegoCardoso @vursen @vaadin-bot