Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor handleKeydown Method for Improved Readability and Maintainability #804

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

shivam-daksh
Copy link
Contributor

@shivam-daksh shivam-daksh commented Oct 20, 2024

Description

This PR addresses the issue described in Issue #795. The handleKeydown method in the KTable component has been refactored to improve readability and maintainability while preserving the existing functionality and accessibility of the keyboard navigation.

Issue addressed

#795

Addresses #PR# HERE

Summary

In #727, we introduced a new component—KTable—which provides a flexible, accessible table layout with enhanced keyboard navigation capabilities. As part of this, the handleKeydown method is responsible for managing keyboard navigation between table cells, handling focus, and managing the arrow, tab, and other keys.

The current implementation of the handleKeydown method has grown more complex over time, making it harder to maintain and reason about. The goal of this issue is to refactor the method into a more readable and maintainable format while preserving the functionality and accessibility of the keyboard navigation.

Changes Made

  1. Refactored handleKeydown Method:
    -- Broke down the logic into smaller, modular methods for handling different key events.
    -- Ensured that the Tab key mimics the functionality of the ArrowRight key.
    -- Ensured that the Shift+Tab key mimics the functionality of the ArrowLeft key and moves to the last cell when the first cell is focused.
methods: {
  handleKeydown(event, rowIndex, colIndex) {
    const key = event.key;
    const totalRows = this.rows.length;
    const totalCols = this.headers.length;

    let nextRowIndex = rowIndex;
    let nextColIndex = colIndex;

    switch (key) {
      case 'ArrowUp':
        ({ nextRowIndex, nextColIndex } = this.handleArrowUp(rowIndex, colIndex, totalRows));
        break;
      case 'ArrowDown':
        ({ nextRowIndex, nextColIndex } = this.handleArrowDown(rowIndex, colIndex, totalRows));
        break;
      case 'ArrowLeft':
        ({ nextRowIndex, nextColIndex } = this.handleArrowLeft(rowIndex, colIndex, totalRows, totalCols));
        break;
      case 'ArrowRight':
        ({ nextRowIndex, nextColIndex } = this.handleArrowRight(rowIndex, colIndex, totalRows, totalCols));
        break;
      case 'Enter':
        this.handleEnter(rowIndex, colIndex);
        break;
      case 'Tab':
        ({ nextRowIndex, nextColIndex } = this.handleTabKey(event, rowIndex, colIndex));
        break;
      default:
        return;
    }

    this.updateFocus(nextRowIndex, nextColIndex);
    event.preventDefault();
  },

  handleArrowUp(rowIndex, colIndex, totalRows) {
    let nextRowIndex = rowIndex === -1 ? totalRows - 1 : rowIndex - 1;
    return { nextRowIndex, nextColIndex: colIndex };
  },

  handleArrowDown(rowIndex, colIndex, totalRows) {
    let nextRowIndex = rowIndex === -1 ? 0 : (rowIndex + 1) % totalRows;
    return { nextRowIndex, nextColIndex: colIndex };
  },

  handleArrowLeft(rowIndex, colIndex, totalRows, totalCols) {
    let nextRowIndex = rowIndex;
    let nextColIndex = colIndex;

    if (rowIndex === -1) {
      if (colIndex > 0) {
        nextColIndex = colIndex - 1;
      } else {
        nextColIndex = totalCols - 1;
        nextRowIndex = totalRows - 1;
      }
    } else if (colIndex > 0) {
      nextColIndex = colIndex - 1;
    } else {
      nextColIndex = totalCols - 1;
      nextRowIndex = rowIndex > 0 ? rowIndex - 1 : -1;
    }

    return { nextRowIndex, nextColIndex };
  },

  handleArrowRight(rowIndex, colIndex, totalRows, totalCols) {
    let nextRowIndex = rowIndex;
    let nextColIndex = colIndex;

    if (colIndex === totalCols - 1) {
      if (rowIndex === totalRows - 1) {
        nextColIndex = 0;
        nextRowIndex = -1;
      } else {
        nextColIndex = 0;
        nextRowIndex = rowIndex + 1;
      }
    } else {
      nextColIndex = colIndex + 1;
    }

    return { nextRowIndex, nextColIndex };
  },

  handleEnter(rowIndex, colIndex) {
    if (rowIndex === -1 && this.sortable) {
      this.handleSort(colIndex);
    }
  },

  handleTabKey(event, rowIndex, colIndex) {
    const currentCell = this.getCell(rowIndex, colIndex);

    if (!currentCell) return false;

    const focusableElements = this.getFocusableElements(currentCell);
    const focusedElementIndex = focusableElements.indexOf(document.activeElement);

    if (focusableElements.length > 0) {
      if (!event.shiftKey) {
        if (focusedElementIndex < focusableElements.length - 1) {
          focusableElements[focusedElementIndex + 1].focus();
          event.preventDefault();
          return { nextRowIndex: rowIndex, nextColIndex: colIndex };
        }
      } else {
        if (focusedElementIndex > 0) {
          focusableElements[focusedElementIndex - 1].focus();
          event.preventDefault();
          return { nextRowIndex: rowIndex, nextColIndex: colIndex };
        }
      }
    }

    const totalRows = this.rows.length;
    const totalCols = this.headers.length;
    let nextRowIndex = rowIndex;
    let nextColIndex = colIndex;

    if (!event.shiftKey) {
      // Mimic ArrowRight key
      if (colIndex === totalCols - 1) {
        nextColIndex = 0;
        nextRowIndex = rowIndex === totalRows - 1 ? -1 : rowIndex + 1;
      } else {
        nextColIndex = colIndex + 1;
      }
    } else {
      // Mimic ArrowLeft key
      if (colIndex === 0) {
        if (rowIndex === 0) {
          // Move to the last cell if the first cell is focused
          nextColIndex = totalCols - 1;
          nextRowIndex = totalRows - 1;
        } else {
          nextColIndex = totalCols - 1;
          nextRowIndex = rowIndex > 0 ? rowIndex - 1 : -1;
        }
      } else {
        nextColIndex = colIndex - 1;
      }
    }

    return { nextRowIndex, nextColIndex };
  },

  moveToNextCell(rowIndex, colIndex) {
    const totalRows = this.rows.length;
    const totalCols = this.headers.length;

    let nextRowIndex = rowIndex;
    let nextColIndex = colIndex;

    if (colIndex < totalCols - 1) {
      nextColIndex = colIndex + 1;
    } else if (rowIndex < totalRows - 1) {
      nextColIndex = 0;
      nextRowIndex = rowIndex + 1;
    }

    this.focusCell(nextRowIndex, nextColIndex);
  },

  moveToPreviousCell(rowIndex, colIndex) {
    const totalRows = this.rows.length;
    const totalCols = this.headers.length;

    let nextRowIndex = rowIndex;
    let nextColIndex = colIndex;

    if (colIndex > 0) {
      nextColIndex = colIndex - 1;
    } else if (rowIndex > 0) {
      nextColIndex = totalCols - 1;
      nextRowIndex = rowIndex - 1;
    }

    this.focusCell(nextRowIndex, nextColIndex);
  },

  updateFocus(nextRowIndex, nextColIndex) {
    this.focusCell(nextRowIndex, nextColIndex);
    this.focusedRowIndex = nextRowIndex === -1 ? null : nextRowIndex;
    this.focusedColIndex = nextColIndex;
    this.highlightHeader(nextColIndex);
  }
}

This pull request introduces several enhancements and refactorings to the keyboard navigation and focus management logic in the KTable component. The changes improve the handling of various keyboard events and modularize the code for better readability and maintainability.

Enhancements to keyboard navigation:

  • Enter and Tab Key Handling: Added logic to handle Enter key for sorting columns and Tab key for navigating between focusable elements within a cell. (lib/KTable/index.vue)
  • Arrow Key Navigation: Refactored the logic for ArrowUp, ArrowDown, ArrowLeft, and ArrowRight keys to simplify and enhance navigation between cells. (lib/KTable/index.vue)

Code modularization:

  • Extracted Methods: Created getNextIndexes, handleTabKey, moveToNextCell, moveToPreviousCell, and getFocusableElements methods to modularize and clean up the main event handler logic. (lib/KTable/index.vue)

Changelog

  • Description: Refactored handleKeydown method into smaller, modular methods for handling different key events. Ensured that the Tab key mimics the functionality of the ArrowRight key, and the Shift+Tab key mimics the functionality of the ArrowLeft key. Additionally, pressing Shift+Tab when the first cell is focused now moves the focus to the last cell (bottom-right cell).
  • Products impact: none
  • Addresses: [KTable]: Refactoring the handleKeydown method in KTable #795
  • Components: KTable
  • Breaking: no
  • Impacts a11y: yes
  • Guidance: No special guidance required for consumers.

Steps to test

  1. Step 1
  2. Step 2
  3. ...

(optional) Implementation notes

At a high level, how did you implement this?

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
Copy link
Contributor Author

Hi @MisRob & @BabyElias, kindly review it.
I've made slight changes in the functionality of tab and shift+tab key. Basically, these are mimicing the functionality of left arrow and right arrow key respectively.

@BabyElias
Copy link
Contributor

@shivam-daksh Can you please update the Changelog in description with all the fields that were originally in the template?
You can see the Changelog here for reference.

Also, please run the yarn lint fix command to fix linting issues with the PR. Thanks :)

@shivam-daksh
Copy link
Contributor Author

shivam-daksh commented Oct 20, 2024

@BabyElias, I've updated the changelog section in description. Please have a look at it.
I ran yarn lint fix command but it didn't made any changes to the code formatting.

@BabyElias
Copy link
Contributor

@shivam-daksh The Changelog looks good 👍

For the linter - maybe try yarn install --force then yarn lint-fix again, perhaps it is an issue with your currently installed version of kolibri-tools (where the linter lives). However, from the issues that I can see with the failed linter test, it is caused due to declaration of unused variables and the linter can't fix these issues on its own (it can fix alignment on its own, but has issues with code refactor related issues like these). So, this change needs to be manually done by you for the tests to pass. You can have a look at the issues after execution of yarn lint-fix in the console itself. Hope this helps .

@shivam-daksh
Copy link
Contributor Author

Hey @BabyElias, I've just passed the linting test by commenting the unsued code. Could you please review it now?

@BabyElias
Copy link
Contributor

BabyElias commented Oct 22, 2024

Hey @shivam-daksh , good work overall :)
I had a few pointers related to keyboard nav features that existed earlier and have now been affected by the refactor :

  • Shift+Tab key combination does not focus on focusable elements within the cell (while tab key does)
  • Using arrow keys, the corresponding cell header and entire cell get highlighted (as expected). The same is supposed to happen when the tab and Shift+Tab keys are used, but that does not happen.

Don't know if you have an idea about the playground page yet to test your changes or not, you can have a look at it here. This comes in extremely handy to preview the output for code changes as and when they are done.
I would highly recommend testing your keyboard navigation features by copy-pasting this playground page in your local repository. This playground page contains example for the table with focusable elements as well, that can help you better in reviewing the code.
Remember to not commit playground page changes, just keep them for local reference and testing. Thanks!

@shivam-daksh
Copy link
Contributor Author

Hi @BabyElias , Tab and Shift+Tab key is working as expected. Please take a look.

@BabyElias
Copy link
Contributor

Hey @shivam-daksh :)
The row and header highlight work just perfect now, kudos on that!
A few issues that I can observe here though,

  • Using tab key, we are able to focus on the focusable elements within the cell but using Shift+tab key while moving backwards it the table, we cannot. This needs to be fixed.
  • The observation, that Tab Key and Shift+Tab key mimic left and right arrow keys is valid when navigating inside the table, however, Tab and Shift+Tab key are originally meant to move from one element to other according to the DOM order. This means, that once a user reaches the end of the table (bottom rightmost cell), the tab key should function as it's default and move out of the table to the next element within DOM order (unlike arrow keys that keep moving within the table). Similarly, for Shift+Tab key in the opposite order. This doesn't need to be engineered manually as such- you just have to enable default tab behaviour when reaching end of table, you can see how it was implemented in the previous code and take inspiration accordingly.

Thanks!

@shivam-daksh
Copy link
Contributor Author

Hey @BabyElias , Please review it again.

@BabyElias
Copy link
Contributor

BabyElias commented Oct 25, 2024

@shivam-daksh The Tab key behaviour is as expected now, great job there!
I am still unable to focus elements using Shift+Tab key combo, not sure if you have fixed that or are yet to do the same.

Also, I was having a look at the code, and I could see that a number of comments that existed earlier have been removed by the new commits. We try to keep our code as well documented and easy to understand as possible so that it can be easier for future contributors to make sense of the code-base and contribute in a better way, especially with something as complicated as the handlekeydown method which might not be understandable for everyone. So, it would be really helpful if you could add a few of those comments back in the further commits or maybe write something of your own to explain the newly done changes :)

@shivam-daksh
Copy link
Contributor Author

Hi @BabyElias, Thanks for the reminding! I’ve re-added the lost comments during the refactoring to help clarify the code for future contributors.

Regarding the Shift+Tab functionality, I’m a bit unclear on how it should be working differently. As far as I can tell, its behavior remains the same as before the refactor. If you could provide a little visual help or specific examples of what you expect, that would really help me address the issue effectively!

Looking forward to your input!

Screen.Recording.2024-10-27.at.3.59.11.PM.mov

@BabyElias
Copy link
Contributor

@shivam-daksh,
About the Shift+Tab key, the KTable example in the previous video does not contain any focusable elements within the cell (like a button, the example for such a table can be found in the playground page I referenced in an earlier comment). So, if you use that example, then the way Tab key works in the forward navigation - Shift+Tab key is expected to work in the same way in backward direction.
For eg - if we have three cells in a row in which third one has a button, the order of navigation moves as
Tab key : cell1->cell2->cell3->button in cell3->next row, first cell
Shift+Tab key : next row,first cell -> button in cell3->cell3->cell2->cell1
Basically, the button should be focussed using both tab key and shift+tab keys.
Hope this makes it a little clearer than before, in case you still find any difficulty in understanding - I'll also record a video demo for the same. Currently, have some technical issues with doing that so :|

@shivam-daksh
Copy link
Contributor Author

@BabyElias, Okay. I'm sorry. Totally misunderstood that 😆 .
I'm on it.

@MisRob
Copy link
Member

MisRob commented Nov 6, 2024

Thanks @shivam-daksh for this work and @BabyElias for taking care of the review, we really appreciate it :)

@MisRob
Copy link
Member

MisRob commented Nov 6, 2024

Okay. I'm sorry. Totally misunderstood that 😆 .

No worries, @shivam-daksh. The table has many features that are not immediately obvious, we're currently discussing how to document technical requirements, including a11y, in detail, to guide future work on more complex components. Meanwhile, fortunately @BabyElias and @radinamatic have everything in their mind.

@MisRob
Copy link
Member

MisRob commented Nov 13, 2024

Hi @shivam-daksh, no time pressure around this task, just checking in if there's anything you'd need since it seems some aspects are a bit tricky.

I also noted @BabyElias's

Hope this makes it a little clearer than before, in case you still find any difficulty in understanding - I'll also record a video demo for the same. Currently, have some technical issues with doing that so :|

and if that'd help, I could try to do the recording for you

@shivam-daksh
Copy link
Contributor Author

@BabyElias and @MisRob, I am really sorry for extending that long. My exams are going on. However, I will finish it at end of this week.
Also, I understand the problem and have figured out how to properly refactor it. Thanks to @BabyElias for the explaination.

@MisRob
Copy link
Member

MisRob commented Nov 13, 2024

@BabyElias and @MisRob, I am really sorry for extending that long. My exams are going on. However, I will finish it at end of this week.
Also, I understand the problem and have figured out how to properly refactor it. Thanks to @BabyElias for the explaination.

@shivam-daksh Really please take all the time you need - it's absolutely fine for volunteers to work on tasks for longer periods of time - weeks, even months. Most of our 'help wanted' are intentionally not time sensitive, and for those that are, we'd inform volunteers beforehand.

You're generously volunteering your time and energy and there's really no need to make it into feeling bad! I hope you can focus on your exams and don't forget to get plenty of rest ;)

I'm glad to hear that the expectations are clearer. Thanks @BabyElias for all the guidance for @shivam-daksh.

@shivam-daksh
Copy link
Contributor Author

Hey @BabyElias, please have a look at this.

@BabyElias
Copy link
Contributor

Hey @shivam-daksh, so happy to see all the pieces finally put together!
The functionality is almost completely implemented now, I'll make a few changes and add them as review here so that you can also understand the few parts that remain. You can just commit them and the code will be good to go :)

lib/KTable/index.vue Outdated Show resolved Hide resolved
lib/KTable/index.vue Outdated Show resolved Hide resolved
lib/KTable/index.vue Outdated Show resolved Hide resolved
lib/KTable/index.vue Outdated Show resolved Hide resolved
lib/KTable/index.vue Show resolved Hide resolved
lib/KTable/index.vue Show resolved Hide resolved
@MisRob MisRob requested a review from AlexVelezLl November 20, 2024 15:34
@MisRob
Copy link
Member

MisRob commented Nov 20, 2024

Wonderful work here @shivam-daksh and @BabyElias.

As agreed with @BabyElias, @AlexVelezLl will join us here to offer second eyes for the code as soon as all @BabyElias's feedback is resolved. I will then prepare test environment for our QA team in Kolibri.

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thank you @shivam-daksh! This is looking good. I have left some minor comments on small things I noticed. Please let me know if you have any questions :)

lib/KTable/index.vue Outdated Show resolved Hide resolved
lib/KTable/index.vue Outdated Show resolved Hide resolved
lib/KTable/index.vue Outdated Show resolved Hide resolved
const nextCell = this.getCell(nextRowIndex, nextColIndex);
const nextFocusableElements = this.getFocusableElements(nextCell);
const nextCellAndFocusableElements = [nextCell, ...nextFocusableElements];
nextCellAndFocusableElements[0].focus();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this nextCellAndFocusableElements if we always will be just focusing the index 0? We dont do anything with this array after that. That means that we will be always focusing the nextCell. We dont need to query the focusable elments of the next cell.

Copy link
Member

Choose a reason for hiding this comment

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

Btw in the updateFocusState we are already focusing the next cell, so there is no need of any of these lines:

            const nextCell = this.getCell(nextRowIndex, nextColIndex);
            const nextFocusableElements = this.getFocusableElements(nextCell);
            const nextCellAndFocusableElements = [nextCell, ...nextFocusableElements];
            nextCellAndFocusableElements[0].focus();

lib/KTable/index.vue Show resolved Hide resolved
@AlexVelezLl
Copy link
Member

Thank you @shivam-daksh! Code changes looks good to me! I will not yet approve this PR since @MisRob will check this with our QA team, but we can have my review as ✔️. I noticed that the issue description mentioned that we needed to add unit tests to this part of the code. I am not sure if you already discussed about this with @BabyElias or @MisRob (I didnt read the whole discussion). But pinging them in case the unit tests are required c:. Thank you!

@BabyElias
Copy link
Contributor

The code changes do not introduce any such new functionalities that might need to be covered by new unit tests. So, seems good to go from my end too :)

@MisRob
Copy link
Member

MisRob commented Dec 4, 2024

Thanks all of you, I will get ready the test Kolibri PR for QA

@MisRob MisRob mentioned this pull request Dec 4, 2024
7 tasks
@MisRob
Copy link
Member

MisRob commented Dec 4, 2024

@shivam-daksh would you please merge the latest develop to this branch? (this will pull in #824 I've just merged and we will be sure we have the very latest KTable before QA). I could also do it later on if you don't mind I pushed to your branch.

@shivam-daksh
Copy link
Contributor Author

@MisRob, I've merged the develop into our current branch. The code is runnnig fine and table is working properly but having a warning in terminal. This happened after merge.

js

 ERROR  [Vue warn]: Invalid prop: custom validator check failed for prop "headers".                                  10:59:01

found in

---> <KTable> at lib/KTable/index.vue
       <Playground> at docs/pages/playground/index.vue
         <Nuxt>
           <.nuxt/layouts/default.vue> at .nuxt/layouts/default.vue
             <Root>

@BabyElias
Copy link
Contributor

BabyElias commented Dec 5, 2024

That is just a validator warning since a new PR merged for KTable mandates having a columnID for each header row. Nothing blocking.

@MisRob
Copy link
Member

MisRob commented Dec 6, 2024

Test PR in Kolibri open here learningequality/kolibri#12914. If QA confirms no regressions, we can merge this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants