-
Notifications
You must be signed in to change notification settings - Fork 328
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
Update form inputs focus to comply with WCAG 2.1 #1312
Conversation
a02bbef
to
70a9014
Compare
70a9014
to
4ace88d
Compare
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.
This is generally looking good, but I think there might be a few minor issues with the file upload component (damnit, Firefox!)
src/components/input/_input.scss
Outdated
|
||
&:focus { | ||
// Double the border by adding its width again. Use `box-shadow` to do | ||
// this instead of changing `border-width` (which changes element size) and |
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 not sure this is true (at least not in all browsers) as inputs have a fixed height. I think this is the case for textareas and file uploads though, so we might be doing this for consistency?
(I've had a little play in a codepen to try it out – https://codepen.io/36degrees/pen/QPeYVN)
Either way it's worth bearing in mind that box-shadows aren't rendered in high contrast mode / with overridden colours, so users of those features won't be able to perceive this. I wonder if there are any other ways we might be able to use a thicker border on textareas without an explicit height that we should be trying? @dashouse is this something you spent time looking at? are you able to provide any additional context?
If we keep using box-shadow, I suggest updating this comment as it may be a little misleading.
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.
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 @36degrees I've updated the comment to more accurately explain what's going on.
I think this branch matches the focus state on @dashouse's branch when colours are overridden
So there is a visible change. @36degrees what do you think?
} | ||
} | ||
|
||
// Set "focus-within" to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1430196 |
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.
Great catch, and excellent comments 👏
border: $govuk-border-width-form-element-error solid $govuk-error-colour; | ||
|
||
&:focus { | ||
border-color: $govuk-input-border-colour; |
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 don't think this is working quite as intended in firefox, I suspect because of the :focus-within
hack.
In Chrome the file input, in error state, when focused, looks like this:
This is consistent with the other form input types – when in an error state, the red border is replaced with a black border.
But in Firefox, you always end up with a 'triple ring' like this:
You also see the triple border in Chrome if you click on the 'Choose file' button (I assume because the button is focused, so this counts as 'focus within' the input.
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.
Fixed. I'm adding the same resets to :focus-within
as to :focus
to prevent the error styles from being applied when the focus is within.
@@ -7,13 +7,66 @@ | |||
@import "../label/label"; | |||
|
|||
@include govuk-exports("govuk/component/file-upload") { | |||
$spacing-unit: govuk-spacing(1); |
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.
Can we think of a more descriptive name for this?
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.
Updated!
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.
Looks good once it has a CHANGELOG entry
b4db471
to
76663f7
Compare
Double the border width on focus using `box-shadow`. Cater for IE8 which doesn't support `box-shadow` by setting `border-width`. The component error style already has a double width border so don’t apply `box-shadow` on focus there.
Double the border width on focus using `box-shadow`. Cater for IE8 which doesn't support `box-shadow` by setting `border-width`. The component error style already has a double width border so don’t apply `box-shadow` on focus there.
Double the border width on focus using `box-shadow`. Cater for IE8 which doesn't support `box-shadow` by setting `border-width`. The component error style already has a double width border so don’t apply `box-shadow` on focus there.
Double the border width on focus using `box-shadow`. Cater for IE8 which doesn't support `box-shadow` by setting `border-width`. The component error style already has a double width border so don’t apply `box-shadow` on focus there. The new focus styles make heavier use of `box-shadow` than the previous style and the styles visually overlap the component. Add some padding (and negative margin) to align element.
Fix an existing undocumented issue where file upload component doesn't display focus indicator in Firefox (in standard or inverted colours mode). Underlying issue reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=1430196 The focus state can be made to display in FF with `focus-within`. Unfortunately `focus-within` cannot be set together with `:focus` (to reduce duplication) as all versions of IE fail to recognise `focus-within` and don't set any styles from the block if `focus-within` is a selector. It's therefore set separately in the SASS. Also manually emulate styles from `govuk-focusable` for file upload in FF. Alternatively `govuk-focusable` mixin could be made to accept a new `$focusable-within` param that adds the `focus-within` styles. However from API point of view this it might be unclear whether the mixin adds both `focus` and `focus-within` styles if the param is set or just one or the other. Alternatively, a new mixin `govuk-focusable-within` mixin could be created but since `focus-within` has poor browser support (no IE or Edge) it seems preferable not to make it part of public API as someone might mistakenly use it instead of `govuk-focusable`.
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.
Just need to resolve conflict but apart from that its looks good to go.
76663f7
to
89efcd9
Compare
Iterate on #1245 to update the GOV.UK Frontend form inputs.
Affected components:
Method:
box-shadow
.box-shadow
by settingborder-width
.box-shadow
on focus there.Also fix an existing bug where the file upload doesn't receive visible focus in Firefox when tabbed into (standard and inverted colours mode).
Tested on:
✅Chrome 74
✅Firefox 66
✅OS Safari 12.1
✅ Android Samsung Galaxy (Chrome)
❗️iOS XS / 8 (Safari) - all good except file upload doesn't show visible focus. This is already present in the current component. We could dig into it a bit more 🤷♀
✅Edge 18
✅Edge 16
✅IE11
❗️ IE 9-10: all good except the file upload component doesn't display the black box-shadow (there's no obvious reason here as the box shadow works for the other components. Suspect it's a native bug we probably shouldn't spent too much time on as the user numbers are very small and the yellow focus outline is still present).
✅IE8
Closes: #1303