-
Notifications
You must be signed in to change notification settings - Fork 331
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 links (and things that look like links) to use the new focus style #1309
Conversation
src/components/details/_details.scss
Outdated
color: $govuk-focus-text-colour; | ||
background: $govuk-focus-colour; | ||
.govuk-details__summary:hover { | ||
color: $govuk-link-hover-colour; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -10,7 +10,7 @@ | |||
|
|||
@mixin govuk-link-common { | |||
@include govuk-typography-common; | |||
@include govuk-focusable-fill; | |||
@include govuk-focusable-text-link; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/helpers/_focusable.scss
Outdated
// backgrounds and box-shadows disappear, so we need to ensure there's a | ||
// transparent outline which will be set to a visible colour. | ||
|
||
// Since Internet Explorer 8 does not support box-shadow, we want to force the user-agent outlines |
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.
Should we consider adding e.g. a black border all round for IE8? Or are we happy with just user agent defaults?
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.
Adding a black border may make reading the text difficult, so the user-agent styles (screenshot) above seemed better. Happy to go with whatever people think is best.
c983460
to
a3db9e5
Compare
text-decoration: none; | ||
} | ||
|
||
@mixin govuk-focusable-text-link { |
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 mixin seems to be performing a task very similar to @mixin govuk-focusable-fill
. Do we need both? It would be good to have a comment to indicate what the difference between the two is.
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.
As discussed on Slack we're going to wait until we have everything merged before deciding how the mixins should look.
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.
👍
7ed7fe4
to
822c5f9
Compare
822c5f9
to
d0a1965
Compare
This approach uses box-shadow, which has broadly good support across browser. We have found that Edge and Internet Explorer produce rendering issues. Edge 18 has quite problematic issues but this can be seen on any website using custom focus styles. Edge 18 supports 'drop-shadow()' which can work but only on elements that are `diplay: inline`, with this in mind we think the complexity is not worth the cost to users for all browsers. Edge 19+ will be using the Chromium rendering engine so this will not be an issue. Edge 17 and below, and IE 9-11 have lesser issues that we have determined will not cause a barrier. See: https://gist.github.com/nickcolley/3788eefd34e7f04391de07867a85018b for an example of how `drop-shadow()` could work. Co-authored-by: Nick Colley <nick.colley@digital.cabinet-office.gov.uk>
…ixin Co-authored-by: Nick Colley <nick.colley@digital.cabinet-office.gov.uk>
Co-authored-by: Nick Colley <nick.colley@digital.cabinet-office.gov.uk>
We can remove a lot of the custom focus styles since they were relevant to the relationship between the background and outline which are not used anymore. Co-authored-by: Nick Colley <nick.colley@digital.cabinet-office.gov.uk>
When we updated the the focus styles for to have the same colour as the body text, we forgot to update the specific overrides we need when using GOV.UK Frontend with GOV.UK Template (legacy). This meant that when you focused a link in the error summary the color was red instead of black, removing this block achieves the result we want.
d0a1965
to
eca6ca0
Compare
@@ -5,7 +5,8 @@ | |||
@include govuk-exports("govuk/component/skip-link") { | |||
.govuk-skip-link { | |||
@include govuk-visually-hidden-focusable; | |||
@include govuk-link-common; | |||
@include govuk-typography-common; | |||
@include govuk-focusable-fill; |
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.
Why is this using govuk-focusable-fill and why is the skip link styled differently to other links?
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 a design decision from @dashouse to not use the heavy underline focus state for skip-link.
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.
🤔 ok, currently this component and Footer are the only two components still using govuk-focusable-fill
mixin. When we come to review the public mixing perhaps we can just hard code the mixins output here instead of having it hanging around
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.
Yeah once we've got everything production ready we can (after the audit potentially) decide on how the mixins should work, with the new guidance in mind...
Looks good from a code POV, I'm now going to run through a quick xbrowser test before approving |
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.
Implementation of the focus iteration.
Updates components and styles that mostly look like 'links' including:
Component changes
Screenshots
Links
Before:
After:
Legacy:
Back link
Before:
After:
Legacy:
Breadcrumbs
Before:
After:
Legacy:
Details
Before:
After:
Legacy:
Skip link
Before:
After:
Legacy:
Error summary links
Before:
After:
Legacy:
Testing notes
Internet explorer 8 does not have support for box-shadow, to meet the WCAG 2.1 requirements we intentionally unset the outline and allow the user-agent style to appear which looks like the following:
We've found some rendering issues with Edge 18 and Internet Explorer 11
Edge 18
You can see that in Edge 18, all custom focus styles are broken, the following example is of www.GOV.UK that is not using our new focus styles.
This is how the new focus styles look in Edge 18:
Internet Explorer 11
According to this, https://en.wikipedia.org/wiki/Microsoft_Edge#Chromium_(2019%E2%80%93present) the chromium version of Edge is going to be fall this year
You can see for a browser like Chrome, that it takes maybe 2-3 months before most people migrate to the latest version http://gs.statcounter.com/browser-version-market-share
So we're looking at Edge being fixed end of the year, early next year.
For Internet Explorer / Edge traffic:
It's dropped by around 1million users a month in a year, but is pretty stable
Overall IE11 and Edge 18 will have around 13% share of our users for at least 7-8months, if not more.
Edge 18 has quite problematic issues but this can be seen on any website using custom
focus styles. Edge 18 supports 'drop-shadow()' which can work but only on elements that are
display: inline
it also doesnt work with users who change their colours, with this in mind we think the complexity is not worth the cost to users for all browsers.Edge 19+ will be using the Chromium rendering engine so this will not be an issue.
Edge 17 and below, and IE 9-11 have lesser issues that we have determined will not cause a barrier.
See: https://gist.github.com/nickcolley/3788eefd34e7f04391de07867a85018b for an example of how
drop-shadow()
could work.With all this in mind we're going to raise a bug report with the Edge team to make them aware of this problem that was introduced in Edge 18 (with previous versions behaving like Internet Explorer).
We have also decided to continue with the rendering issues with Internet Explorer 11 as we think the focus styles are still an improvement over user-agent styling.
Related Edge bug: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/21053902/
Closes: #1302
Also fixes #1236