-
Notifications
You must be signed in to change notification settings - Fork 292
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
UI: Enhance navigation and accesibility with keyboard tabindex attributes #1914
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this kind of UI change, you should take before-after screenshots/videos. This is a video that I recorded on the current stable version:
capture.mp4
In this video:
- Terminal input is automatically focused after loading and switching tabs.
- The player can navigate the "Active Scripts" page with tab.
What are your changes? I checked out your branch and ran it, but it's not obvious what the new changes are.
Added tabindex attribute to the terminal input field so players can focus on it with the Tab key.
Can you show it with a before-after screenshot/video?
Implemented keydown handling for the Enter key to execute terminal commands when the input field is focused.
Enabled focus feedback for the terminal to make it visually clear when it is active.
This is your change:
You make the terminal input look "special", but, IMO, it looks confusing. In real terminal UI, I have never seen this kind of "special" UI.
When I looked at it the first time, I thought that it's a UI bug, not a feature.
"No command entered." is also an unnecessary "feature".
Added tabindex attribute to Active Scripts
Same as before, what is the new change?
@@ -70,17 +81,26 @@ export function TerminalInput(): React.ReactElement { | |||
} | |||
} | |||
|
|||
function handleFocus(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are unused. Why is that?
function handleValueChange(event: React.ChangeEvent<HTMLInputElement>): void { | ||
saveValue(event.target.value); | ||
setPossibilities([]); | ||
setSearchResults([]); | ||
setAutofilledValue(false); | ||
} | ||
|
||
function resetSearch(isAutofilled = false) { | ||
function resetSearch(clearFocus?: boolean): void { | ||
//Edited to ensure resetSearch clears all autocomplete and search states |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the comment explains the removal of setAutofilledValue
and other changes, but I still don't get it.
Edited to ensure ...
What was "edited"? Is there a bug that is fixed by this change? Why is it necessary?
Can you rewrite the comment here?
@@ -44,6 +54,7 @@ export function TerminalInput(): React.ReactElement { | |||
const [searchResults, setSearchResults] = useState<string[]>([]); | |||
const [searchResultsIndex, setSearchResultsIndex] = useState(0); | |||
const [autofilledValue, setAutofilledValue] = useState(false); | |||
const [isFocused, setIsFocused] = useState(false); // New state for focus feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick (feel free to ignore it): "New something for something" is an explanation of the reason for adding this new variable. After this change is merged, looking back on this comment is weird. "New state" is not that "new" anymore. Suggestion: "This state is for ...".
Some high-level comments:
|
Per your other PR: You're operating with a single branch branch |
Pull Request Description
Summary
This pull request improves and includes keyboard accessibility for key areas of the Bitburner interface, ensuring that players can navigate menus and trigger actions without relying on a mouse. This aligns with the discusión in issue #464, where it was described the pressence of accessibility-hostile things that could gradually be cleaned up, such as making sure that keyboard navigation works correctly and including functionalities that can be triggered not only by cliking.
Changes
1. Added tabindex attribute to the terminal input field so players can focus on it with the Tab key.
• The tabIndex: 0 in the InputProps ensures the input field is tabbable, allowing it to gain focus when the Tab key is pressed. Normal functionality is not affected and works correctly.
2. Implemented keydown handling for the Enter key to execute terminal commands when the input field is focused.
• If the Enter key is pressed, the onKeyDown function checks if the input field is focused.
• Includes logic to display the entered command and execute it using Terminal.executeCommands.
• For error handling a try-catch was added for robust handling during command execution, so that errors are logged and reported without crashing the terminal.
3. Enabled focus feedback for the terminal to make it visually clear when it is active.
• Added isFocused state to track if the terminal input is active. And handleFocus and handleBlur methods update the focus state.
• Added focusedInput class for active focus styling. This implementation ensures that the terminal visually indicates when it is active.
• Included transition for a smoother visual effect when focus changes. This implementation ensures that the terminal visually indicates when it is active, improving the user experience.
4. Added tabindex attribute to Active Scripts
• This allows users to quickly navigate there using the keyboard. This improves accessibility and makes the interface more efficient and inclusive for keyboard navigation.