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 :)

BabyElias · 2025-03-07T06:49:41Z

The changes look good to me, functionality is working as expected and the Shift+Tab key nav doesn't get stuck anymore :)
I think the PR can be sent for QA now, @MisRob.

On a side note - I did work with the playground code provided here, and noticed something that might be worth working on (not as a part of this issue, but maybe a follow-up). The checkbox as a part of KTable is focusable, but on pressing the enter key - it's not toggled as such (i.e originally, checkboxes are meant to toggle state as checked and unchecked upon pressing the enter key but that does not happen here). If you are interested in working on this @Pandaa007, I'll create an issue for the same and assign it to you, or maybe you can raise an issue yourself and start working on it.
For reference, the KTable cells are made up of a component called KTableGridItem. So, while KTable is responsible for managing navigation from one cell to another, KTableGridItem is responsible for managing navigation within the cell. This issue of enter key not toggling states is also associated with the KTableGridItem code for enter key here..

Apart from this, talking about a wider scope of work - I was thinking that the code for checking focusable elements within a cell and their subsequent navigation could be refactored within KTableGridItem itself rather than KTable. This would help us continue ensuring the separation of responsibilities between KTable and KTableGridItem, making tracking of bugs easier in the future and also, allowing us to incorporate more functionalities easily when needed. This might require major refactor and number of considerations to be made, so not suggesting that we do it immediately - but for future references, I thought it would be good to put it out here,

Pandaa007 · 2025-03-07T10:11:37Z

Thanks a lot for the review @BabyElias for the review and recommending the PR for further QA.

Thanks for pointing out the additional issue with accessibility issue associated with the Enter key and the concern that you pointed out. I also agree that it would make sense that we move the intra-cell navigation logic to KTableGridItem itself, and the item component should be responsible be managing the focus and accessibility behaviour when the focus is "inside" the cell, and KTable should only be responsible for managing the same when the whole cell is in focus. This would further help us avoid these little caveats that we you mentioned. I would love to work on the same, and can open the issue if that is something the maintainers would like to see to be worked upon.

Edit: Maybe we could add this as a sub-issue to #829? It could act as a parent issue tracker for the KTable related work?

Pandaa007 · 2025-03-07T10:18:41Z

Also, might be a bit out of scope for this discussion, but since we are ideating here, I also had an idea that I thought I would open post the merging of this PR: I have been working on introducing VTL to KDS via my pull request #897. After the same is merged, I could use the same to create a bit more elaborate test suite for keyboard naviagtion using:

Tab
Shift + Tab
Enter
Arrow Keys

that can make sure that the order of the elements in foucs, and the row and column highlights work as expected. This would help us to future-proof these changes across PR, and can help to reduce the burden of this mannual testing of PRs before QA to catch regressions in the bud itself. I really have had a pleasant experience (that is both challenging and educative) working with the KTable, and would like to help more with the same as much as I could.

Thanks @MisRob @BabyElias again for all the guidance.

MisRob · 2025-03-13T07:36:27Z

Thanks both @Pandaa007 @BabyElias, wonderful work, and so many good suggestions for the future work!

(1) Kolibri QA PR for final approval here

(2)

The checkbox as a part of KTable is focusable, but on pressing the enter key - it's not toggled as such

This is general checkbox intended behavior, only Space should toggle it, not Enter (can be tried out here). Few references

I too was a bit surprised when I learned about it on another opportunity :) It's confirmed with @radinamatic though so no updates needed.

Apart from this, talking about a wider scope of work - I was thinking that the code for checking focusable elements within a cell and their subsequent navigation could be refactored within KTableGridItem itself rather than KTable

High-level this makes sense to me @BabyElias and @Pandaa007, yes I welcome a new issue (but not sub-issue of #829, that's concerned with public interface only). Please also post a comment to the issue right away if it's something you wish to work on and reference this comment of mine to ensure it's reserved for you.

(3)

I have been working on introducing VTL to KDS via my pull request #897. After the same is merged, I could use the same to create a bit more elaborate test suite for keyboard naviagtion

That would be very helpful in long term, and if it was done before (2), then it would give us even more confidence during refactoring, right? #897 slipped my attention, I will try if we can get it in finally.

MisRob · 2025-03-13T07:37:48Z

I really have had a pleasant experience (that is both challenging and educative) working with the KTable, and would like to help more with the same as much as I could.

I am happy to hear it is rewarding, KTable indeed is one of the more complex and nuanced components, but thanks to @BabyElias's help, it is possible to loop in new developers. We appreciate this work very much.

BabyElias · 2025-03-13T08:17:58Z

This is general checkbox intended behavior, only Space should toggle it, not Enter (can be tried out here).

Ohhh, that's interesting and new. Thank you so much for sharing all the resources! An year of getting to know about web accessibility and it still keeps surprising me with it's depth.

That would be very helpful in long term, and if it was done before (2), then it would give us even more confidence during refactoring, right?

Indeed. @Pandaa007, I recommend opening an issue for enhancing the test suite for KTable in terms of keyboard navigation first - once the previous PR concerning VTL is merged and then maybe, we can open another issue for the code refactor later on.

Pandaa007 · 2025-03-13T18:21:43Z

This is general checkbox intended behavior, only Space should toggle it, not Enter (can be tried out here). Few references

https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/

https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/examples/checkbox/

Wow nice! I wasn't aware of the same at all! That's for this new information byte!

High-level this makes sense to me @BabyElias and @Pandaa007, yes I welcome a new issue (but not sub-issue of #829, that's concerned with public interface only). Please also post a comment to the issue right away if it's something you wish to work on and reference this comment of mine to ensure it's reserved for you.

Got it @MisRob! I will open a new issue for the same soon and will leave a comment there.

That would be very helpful in long term, and if it was done before (2), then it would give us even more confidence during refactoring, right? #897 slipped my attention, I will try if we can get it in finally.

Thanks! I would love to work on the same on a priority basis as soon as the changes are merged. Thanks!

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

BabyElias self-requested a review March 8, 2025 02:55

BabyElias approved these changes Mar 8, 2025

View reviewed changes

MisRob mentioned this pull request Mar 13, 2025

[DO NOT MERGE] QA for KDS #900 Fixes keyboard navigation in Ktable learningequality/kolibri#13201

Open

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

BabyElias commented Mar 7, 2025 •

edited

Loading

Pandaa007 commented Mar 7, 2025 •

edited

Loading

Pandaa007 commented Mar 7, 2025 •

edited

Loading

MisRob commented Mar 13, 2025 •

edited

Loading

MisRob commented Mar 13, 2025

BabyElias commented Mar 13, 2025 •

edited

Loading

Pandaa007 commented Mar 13, 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

BabyElias commented Mar 7, 2025 • edited Loading

Pandaa007 commented Mar 7, 2025 • edited Loading

Pandaa007 commented Mar 7, 2025 • edited Loading

MisRob commented Mar 13, 2025 • edited Loading

MisRob commented Mar 13, 2025

BabyElias commented Mar 13, 2025 • edited Loading

Pandaa007 commented Mar 13, 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

BabyElias commented Mar 7, 2025 •

edited

Loading

Pandaa007 commented Mar 7, 2025 •

edited

Loading

Pandaa007 commented Mar 7, 2025 •

edited

Loading

MisRob commented Mar 13, 2025 •

edited

Loading

BabyElias commented Mar 13, 2025 •

edited

Loading