-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add default focus outline to buttons #4679
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7770968
to
9bd81da
Compare
9bd81da
to
32b7290
Compare
32b7290
to
fba1089
Compare
001cb98
to
5409da5
Compare
a31ab34
to
bd13596
Compare
There were no elements matching this selector since commit d679c1e and these styles were completely ignored. I'm re-adding the ones with make sense in my humble opinion. I'm not adding top and bottom paddings since they affect the way the height of the element is calculated, and am not sure about the intention behind setting the height property.
We were using a focus outline on links, but weren't doing the same for buttons. Since sometimes browsers use a default outline which is barely visible, this was very disorienting when browsing using the keyboard; we were navigating through links that clearly indicated where the keyboard focus was, and when reaching a button suddenly we had this almost imperceptible feedback. Even if I'm used to it, my first reaction is always "where did the focus go?" until I realize it's now on a button. This is even more confusing because we've got buttons looking like links and links looking like buttons. Note that in the rules for the `:focus` styles we're including buttons and the `[type="button"]` attribute. This seems redundant since those styles are already covered by the `button` selector. However, Foundation adds styles to buttons with the `[type]` attribute. Since the attribute selector has precedence over the tag selector, we need to use the attribute selector as well in order to override Foundation's styles.
We lost focus styles on certain controls in commit 4dad04a, since we were applying a border with a rule which had more precedence than the rule of border on focus.
Styles on keyboard focus are essential for keyboard navigation; without them, keyboard users wouldn't see which element they're currently interacting with. That's why we use an `outline` on elements having the current keyboard focus. However, this is sometimes annoying for mouse/touchscreen users, since clicking/touching an element also gives it keyboard focus. When clicking on a button performing some kind of action through JavaScript, keeping the outline on the button after clicking it is distracting. Even after clicking a link, for some users having the outline present while they wait for the next page to load is annoying. That's why modern browsers (at the time of writing, 74%) implement the `:focus-visible` pseudoclass, which selects elements which have received focus using the keyboard, but not elements which have been clicked/tapped on. We can use it to provide focus styles for keyboard users without getting in the way of mouse/touchscreen users. Usually we wouldn't use a feature which isn't supported in more than 96% of the browsers out there. However, in this case we've got a solid fallback: we just use the `:focus` pseudoclass. Since the `@support` CSS condition doesn't accept pseudoclasses as parameters, we're disabling `:focus` styles only on browsers supporting the `:focus-visible` pseudoclass using the `:focus:not(:focus-visible)` selector, which will be ignored by browsers without support for `:focus-visible`. Since now users receive less feedback when clicking/touching a link or a button, we're adding styles for the `:active` pseudoclass. This way users will know which item they're clicking/tapping on. I'm not sure the outline is a good option for this case, though; I think for touchscreen users a better solution would be to apply the styles we apply on hover. We might change it in the future. Note grouping styles together like this would *not* work: ``` &:focus, &:focus-visible { // Styles here } ``` Browsers not supporting the `:focus-visible` pseudoclass would ignore this statement completely, meaning they wouldn't apply the styles on `:focus` either.
taitus
approved these changes
Sep 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Visual Changes
Before these changes
After these changes