Improve the reverse navigation order of focusable elements within KTable cells

[shivam-daksh]

Description

This pull request addresses an issue with the Shift+Tab key navigation behavior in the KTable component (follow-up to issue #795). Previously, pressing Shift+Tab to navigate backward through table cells did not follow the expected reverse order of focusable elements, leading to an unintuitive and confusing user experience.

Issue addressed

Closes #837

Addresses #PR# HERE
#795

Before/after videos:

Before:
View actual behavior video

After:
View expected behavior video

Example:

Given a table with the following structure:

• Row 1:

  • Cell 1
  • Cell 2
  • Cell 3 containing:
    • Button 1
    • Button 2

• Row 2:

  • Cell 1 (current focus)
  • Cell 2
  • Cell 3 containing:
    • Button 1
    • Button 2

Expected Behavior:

• When using Shift+Tab, the focus should move backward through the focusable elements within each table cell in reverse DOM order.
• For example, if the focus is on Cell 1 in Row 2, pressing Shift+Tab should move the focus to Button 2 inside Cell 3 of Row 1, then to Button 1, then to Cell 3, and so on.

Changes Made:

• Modified the handleTabKey function to ensure that Shift+Tab navigation moves the focus through focusable elements in reverse DOM order.
• Corrected the behavior to ensure that pressing Shift+Tab from a focusable element moves focus to the previous focusable element within the cell, and continues navigating through the table cells in reverse order.
• Removed the unintended Cell 4 from the example in the associated issue description for clarity.

Changelog

• Description: Corrected Shift+Tab navigation behavior to follow the expected reverse order of focusable elements within cells.
• Products impact: bugfix
• Addresses: [KTable]: Improve Shift+Tab key navigation in KTable #837 Refactor handleKeydown Method for Improved Readability and Maintainability #804
• Components: KTable
• Breaking: no
• Impacts a11y: yes
• Guidance: No special guidance required for consumers.

Steps to Test:

1. Open the KTable component with multiple rows and focusable elements within cells (e.g., buttons).
2. Focus on a cell (e.g., Cell 1 of Row 2).
3. Press Shift+Tab and verify that the focus moves to the correct previous focusable element (e.g., Button 2 inside Cell 3 of Row 1).
4. Continue pressing Shift+Tab and verify that focus correctly moves through the elements in reverse order, respecting the DOM structure.

(optional) Implementation notes

At a high level, how did you implement this?

Initially focusCell in updateFocusState method was causing diffculty in implementation of the ideal navigation. It can be tackled by introducing a third parameter in updateFocusState to check if focusCell is necessary or not.

Code

      updateFocusState(nextRowIndex, nextColIndex, shouldFocusCell = true) {
        this.focusedRowIndex = nextRowIndex === -1 ? null : nextRowIndex;
        this.focusedColIndex = nextColIndex;
        this.highlightHeader(nextColIndex);

        if (shouldFocusCell) {
          this.focusCell(nextRowIndex, nextColIndex);
        }
      },

            const prevCellAndFocusableElements = [prevCell, ...prevFocusableElements];
            prevCellAndFocusableElements[prevCellAndFocusableElements.length - 1].focus();
            this.updateFocusState(nextRowIndex, nextColIndex, false);
            event.preventDefault();

Does this introduce any tech-debt items?

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical and brittle code paths are covered by unit tests
• The change is described in the changelog section above

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

Comments

[shivam-daksh]

@MisRob and @BabyElias , Please have a look at it.

[BabyElias]

@shivam-daksh , The overall working looks nicee to me now. Thanks :)
This PR contains all commits that the #804 PR about Refactoring handlekeydown has. So, I was just wondering if we should consider both the PRs or just this one for both the issues and have QA and further performance reviews for this PR itself? Opinions on this @MisRob?

[BabyElias]

About the use of querySelectorAll here, I was actually having a little lag in the navigation behaviour when moving through the focusable elements - this reminded me of an earlier code review comment here. We were using querySelectorAll earlier as well, but it caused performance issues, so we shifted to getElementByTagname and it had a very positive impact on the same. Can you update this code to make use of the previous getElementbyTagname logic? or maybe if there's any other conclusion you derive from that conversation about performance improvement- it is more than welcome. Thanks!

[shivam-daksh]

@BabyElias, Thanks for noticing. I've changed it to getElementsByTagName.

[shivam-daksh]

I was just wondering if we should consider both the PRs or just this one for both the issues and have QA and further performance reviews for this PR itself? Opinions on this @MisRob?

@BabyElias , I was thinking the same. Let's leave it to @MisRob what she decides.

[MisRob]

Thanks @shivam-daksh and @BabyElias.

Since the first PR review is almost complete, I would recommend to wait until we finish QA and merge #804. Then @shivam-daksh you could rebase this PR on top of the latest develop which will only keep the new diff.

[MisRob]

Hi @shivam-daksh, I've just merged #804. Could you now rebase this PR on top of the latest develop?

[shivam-daksh]

@MisRob, Sure

[shivam-daksh]

@MisRob, Please have a look.

[MisRob]

Hi @shivam-daksh, there are many conflicting files now - perhaps something went wrong during the rebase/merge?

[shivam-daksh]

Hi @MisRob, I'm running into an issue while rebasing—it’s taking much longer than usual, likely due to changes in yarn.lock. I tried a couple of times, but after waiting for 20–30 minutes, I had to abort the process.

A quick solution I’m considering is starting a new branch from the latest develop and applying my changes there. Let me know what you think or if you have any suggestions.

Looking forward to your guidance!

[MisRob]

Hi @shivam-daksh, ah I'm sorry this is such a hassle. One idea I had was to try to reset all files that are not relevant to this work to their develop version via git checkout develop -- <file_name> (https://stackoverflow.com/a/46571620). This would include package.json and yarn.lock. Then you can run yarn install again. This way you could avoid having to start a new branch. If this doesn't work, of course feel free to start a new branch. You could even force push it to this one perhaps.

[shivam-daksh]

@MisRob, Could you please take a look at this? I believe it should work as expected.

[MisRob]

Thanks @shivam-daksh! The diff now looks meaningful and I confirmed locally that the fix still works. I haven't observed any regressions. Yey, finally merging. Thanks everyone for patience.

[shivam-daksh]

Big Thanks to @MisRob, @BabyElias and @AlexVelezLl for being with me and guiding through the whole. I'm really grateful for your support and all the learning I've gained along the way.

[shivam-daksh]

Hey @MisRob, I’m ready to dive deeper into fixing complex issues and am fully committed to investing time in learning and improving. Looking forward to growing and contributing even more!