-
Notifications
You must be signed in to change notification settings - Fork 45
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
Keyboard tab navigation #921
Conversation
Added keyboard tab navigation. Selected item/element is highlighted with an orange outline/border. All elements should be accessible via keyboard (except for the svg-icon at the moment, still in progress)
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.
THANKS for being the person that finally tackles this :D
While this is certainly already an improvement over the current status, I noticed some things and left some comments. If you have any more questions about those, just ask.
src/style/global-style.js
Outdated
/* Remove the focus indicator on mouse-focus for browsers | ||
that do support :focus-visible */ | ||
outline: none !important; | ||
background: transparent; |
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.
Here are a few things I found in addition to Lukas' review: studio-tab-navigation.mp4 |
Changes: - Highlight-Color changed - every element should now be reachable and selectable/clickable via keyboard - added a few tabindex=-1 for elements, that should not be reachable via keyboard (and are also not clickable via mouse)
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.
Regarding functionality, this is much better. I think everything can be used now with keyboard alone. And no elements are moving around anymore, so that's also good! The remaining comments are just about the design/style.
One major bug is that the buttons, when focused, are still always green (even if they were white or red before). Not sure why that happens, but it should be fixed.
Regarding design, my suggestion would be:
- Remove the
box-shadow
completely (I know, I suggested it in the first place :P) - For the links in the header (recording, settings, about), I would set
outline: orange solid 3px
andoutline-offset: -3px
. Fororange
you can choose a more specific tone, but I think literallyorange
works quite well, too. - For the settings cog icon button to open video settings, maybe pick a specific style as well. Maybe that's too subtle without the box-shadow?
Of course, this means we have either a dark green outline or an orange one, depending on the element. But personally, I don't think that's a problem. And it's certainly good to see IMO!
src/style/global-style.js
Outdated
|
||
*:focus-visible { | ||
outline: 2px solid #286244 !important; | ||
box-shadow: 0 0 5px 3px inset #8ec8aa !important; |
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.
I think I'm not a fan of the blur :/ I would use a blur of 0. But also, apparently I was wrong and outline
works fine with border-radius. So maybe there is no use for box-shadow at all anymore.
src/style/global-style.js
Outdated
} | ||
|
||
*:focus-visible { | ||
outline: 2px solid #286244 !important; |
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.
src/style/global-style.js
Outdated
|
||
|
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 two empty lines could be removed I guess.
src/ui/studio/video-setup/prefs.js
Outdated
@@ -422,7 +427,7 @@ const RadioButton = ({ id, value, checked, name, onChange, label, state }) => { | |||
}, | |||
}} | |||
/> | |||
<label htmlFor={id}>{ label || value }</label> | |||
<label tabIndex={isExpanded ? '0' : '-1'} onKeyDown={e => (e.key === 'Enter' || e.key === ' ' ) && onChange(value)} htmlFor={id}>{ label || value }</label> |
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.
That's a long line, I would break it up.
<label tabIndex={isExpanded ? '0' : '-1'} onKeyDown={e => (e.key === 'Enter' || e.key === ' ' ) && onChange(value)} htmlFor={id}>{ label || value }</label> | |
<label | |
tabIndex={isExpanded ? '0' : '-1'} | |
onKeyDown={e => (e.key === 'Enter' || e.key === ' ' ) && onChange(value)} | |
htmlFor={id} | |
>{ label || value }</label> |
- removed box-shadow - adjusted colors for dark and light background - adjusted colors for buttons
Logo is now accessible via keyboard. Small bug fixed (navigation bar could be controlled via keyboard during recording.)
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.
I really like the design now. I think it's usable and looks good.
Only one last thing: the button color on hover. I'd suggest:
-
Remove
background-color: #f5f5f5 !important;
-
In
theme.js
, on roughly line 85 (buttons
->danger
), behind&:not(:disabled):hover
, add,&:not(:disabled):focus
. So that it reads:'&:not(:disabled):hover,&:not(:disabled):focus': { bg: darken('error', 0.03) }
-
Do the same for line roughly 94 (
buttons
->text
)
This is the bug that made the danger button background green. It was introduced in #902. It would be great if you could fix it!
And then I'm very happy with this.
- removed background-color from global-style.js - added '&:not(:disabled):focus' to buttons and text in theme.js
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.
I'm happy now ^_^ Thanks again.
Added keyboard tab navigation.
Selected item/element is highlighted with an orange outline/border.
All elements should be accessible via keyboard (except for the svg-icon at the moment, still in progress)