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.

Sign up for GitHub

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
Open

Refactor handleKeydown Method for Improved Readability and Maintainability #804

Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a4188c8
refactor the handlekeydown code
shivam-daksh Oct 20, 2024
1bb1471
lint fix by commenting the unused code
shivam-daksh Oct 21, 2024
75f07a2
row highlight fix when tab key pressed
shivam-daksh Oct 23, 2024
6163a15
Fix the infinite loop of Tab key over table cells
shivam-daksh Oct 25, 2024
c518276
adding the lost comments during refactoring
shivam-daksh Oct 27, 2024
f41e333
Merge branch 'learningequality:develop' into refactor-handlekeydown
shivam-daksh Nov 15, 2024
b6f712e
fixing the tab and shift+tab key functionality
shivam-daksh Nov 16, 2024
0708628
highlight rows and columns
shivam-daksh Nov 17, 2024
fff71d4
tab and shift+tab working but first cell of table not getting focused
shivam-daksh Nov 17, 2024
4b61140
Merge branch 'refactor-handlekeydown' of https://github.com/shivam-da…
shivam-daksh Nov 17, 2024
59e33b9
sort the columns only from header row
shivam-daksh Nov 20, 2024
0f5bd96
utilize updateFocusState function in arrow keys handling
shivam-daksh Nov 20, 2024
354df74
change parameter name of updateFocusState function
shivam-daksh Nov 20, 2024
021bf28
modify the shift+tab conditions
shivam-daksh Nov 20, 2024
c153871
replace ternary if-else for readability
shivam-daksh Nov 23, 2024
e84bb62
change tab behavior comment
shivam-daksh Nov 23, 2024
ed52699
remove redundant code from tab functionality
shivam-daksh Nov 23, 2024
15d86e5
Merge remote-tracking branch 'origin/develop' into refactor-handlekey…
shivam-daksh Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 104 additions & 116 deletions lib/KTable/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,156 +294,144 @@
* - header highlight
*/
handleKeydown(event, rowIndex, colIndex) {
const key = event.key;
switch (event.key) {
case 'ArrowUp':
case 'ArrowDown':
case 'ArrowLeft':
case 'ArrowRight':
this.handleArrowKeys(event.key, rowIndex, colIndex);
break;
case 'Enter':
this.handleEnterKey(rowIndex, colIndex);
break;
case 'Tab':
this.handleTabKey(event, rowIndex, colIndex);
break;
default:
break;
}
},

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

const lastRowIndex = totalRows - 1;
const lastColIndex = totalCols - 1;
let nextRowIndex = rowIndex;
let nextColIndex = colIndex;

switch (key) {
case 'ArrowUp':
if (rowIndex === -1) {
nextRowIndex = totalRows - 1;
} else {
nextRowIndex = rowIndex - 1;
}
nextRowIndex = rowIndex === -1 ? lastRowIndex : rowIndex - 1;
break;
case 'ArrowDown':
if (rowIndex === -1) {
nextRowIndex = 0;
} else if (rowIndex === totalRows - 1) {
nextRowIndex = -1;
} else {
nextRowIndex = (rowIndex + 1) % totalRows;
}
nextRowIndex = rowIndex === -1 ? 0 : rowIndex === lastRowIndex ? -1 : rowIndex + 1;
shivam-daksh marked this conversation as resolved.
Show resolved Hide resolved
break;
case 'ArrowLeft':
if (rowIndex === -1) {
if (colIndex > 0) {
nextColIndex = colIndex - 1;
} else {
nextColIndex = totalCols - 1;
nextRowIndex = totalRows - 1;
}
} else if (colIndex > 0) {
nextColIndex = colIndex - 1;
nextColIndex = colIndex > 0 ? colIndex - 1 : lastColIndex;
nextRowIndex = colIndex === 0 ? lastRowIndex : -1;
} else {
nextColIndex = totalCols - 1;
nextRowIndex = rowIndex > 0 ? rowIndex - 1 : -1;
nextColIndex = colIndex > 0 ? colIndex - 1 : lastColIndex;
nextRowIndex = colIndex === 0 ? (rowIndex > 0 ? rowIndex - 1 : -1) : rowIndex;
shivam-daksh marked this conversation as resolved.
Show resolved Hide resolved
}
break;
case 'ArrowRight':
if (colIndex === totalCols - 1) {
if (rowIndex === totalRows - 1) {
nextColIndex = 0;
nextRowIndex = -1;
} else {
nextColIndex = 0;
nextRowIndex = rowIndex + 1;
}
if (colIndex === lastColIndex) {
nextColIndex = 0;
nextRowIndex = rowIndex === lastRowIndex ? -1 : rowIndex + 1;
} else {
nextColIndex = colIndex + 1;
}
break;
case 'Enter':
if (rowIndex === -1 && this.sortable) {
this.handleSort(colIndex);
}
break;
case 'Tab': {
// Identify all focusable elements inside the current cell
const currentCell = this.getCell(rowIndex, colIndex);
}
this.updateFocusState(nextRowIndex, nextColIndex);
event.preventDefault();
},

// Collect focusable elements using native DOM methods
const focusableElements = [];
handleEnterKey(rowIndex, colIndex) {
if (rowIndex === -1 && this.sortable) {
this.handleSort(colIndex);
}
},

handleTabKey(event, rowIndex, colIndex) {
const totalRows = this.rows.length;
const totalCols = this.headers.length;
let nextRowIndex = rowIndex;
let nextColIndex = colIndex;

if (currentCell) {
const buttons = currentCell.getElementsByTagName('button');
const links = currentCell.getElementsByTagName('a');
const inputs = currentCell.getElementsByTagName('input');
const selects = currentCell.getElementsByTagName('select');
const textareas = currentCell.getElementsByTagName('textarea');
const currentCell = this.getCell(rowIndex, colIndex);
const focusableElements = this.getFocusableElements(currentCell);
const focusedElement = document.activeElement;

focusableElements.push(...buttons, ...links, ...inputs, ...selects, ...textareas);
}
// Include the cell itself as the first focusable element
const cellAndFocusableElements = [currentCell, ...focusableElements];
const focusedIndex = cellAndFocusableElements.indexOf(focusedElement);

const focusedElementIndex = focusableElements.indexOf(document.activeElement);
if (focusableElements.length > 0) {
if (!event.shiftKey) {
// if navigating between more focusable elements within the cell
if (focusedElementIndex < focusableElements.length - 1) {
focusableElements[focusedElementIndex + 1].focus();
event.preventDefault();
return;
} else {
if (colIndex < totalCols - 1) {
nextColIndex = colIndex + 1;
} else if (rowIndex < totalRows - 1) {
nextColIndex = 0;
nextRowIndex = rowIndex + 1;
} else {
// Allow default behavior when reaching the last cell
return;
}
}
} else {
if (focusedElementIndex < focusableElements.length - 1) {
// if navigating between more focusable elements within the cell
focusableElements[focusedElementIndex + 1].focus();
event.preventDefault();
return;
} else {
if (colIndex > 0) {
nextColIndex = colIndex - 1;
} else if (rowIndex > 0) {
nextColIndex = totalCols - 1;
nextRowIndex = rowIndex - 1;
} else {
// Allow default behavior when reaching the first cell
return;
}
}
}
if (!event.shiftKey) {
// Tab key navigation
if (focusedIndex < cellAndFocusableElements.length - 1) {
// Move to the next focusable element within the cell
cellAndFocusableElements[focusedIndex + 1].focus();
event.preventDefault();
return;
} else {
// Move to the first focusable element of the next cell
shivam-daksh marked this conversation as resolved.
Show resolved Hide resolved
if (colIndex < totalCols - 1) {
nextColIndex = colIndex + 1;
} else if (rowIndex < totalRows - 1) {
nextColIndex = 0;
nextRowIndex = rowIndex + 1;
} else {
if (!event.shiftKey) {
if (colIndex < totalCols - 1) {
nextColIndex = colIndex + 1;
} else if (rowIndex < totalRows - 1) {
nextColIndex = 0;
nextRowIndex = rowIndex + 1;
} else {
// Allow default behavior when reaching the last cell
return;
}
} else {
if (colIndex > 0) {
nextColIndex = colIndex - 1;
} else if (rowIndex > 0) {
nextColIndex = totalCols - 1;
nextRowIndex = rowIndex - 1;
} else {
// Allow default behavior when reaching the first cell
return;
}
}
return; // Allow default Tab behavior when reaching the end
}

break;
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();

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

default:
} else {
// Shift+Tab key navigation
if (focusedIndex < cellAndFocusableElements.length - 1) {
// Move to the previous focusable element within the cell
cellAndFocusableElements[focusedIndex + 1].focus();
event.preventDefault();
shivam-daksh marked this conversation as resolved.
Show resolved Hide resolved
return;
} else {
// Move to the last focusable element of the previous cell
if (colIndex > 0) {
nextColIndex = colIndex - 1;
} else if (rowIndex > 0) {
nextColIndex = totalCols - 1;
nextRowIndex = rowIndex - 1;
} else {
return; // Allow default Shift+Tab behavior when at the beginning
}
const prevCell = this.getCell(nextRowIndex, nextColIndex);
const prevFocusableElements = this.getFocusableElements(prevCell);
const prevCellAndFocusableElements = [prevCell, ...prevFocusableElements];
prevCellAndFocusableElements[prevCellAndFocusableElements.length - 1].focus();
this.updateFocusState(nextRowIndex, nextColIndex);
shivam-daksh marked this conversation as resolved.
Show resolved Hide resolved
event.preventDefault();
}
}

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

this.highlightHeader(nextColIndex);
this.focusCell(nextRowIndex, nextColIndex);
},
shivam-daksh marked this conversation as resolved.
Show resolved Hide resolved

event.preventDefault();
getFocusableElements(cell) {
if (!cell) return [];
const selectors = 'button, a, input, select, textarea';
return Array.from(cell.querySelectorAll(selectors));
},

getCell(rowIndex, colIndex) {
if (rowIndex === -1) {
return this.$refs[`header-${colIndex}`][0];
Expand Down