Fixes keyboard navigation in Ktable #900

Pandaa007 · 2025-01-20T13:56:23Z

Description

Changed the handling of Tab & Shift-Tab navigation in the KTable component to address the following:

In the current version, the Tab and Shift-Navigation gets stuck on in the table cells if the same contain any disabled elements (as the current code tries to focus on them blindly by calling .focus() on them, which is not supported for disabled elements)
The shift-tab navigation gets stuck in the header if the same contains any focusable elements.

You can see the behaviour on the current implementation in the following video (The key presses that I am making can't be seen, but I am first trying to navigating forward to the end of the page, and then reverse navigate to the starting. If stuck on disbaled elements, or the header cell in the first table, I using mouse to refocus and continue navigation):

prev.mp4

The same after the changes in the PR are checked out is as follows:

classes-updated.mp4

pg-updated.mp4

I used the following playground snippet to generate the sample page for testing (on which the demo videos are recorded).

Playground

<template>

  <!--
    Playground: A private page for components development
    *******************
    Place a component you're working on here and see it
    live on http://localhost:4000/playground

    Please do not commit your local updates of this file.
  -->
  <div style="padding: 24px">
    <!-- 
      Example: Uncomment lines bellow, change something
      in lib/KBreadcrumbs.vue and see the updates reflected
      on this page
    -->
    <!-- <KBreadcrumbs
      :items="[
        { text: 'Global Digital Library', link: { path: '#' } },
        { text: 'English', link: { path: '#' } },
        { text: 'Level 2 ', link: { path: '#' } },
      ]"
    /> -->
    <!-- Play around with your component here: -->

    <KCheckbox label="First" />
    <KCheckbox label="Second" />

    <KTable
      :headers="headers"
      :rows="rows"
      caption="Table caption"
      sortable
    >
      <template #header="{ header }">
        <!-- <KCheckbox :label="header.label"/> -->
        <KButton>{{ header.label }}</KButton>
      </template>
      <template #cell="{ content, colIndex }">
        <span v-if="colIndex === 0">
          <!-- <KCheckbox :label="content"/> -->
          <KButton>{{ content }} 1 </KButton>
          <KButton disabled>{{ content }} 2 </KButton>
          <KButton>{{ content }} 3 </KButton>
        </span>
        <span v-else>{{ content }}</span>
      </template>
    </KTable>

    <KCheckbox label="Third" />
    <KCheckbox
      label="Fourth"
      disabled
    />
    <KCheckbox label="Fifth" />
    
    <KTable
      :headers="headers"
      :rows="rows"
      caption="Table caption"
    />

    <KCheckbox label="Sixth" />
  </div>

</template>


<script>

  /*
    Playground is a Vue component too,
    so you can also use data, methods, etc.
    as usual if helpful
  */
  export default {
    name: 'Playground',
    data() {
      return {
        headers: [
          { label: 'Name', dataType: 'string', columnId: 'name' },
          { label: 'Age', dataType: 'number', columnId: 'age' },
          { label: 'City', dataType: 'string', columnId: 'city' },
        ],
        rows: [
          ['John Doe', 28, 'New York'],
          ['Jane Smith', 34, 'Los Angeles'],
          ['Emily Davis', 27, 'Philadelphia'],
        ],
      };
    },
    methods: {},
  };

</script>

To make the changes, I have

Updated the code and introduced many minor functions like getNextCellCoordinates and getPreviousCellCoordinates to that the actual navigation logic becomes more concise and easy to follow (reducing the inling of code)
Seperated out the code for forward and backward tab navigation in different functions, as it allows us to make some minor improvements in performace (like while forwarding navigating with tab, if we are at the end of the focusable elements in the current cell, we do not need to query all the focusable elements of the next cell, and can directly just focus on the next cell itself!)
Created helper methods to find the focusable elements in a cell, and modified it to handle the disabled state of elements.

Issue addressed

Fixes #883

Changelog

Description: Fixed bug in keyboard navigation in KTable
Products impact: bugfix
Addresses: [KTable]: Shift + Tab navigation is trapped #883
Components: KTable
Breaking: no
Impacts a11y: yes
Guidance: Fixes bug with keyboard navigation for consumers of KTable

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

MisRob · 2025-01-22T18:39:52Z

Hi @Pandaa007, thank you! I appreciate your critical and pro-active mindset, and it is true these improvements are needed. However I'd recommend to check on proposed changes that are out of scope beforehand in the issue comment with the team. Unfortunately, we won't be able to accept everything. Some of the extra issues you took on are already addressed and reviewed, and only wait for final QA here:

Refactor handleKeydown Method for Improved Readability and Maintainability #804
Improve the reverse navigation order of focusable elements within KTable cells #840

I'd recommend you only keep changes directly related to #883.

We're a bit behind on QA but I hope we can merge all pre-existing KTable work some time next week so perhaps it'd make sense for you to wait so later you don't need to deal with conflicts since as you can see in the PRs I referenced, there are many changes.

Pandaa007 · 2025-01-23T13:21:29Z

Thanks @MisRob for the review. Totally my bad as I wasn't aware that these problems of refactoring and keyboard navigation were already being worked upon by other contributors, and thus felt that I might adress them here itself. As per your advice, I'd like to wait for them to be merged to the codebase first and then make my changes on top of them! Would really appreciate a tag whenever the same are merged and I can start working again on it. Thanks :)

MisRob · 2025-01-23T13:40:44Z

Thanks for your understanding @Pandaa007. I will make sure to let you know when it's time to rebase.

MisRob · 2025-01-31T18:17:13Z

Hi @Pandaa007, we're close to having everything ready for you. I will follow-up hopefully next week.

MisRob · 2025-02-11T11:57:16Z

Hi @Pandaa007, we've just merged

Refactor handleKeydown Method for Improved Readability and Maintainability #804
Improve the reverse navigation order of focusable elements within KTable cells #840

and KTable in develop is now the latest version, ready for more work.

I assume that #883 will still be valid but let' see.

…table

Pandaa007 · 2025-02-17T14:40:47Z

Hi @MisRob! Thanks for the heads-up. I had checked out the latest changes and noted that the same playground example that I was using to test the navigation was not working correctly with the current code. It was most probably due to not handling the disabled state of specific attributes when working with some tags, and some inconstitencies in handling the default behaviour.

I have updated the PR with the changes so that the example functions well again, as well tried to pull out some of the stuff that was inlined before to sepearate functions for better maintainability. I have also seperated out the code for forward and backward tab navigation, as it allows us to make some minor improvements in performace (like while forwarding navigating with tab, if we are at the end of the focusable elements in the current cell, we do not need to query all the focusable elements of the next cell, and can directly just focus on the next cell itself!)

I would love to hear the views of the team on these changes. Thanks!

MisRob · 2025-02-24T04:09:55Z

Hi @Pandaa007, would you be able to update the PR description with the changes you described in the latest comment and the recordings of the latest issues you mention? I think it would help us to orient ourselves. Thank you.

Pandaa007 · 2025-02-24T13:56:11Z

@MisRob I have updated the same with detailed descriptions of the changes as well as before & after videos! Please let me know if any more modifications are needed!

MisRob · 2025-02-25T06:57:22Z

Thank you @Pandaa007, that's helpful.

@BabyElias this is another chunk of work on your KTable :)! if you'd like to, as always I welcome your review, whether partial or full :) If not possible, no problem - I will review or loop in more people. Thank you.

BabyElias · 2025-02-25T07:21:26Z

Thanks for the heads up @MisRob, will have a look at it :)

MisRob · 2025-02-27T06:31:59Z

Thank you @BabyElias, as always. There's no time pressure around this work, so any time you can - this review will take a while.

BabyElias · 2025-02-27T10:06:25Z

@Pandaa007, thank you for working on this issue!
I tried this new keyboard navigation configuration for KTable in Kolibri (on the Facility>Classes Table, code can be seen here).
A few issues I noticed were:

Navigation lags when moving from the last cell of each row to the first cell of next row, i.e, we expect that for each navigated cell, the corresponding cell header and row headers should be highlighted at all times. However, when moving from last cell to the next first cell- that highlight cannot be seen. However, this highlighting resumes again once we move to the second cell of that row.
For the Shift+Tab key navigation, this highlight feature doesn't work at all.

I think these can also be noticed in the video samples that you have attached in the PR description.

MisRob · 2025-02-27T11:11:06Z

Thank you @BabyElias. @Pandaa007 since the refactor as a whole has some important regressions, I'd recommend to separate this work into two parts. The first one would be solely for #883. After that, if you'd still like to do further improvements, we can consider. This will make it easier for everyone to understand the source of these regressions, and help us to focus on the bug we need to fix and which has much higher priority than further internal refactors.

MisRob · 2025-02-27T11:12:18Z

This will also help the review process since it seems there will be some things to look into deeper here - KTable is quite a nuanced and complex component to work with. Thank you!

Pandaa007 · 2025-02-28T17:40:08Z

Hi @BabyElias. Thanks for your review. I actaully tried to setup Kolibri locally and used this updated version of KDS in the same, but was not able to make out the regressions that you have mentioned in the review. It's entirely possible that I was looking at the wrong screen though as I am new to Kolibri. If it's not too much trouble, can you please attach a video and help me pinpoint the regressions? I can then work to create a minimal reproduction playground script and use that to carry on the changes.

Thanks for your time!

BabyElias · 2025-02-28T17:46:10Z

@Pandaa007, it might take some time before I am able to send the video here. Just for reference uptil then, the current.mp4 file that you have added in the PR description- these can be noticed there as well. The light grey color bg that appears all over the row cell which is active, and the corresponding column header cell is not happening for the first cell of each row or when shift+tab key is used.

Pandaa007 · 2025-02-28T17:56:20Z

Ahh okay! Got it. Completely missed that part. I will look into fixing the same. Thanks for the review!

Also just for reference, is this the correct table which you had earlier mentioned in your comment?

classes.mp4

BabyElias · 2025-02-28T18:10:13Z

Yes, this is the table I am talking about. But for noticing your KDS changes in Kolibri, you need to run Kolibri with your local KDS repo. The command to do so is yarn run devserver-with-kds <path to your KDS folder>. This will enable you to see your changes in action in Kolibri. The current version that you see in the video, uses the latest KDS version that has been installed in Kolibri without any of the local changes that you have made.

Pandaa007 · 2025-02-28T18:36:17Z

Thanks for the guidance @BabyElias. I wasn't aware about the devserver-with-kds command, so I used the plain old yarn link and yarn unlink command to test the changes with my version of KDS. I have updated the code to handle the highlight navigation correctly as well now:

classes-updated.mp4

pg-updated.mp4

I have attached the demo for both the playground the earlier mentioned table. Hope that this is the functionlity that we looking for! I have updated the video demo's in the PR description as well, so that it makes for easier review. I wanted to take @MisRob's advice of separating out the work into two PRs, but I feel that tracking down the exact source of the bug is actually much more cumbersome that refactoring the table internals into clean functions (to reduce inlining). I have mostly followed the original implementation, but just make it more conscise with the help of utilities. I hope that would be okay for this PR. Please let me know if you have any more suggestions on the same.

MisRob · 2025-02-28T18:51:36Z

Hi @Pandaa007

I wanted to take @MisRob's advice of separating out the work into two PRs, but I feel that tracking down the exact source of the bug is actually much more cumbersome that refactoring the table internals into clean functions (to reduce inlining).

Sounds fine - take my words as recommendations in case it would actually be helpful, and no problem to leave it aside when it's not. I am glad to have @BabyElias to coordinate with you as fits best since she's the main reviewer of this work now :)

Pandaa007 added 2 commits January 20, 2025 19:20

Fixed bug

0955378

Minor fix

ba2e66f

MisRob self-assigned this Jan 20, 2025

MisRob self-requested a review January 21, 2025 14:20

MisRob added the TODO: needs review Waiting for review label Jan 27, 2025

Pandaa007 added 2 commits February 17, 2025 19:17

Merge https://github.com/learningequality/kolibri-design-system into …

0192748

…table

fix trapped headers & improve code readibility

210c040

Pandaa007 requested a deployment to percy_tests February 17, 2025 14:33 — with GitHub Actions Waiting

revert playground changes

4c1268f

Pandaa007 requested a deployment to percy_tests February 17, 2025 14:34 — with GitHub Actions Waiting

minor code style update

8faadcc

Pandaa007 requested a deployment to percy_tests February 20, 2025 13:22 — with GitHub Actions Waiting

fix highlights

25d9096

Pandaa007 requested a deployment to percy_tests February 28, 2025 18:33 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes keyboard navigation in Ktable #900

Fixes keyboard navigation in Ktable #900

Pandaa007 commented Jan 20, 2025 •

edited

Loading

MisRob commented Jan 22, 2025 •

edited

Loading

Pandaa007 commented Jan 23, 2025

MisRob commented Jan 23, 2025

MisRob commented Jan 31, 2025

MisRob commented Feb 11, 2025 •

edited

Loading

Pandaa007 commented Feb 17, 2025

MisRob commented Feb 24, 2025

Pandaa007 commented Feb 24, 2025

MisRob commented Feb 25, 2025

BabyElias commented Feb 25, 2025

MisRob commented Feb 27, 2025

BabyElias commented Feb 27, 2025 •

edited

Loading

MisRob commented Feb 27, 2025

MisRob commented Feb 27, 2025

Pandaa007 commented Feb 28, 2025

BabyElias commented Feb 28, 2025

Pandaa007 commented Feb 28, 2025 •

edited

Loading

BabyElias commented Feb 28, 2025

Pandaa007 commented Feb 28, 2025 •

edited

Loading

MisRob commented Feb 28, 2025

Fixes keyboard navigation in Ktable #900

Are you sure you want to change the base?

Fixes keyboard navigation in Ktable #900

Conversation

Pandaa007 commented Jan 20, 2025 • edited Loading

Description

Issue addressed

Changelog

Testing checklist

Reviewer guidance

MisRob commented Jan 22, 2025 • edited Loading

Pandaa007 commented Jan 23, 2025

MisRob commented Jan 23, 2025

MisRob commented Jan 31, 2025

MisRob commented Feb 11, 2025 • edited Loading

Pandaa007 commented Feb 17, 2025

MisRob commented Feb 24, 2025

Pandaa007 commented Feb 24, 2025

MisRob commented Feb 25, 2025

BabyElias commented Feb 25, 2025

MisRob commented Feb 27, 2025

BabyElias commented Feb 27, 2025 • edited Loading

MisRob commented Feb 27, 2025

MisRob commented Feb 27, 2025

Pandaa007 commented Feb 28, 2025

BabyElias commented Feb 28, 2025

Pandaa007 commented Feb 28, 2025 • edited Loading

BabyElias commented Feb 28, 2025

Pandaa007 commented Feb 28, 2025 • edited Loading

MisRob commented Feb 28, 2025

Pandaa007 commented Jan 20, 2025 •

edited

Loading

MisRob commented Jan 22, 2025 •

edited

Loading

MisRob commented Feb 11, 2025 •

edited

Loading

BabyElias commented Feb 27, 2025 •

edited

Loading

Pandaa007 commented Feb 28, 2025 •

edited

Loading

Pandaa007 commented Feb 28, 2025 •

edited

Loading