Fixes keyboard navigation in Ktable

[Pandaa007]

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]

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]

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]

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

[MisRob]

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

[MisRob]

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.

[Pandaa007]

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]

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]

@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]

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]

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

[MisRob]

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]

@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]

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]

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]

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]

@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]

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]

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]

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]

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