-
Notifications
You must be signed in to change notification settings - Fork 333
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 linked image focus style #3819
Conversation
@querkmachine Love this by the way Is it a unique problem for images? Wondering if "block-level" links enclosing content other than images would benefit from this |
Yes and no. These styles could be used to wrap multiple elements in a 'block-level' link, but using links in that way doesn't appear to be typical on GOV.UK, which instead wraps only the title in an Doing that isn't an option if there is only an image with no title to use as a link (thinking something like a gallery?), so on that basis this problem is pretty much unique to images. |
6321283
to
c83acd5
Compare
This is a known issue – see #1936 for more info. |
Would it make sense to update thie error summary component to use the new
I agree. I think the focus style on form controls is probably fine as-is, as it already has the contrasting yellow and black combination we use elsewhere (just with the order inverted). |
42e4ce9
to
48fce0a
Compare
There's been some questions over whether the box-shadow being applied should have yellow or black on the outside, and whether one approach provides more usable contrast or is more internally consistent over the other. CurrentlyCurrently, Frontend has a few different focus styles in use. Form controls, which already have a black border, visually expand the black border and add the yellow on the outside. If the field has an error, the red border is changed to black on focus. Buttons, links and 'link-like' components change the background of the focused element to yellow and apply a black border to the bottom of the element. (In my mind, this is equivalent to putting the black border on the outside.) The notification banner and error summary components forego having a black border completely, only using yellow to indicate the element is focused. However, we already know this does not meet the requirements for non-text contrast and seem unsure whether WCAG's intent is that this should be higher contrast (#1936). To me, the above implies that we have three distinct categories of focus style, each the master of their own little realms:
In the case of linked imagesI would argue that images serving as links should be consistent with the second category, as they are interactive but lack a CSS border when in typical use. This would mean having yellow as the inner colour and black as the outer colour. I also feel that having the yellow on the inside, with black as the outer border, provides a more noticable focus ring around an image when on a white background. This is especially true of iconography—which frequently appears on GOV.UK in black or other dark colours—and visually busy images—such as photographs, where a black inner border has a tendency to 'merge into' the photo more readily than the bright yellow. @CharlotteDowns @christopherthomasdesign @Ciandelle Does the Design Gang have any thoughts on this? |
@querkmachine I have to agree with @davidc-gds on keeping the focus look similar to form controls. We are more likely to be in a position where we know the background colour (on white) than the colours within the image, in the example that we are using for testing the yellow is very visible but what if the image itself had a lot of yellow in it (or was yellow)? I think it's a really great update to see that the focus state is perceivable from both the top and bottom of the image. Playing Devil's advocate here, I'm struggling to see a scenario where an image is focusable without surrounding link text? Perhaps this is only for circumstances when an image is expandable and maybe we need to write some guidance for this 🤔. |
@CharlotteDowns Yeah, I rather doubt there'll be many situations where this actually gets used. It's very much a case of this being a bit of WCAG 2.2 that might not be met in a specific situation and trying to cover it off. |
@querkmachine thanks for writing up this full assessment! @CharlotteDowns I was actually leaning towards linked images having the yellow inner border and black outer border, so that they remain distinct from our inputs. Sorry for not providing more clarity in my original comment:
And back to @querkmachine! I'd like to potentially extend one section of your assessment comment, quoted below:
I may be wrong, but I think our inputs are the only elements that already have the black border. If that's the case, we may want to use that to our advantage as a visual cue that "all things with black borders are inputs". That doesn't mean all inputs need black borders, but likely provides a good visual clue for a majority of our inputs. Based on that assumption that "an un-focussed element with a black border = input", I have a few proposals, which I think mostly match with your musings:
|
@davidc-gds That was my thinking too, though probably only going to raise that once we've come to a decision on what we're doing here. Edit: Never mind, did it anyway! |
I think that the focus states should be as consistent as we can reasonably make them across components. So I would lean towards using black inner and yellow outer lines in most cases. We never finished rationalising all this, but I think there is something about the focus state expanding the visual weight of the thing you're focussing on. So with inputs, the border gets thicker (and you add a yellow border to add contrast for darker backgrounds). I think links are a bit of an exception – we don't surround them with borders because if used within a paragraph it'd crash into the surrounding text. To follow on from the 'adding visual weight to the unfocused thing' point, I think it'd be more consistent to use a black border on the inside of an image, because (I think) most cases will be a photo on a white background. So a darker thing on a lighter thing. So the black border expands the size of the image. I'm generalising here a bit but hopefully you get what I mean. Referencing the banner focus issue in #3870, it could even be that the consistent approach there would be to add weight to the borders on banners when focused. I mean it'd need some tweaking, but something like: I think especially on the banners, having 3 layers of variously contrasting lines creating a striped effect is more visually noisy than you need it to be. Especially for the error summary, which creates a slightly alarming combination of red, yellow and black stripes. Error summary is focussed by default when you land on a page so that state will be what most people see when they get an error. This is all reminding me of why we started an investigation into our focus states ages ago, because it needs looking at holistically. Might be worth a chat @CharlotteDowns to see if we think we can pick up that work again and make any swift conclusions, without having to document it to a publishable standard right away. |
I'm going to risk jumping in in the hopes of bringing about a compromise. This is a bit of a tricky stalemate as we have 2 slightly different opinions drawn from expertise in both @christopherthomasdesign and @CharlotteDowns from a design perspective and @davidc-gds from an accessibility perspective. I'd like to drill down into the reasons behind the opinions. @davidc-gds has outlined the benefits of yellow inside here, which make sense to me:
My read of the benefit of yellow outside is consistency with our focus styles, taken from @christopherthomasdesigns's comment above. Whilst there are a different number of benefits, they're not all weighted the same. I think consistency shouldn't be thrown out as an us-only benefit. I do however think that it's not a robust view to disregard majority dark tone images because most are likely to be lighter tone. However HOWEVER, something worth remembering is that this is a feature for something we don't really advise teams do. It's another question of how much we invest in "protecting" users from teams doing things that aren't good for them. Is creating a "new" style of focus worth that effort? I'm not gonna give an opinion just yet. instead I'd like to prompt those in the thread to either reach a compromise between themselves or agree that this needs more data/deeper thought to come to a decision. If we continue going round in circles then I'm going to be more rigid. PS: I think we should put a pin in conversations about issues with error summary/notification banner focus states and the focus states in general. It sounds like this is a discussion worth having re: our focus states at large but not contained to this PR which has a clear and specific aim. |
Thanks @owenatgov – if I boil my comment down to what's directly relevant to this PR, I agree with your point that it's probably not worth creating a new focus style for this one rare use case. |
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 as the yellow on the inside extends the current version of the focus state I think it's fine to take this approach. When we revisit updating the guidance on focus states we can document all the reasons why it might be reverse to the other input styles.
Add linked image focus style
During WCAG 2.2 research, we found that our current link focus style would not meet §2.4.13 Focus Appearance when applied to images, as it does not provide the minimum 2 pixel perimeter and does not visually enclose the focused image. See #3846.
Although this is a Level AAA criterion, our current focus style arguably fails the Level AA §2.4.11 Focus Not Obscured (Minimum) when applied to images too, as the focus style is only visible at the bottom of the image and may therefore be completely obscured to a user (e.g. the focus style may be entirely outside of the viewport).
This PR adds a new focus style (with associated class and mixins) specifically for contexts where an image is serving as a link, and provides that image with a focus style that visually encloses the entire image. Multiple stacked
box-shadow
s are used to achieve this effect, with the same forced colours mode fallback tooutline
that regular links use.Additionally,
display: inline-block
is set so that the focus style is applied around the entire image, andline-height
is reset to 0 to remove excess whitespace being added below the image.Currently, these new styles only work if the image is the only thing within the anchor. Adding text inside of the anchor is not supported by the provided styles, though may work in combination with other typography classes. Images inside of existing
.govuk-link
links are unaffected and continue to use the old styles.Changes
govuk-focused-box
mixin that contains the core focus styles (to complementgovuk-focused-text
).govuk-link-image
mixin that appliesgovuk-focused-box
on focus, as well as other complementary styling..govuk-link-image
class to expose thegovuk-link-image
mixin.Screenshots
Before (
.govuk-link
)After (
.govuk-link-image
)Thoughts
:has
selector, and support for it just isn't there yet.display
property feels kinda dirty, and potentially frustrating if that is overriding something else (block, flexbox, etc.) This becomes unnecessary if we apply thebox-shadow
/outline
to the<img>
instead of the<a>
, but I'm not sure if there are other side effects to doing this.