Skip to content

AdvancedTable focus bug fix - #4017

Open
zamoore wants to merge 6 commits into
mainfrom
zamoore/hds-6545/AdvancedTable-focus-fix
Open

AdvancedTable focus bug fix#4017
zamoore wants to merge 6 commits into
mainfrom
zamoore/hds-6545/AdvancedTable-focus-fix

Conversation

@zamoore

@zamoore zamoore commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

📌 Summary

Clicking a cell in AdvancedTable made the page jump. Removes the center scroll alignment on Td/Th so focus scrolls by the minimum distance instead.

🛠️ Detailed description

Cells are Composite items with a tabindex so clicking one focuses it and fires hdsScrollIntoViewOnFocus. That modifier was passed block="center" inline="center", and scrollIntoView walks up every scroll ancestor including the document, so center recentered the whole page even when the cell was already visible.

Dropping the override falls back to the default nearest behavior.

Background

The bug was introduced in #3818 when we switched keyboard navigation to use the Composite component.

🔗 External links

Jira ticket: HDS-6545


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

📋 PCI review checklist
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've worked with GRC to document the impact of any changes to security controls.
    Examples of changes to controls include access controls, encryption, logging, etc.
  • If applicable, I've worked with GRC to ensure compliance due to a significant change to the in-scope PCI environment.
    Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.

@zamoore
zamoore requested a review from a team as a code owner July 27, 2026 17:45
@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hds-showcase Ready Ready Preview Jul 29, 2026 8:59pm
hds-website Ready Ready Preview Jul 29, 2026 8:59pm

Request Review

@zamoore zamoore changed the title fixed bug AdvancedTable focus bug fix Jul 27, 2026
@didoo

didoo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

@zamoore can you provide links/references to where this bug was introduced (in the PR description)? in this way we can compare the before/after and have more context about the fix itself

"@hashicorp/design-system-components": patch
---

`AdvancedTable` - Fixed an issue where clicking a cell scrolled the page to center that cell

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[todo] Add start and end comments around entry

Comment thread packages/components/src/components/hds/advanced-table/th.gts Outdated

@didoo didoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is something that I don't understand: I checked out the code at this PR merge #3818 and run the showcase; when I click on a cell inside the AdvancedTable, nothing happens (there is no movement of the scrolled table, no centering of the clicked cell).

This tells me that between then and now, a core behaviour of the AdvancedTable has been changed (but I don't see mention of this change in the version history of the component). Now the table "centers" the clicked cell, which is something... unexpected, as a user. Why do we infer that, if I click a cell, I also intend to center it in the table? This is an assumption that we're making, and I am not sure it's expected (I checked other data tables implementations, and I could not find similar behaviour) expecially because it shifts the visual position of the content and is really disorienting.

I would challenge this implementation (sorry I didn't notice before) and would consider removing entirely this "feature" (and with this, also avoid the bug).

cc @shleewhite @alex-ju

@shleewhite

Copy link
Copy Markdown
Contributor

There is something that I don't understand: I checked out the code at this PR merge #3818 and run the showcase; when I click on a cell inside the AdvancedTable, nothing happens (there is no movement of the scrolled table, no centering of the clicked cell).

This tells me that between then and now, a core behaviour of the AdvancedTable has been changed (but I don't see mention of this change in the version history of the component). Now the table "centers" the clicked cell, which is something... unexpected, as a user. Why do we infer that, if I click a cell, I also intend to center it in the table? This is an assumption that we're making, and I am not sure it's expected (I checked other data tables implementations, and I could not find similar behaviour) expecially because it shifts the visual position of the content and is really disorienting.

I would challenge this implementation (sorry I didn't notice before) and would consider removing entirely this "feature" (and with this, also avoid the bug).

cc @shleewhite @alex-ju

@didoo this feature is needed for keyboard users. without the scrolling behavior, your focus can be outside of the view which is a wcag failure.

I believe this was introduced when we switched to the composite component.

as for other workarounds, one option @zamoore and I talked about was to add a check to the scrolling function that checks if the focus event was initiated by the cursor or keyboard. If it is keyboard initiated, then do the scroll. otherwise, ignore. does that option seem better to you?

@zamoore

zamoore commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

other

@didoo @shleewhite - Yes, that is correct. It wasn't explicitly a feature added to the AdvancedTable, it was just a change in the implementation of an existing feature that introduced a regression.

Edit: Ah, you're saying that the bug is not present in #3818 . That's interesting. I'm not certain what else would have changed between then and now?

@didoo

didoo commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@didoo this feature is needed for keyboard users. without the scrolling behavior, your focus can be outside of the view which is a wcag failure.

Yes, that's what I meant (hence the use of the word click). For keyboard users obviously moving the focused cell into view is the expected behaviour/intent (as user).

@zamoore

zamoore commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@didoo @shleewhite - I updated the code to only react to keyboard events

@didoo didoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There may be an issue (?) with sticky header/columns, where the cell focused via keyboard is not visible (it's behind the sticky header/column):

Screen.Recording.2026-07-29.at.09.18.27.mov

No idea how (and even if) this can be solved.

Comment thread packages/components/src/modifiers/hds-scroll-into-view-on-focus.ts
Comment thread packages/components/src/components/hds/advanced-table/td.gts Outdated
Comment thread packages/components/src/modifiers/hds-scroll-into-view-on-focus.ts
Comment thread packages/components/src/components/hds/advanced-table/td.gts Outdated
Comment thread packages/components/src/components/hds/advanced-table/th.gts Outdated
@dchyun dchyun added this to the design-system-components@6.5.0 milestone Jul 29, 2026
@jorytindall

jorytindall commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

I found this while testing the resizing PR #4020 but it might be more related to this; traversing the grid with arrow keys works fine until I sort a column, which then seems to keep the same focus order but results in jumping around because the focus order is different from the visual order of the cells in the column.

Is this a known issue already or expected?

CleanShot 2026-07-29 at 09 56 32

@zamoore

zamoore commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

I found this while testing the resizing PR #4020 but it might be more related to this; traversing the grid with arrow keys works fine until I sort a column, which then seems to keep the same focus order but results in jumping around because the focus order is different from the visual order of the cells in the column.

Is this a known issue already or expected?

CleanShot 2026-07-29 at 09 56 32

This isn't related, but it's also not a known issue. Can you create a ticket so we can address it on its own? Good find

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.

6 participants