Focusable elements across the site have these various focus style scenarios
- default browser focus style only
- default plus Umami focus styles
- Umami successfully overriding default focus styles
| Mac OS - Safari | Windows - Edge | |
|---|---|---|
| Skip link |
|
|
| Menu - active |
|
|
| Menu - not active |
![]()
|
|
| Umami logo |
|
|
| Search field |
|
|
| Search submit button |
|
|
| Log in link |
|
|
| Banner top button |
|
|
| Article card - image |
|
|
| Article card - title |
|
|
| Article card - view more |
|
|
| Recipe card - small - image |
|
|
| Recipe card - small - title |
|
|
| Recipe card - small - view more |
|
|
| Recipe card - large - image |
|
|
| Recipe card - large - title |
|
|
| Recipe card - large - view more |
|
|
| Footer links |
![]()
|
![]()
|
| Breadcrumb |
![]()
|
![]()
|
| Tags |
![]()
|
![]()
|
Pages tested
<front>
/articles
/articles/give-it-a-go-and-grow-your-own-herbs
/recipes
/recipes/super-easy-vegetarian-pasta-bake
Most of the images are from the home page. Components that appear to be re-used are only included once – this includes the cards on the collection pages (/articles and /recipes).
Elements which have two focus states (default and Umami)
*names and screenshots*
Elements which do not have Umami theme focus states
*names and screenshots*
The default focus style on Firefox is a dashed line which is weak (stated in this issue as stated here https://www.drupal.org/project/drupal/issues/2942506 (the problem is when the thin dark outline directly abuts a dark image).
Chrome's is a thick blue line which doesn't always look great (off-centered, a side cut off the border, etc), and fails doesn't satisfy WCAG SC 1.4.11 Non-text contrast against the grey footer.
Proposed solution
- Carry on down the road of using focus styles from the theme?
- overriding the default selector and styles that browsers use to set their styling ie. :focus ?
Remaining tasks
- ???
- Important - if any components are being deferred to follow-up issues, we must record which ones they are, otherwise we'll have to do a full audit all over again :-(
| Comment | File | Size | Author |
|---|---|---|---|
| #54 | search-form-button-focus-outline-firefox64-AFTER-patch-2983568-53.png | 12.95 KB | andrewmacpherson |
| #53 | Screen Shot 2019-01-02 at 17.08.41.png | 37.08 KB | kjay |
| #53 | interdiff-42-53.txt | 581 bytes | kjay |
| #53 | drupal_core-umami-focus-styles-2983568-53.patch | 17.36 KB | kjay |
| #43 | Screen Shot 2018-12-11 at 19.30.47.png | 8.02 KB | smaz |







Comments
Comment #2
emma.mariaComment #3
emma.mariaComment #4
john cook commentedComment #5
john cook commentedComment #6
john cook commentedComment #7
andrewmacpherson commentedThanks for doing an audit of the styles. During the first round of theming (just before 8.5) we managed to get to the stage of satisfying WCAG SC 2.4.7 Focus Visible - which doesn't require them to look consistent or nice ;-)
There are a bunch of follow-up issues for various places where the focus would benefit from some design, but without knowing what that would be. More recently in the Slack channel we talked about having another round of design to aim for a more consistent set of styles for the theme, so we could ditch the UA styles. @kjay was happy with this I think.
Ideally we should survey all our supported browsers, as the user agent styles differ considerably.Update: on seconds thoughts, the Safar/Edge screenshots on their own are enough to justify better designs. Safari, Chrome, and Opera have diverged since the WebKit-Blink fork, and there are some differences between the way Firefox and IE UA styles behave (not sure about Edge).This is missing some context. It's NOT the fact that it's a 1px dotted outline which makes it weak. The problem is that the 1px dotted outline is right next to the image, many of which have a dark background (like the dark wooden table top). Where the thin dark outline meets the dark image, it's hard to make out. I think there are a few images where it's a bit easier. With an outline-offset of a few pixels the 1px dotted outline could be fine.
I guess that's down to our CSS layout, but there are bigger problems with the user agent focus style in the WebKit and Blink browsers:
The Firefox UA focus styles fare much better here, because the outline behaves like the CSS currentColor keyword. So as long as your link text has sufficient contrast for SC 1.4.3 Contrast (minimum), then it will often pass SC 1.4.11 Non-text contrast too, by magic :-)
Comment #9
kjay commentedI was supposed to reference the new static styleguide work that I did for this issue. I am currently working through these styles in the theme to hopefully deliver a more consistent experience as per the v3 styleguide that can be found at #2987164: [meta] Static styleguide for Umami demo
Comment #10
kjay commentedHere's a patch that follows the proposed hover/focus style changes made in v3 of the styleguide (see #9 above). Image attached with screengrabs of all the elements affected showing hover and focus where relevant.
This patch has not been put through CSSComb yet, which I will do but it will make initial review much tougher. I'll post a follow up patch shortly with the CSSComb applied and an interdiff.
Please do help test the search field in the pre-header and the search results page form as I've worked on these to sort alignment issues using the current layout methods. We have a separate issue #2977510: Refactor/improve Umami demo's search form CSS for better responsive support to sort this properly but we can keep that separate if these changes are good-to-go for now.
Note: I have simplified the styling of links in the grey footer area by no longer going with the white arrow pointer.
Still to do:
Comment #11
andrewmacpherson commentedThanks for working on this - it's a great start. Overall these focus styles are looking very neat. and clear.
I haven't given it a manual test yet, but I looked through the patch. Here's some feedback about the code:
:focus:before... I'm sure I have bookmarked some info about this somewhere.How does this focus style work? The "view article" link has an outline in the screenshot from #10, but we have
outline: nonehere.box-shadowis used in a few places. This gets ignored when a Windows High-Contrast theme is in use. We should avoid using box-shadow as the only indication of focus. Need to check all the places it's used, but the header search block looks like it falls foul of this...The
border: 1px solid #dbdbdbis already declared in.form-search, so it isn't changing on focus/hover. We can lose this line.It looks like focus indicator relies entirely on box-shadow. I don't expect this to work when a Windows high-contrast theme is in use. Now, there are ways around this. For instance, adding
outline: 2px dotted transparent- an invisible outline in the default full-colour space. When Windows high-contrast theme is used, the box-shadow isn't there, but , but "transparent" gets overriden so there's chunky 2px outline instead. There's also a ms-high-contrast media query, but Firefox doesn't recognize it last time I checked.Aside: Umami already fares very well in Edge with Windows high-contrast, so this is worth fretting over I think. Other core themes fare very badly, but I'll get around to them :-)
Non-code feedback, based on the image in #10:
For WCAG 1.4.11 Non-text contrast, we need to be sure the green dots have 3:1 contrast against the light pink background.
Also, thin green dotted on a pink background might be a problem for red-green colour blindness, but I'm not sure how to measure that.
Both of these issues could be solved by making the outline dots the same colour as the button text. Assuming the button text has good contrast, so does the focus outline.
Comment #12
kjay commentedThanks @andrewmacpherson, I'm getting back on board with this one today.
1) I've just looked the idea of using pseudo elements up and they look like they could be problematic. I wonder if we even need to worry about the lack of offset in IE11 only? I'll need to test and will post a screenshot back here if it looks like we should discuss compromise on this one.
2) It doesn't work! I've forgotten to style it with the dotted green border like that shown in the static styleguide for titles that are links (https://www.drupal.org/files/issues/2018-07-20/umami_styleguide_v3_links...). But I wonder if I intentionally left this, as I'm sure we've agreed to drop support for these titles being links since they duplicate the View bundle link. I'll check our default for titles as links, we should fix it there anyway and these titles, however they are marked up, will just inherit.
3) That's going to be fun. This part of your feedback I need time to experiment with. I've taken advantage of box-shadow being an alternative to outline or border for elements where those attributes are not suitable.
4) More thinking time needed! Along the lines of 3).
5) Negative offset is used on elements that either won't look good if the focus is pushed outside of the element, such as the search field in the header. Or I've used it where we have no idea what will be in the background, such as buttons overlaid on a background image. With background images, we may know the image used when the theme is first installed but we don't know the screen size, which means contrast will vary subject to button position, and we don't know that the user won't change the image as they fiddle. The lack of consistency troubled me originally but going with a negative offset felt like an acceptable compromise if we do want the consistency of using a dotted border for focus across the site.
6) Sure, I'll redo colours to ensure contrast.
Comment #13
kjay commentedHere's the next round of work on this issue. In response to @andrewmacpherson's feedback:
1) I discussed in Slack with @andrewmacpherson that supporting a workaround for IE11's lack of support for outline-offset is probably not feasible and we could settle for being limited by IE11's limitations!. The outline is present just not offset
2) Titles running onto 2 lines look terrible with outline. Given we have no underline by default on titles as links and then on hover/active/focus we get underline, I am hoping this will be enough for this one style requirement?
3) Box-shadow has been removed and exchanged for borders
4) As per 3), box-shadow is removed
I have also restyled the Search Results page's search form to follow base form element styles as opposed to following the pre-header search form styling. I think this is more logical since the pre-header is designed to be light and compact. Also, this form includes advanced components when logged in.
I have fixed the layout issues for the advanced form components.
I have adjusted the base form buttons to follow our base form style, not the pre-header search form style.
Please see attached screenshot of focus elements demonstrated.
View image
Comment #14
kjay commentedComment #15
andrewmacpherson commented@kjay - I've been playing with
border-style: double;as a focus style for the banner CTA link. This double border idea is an alternative way to inset a focus indicator, because negativeoutline-offsetdoesn't work in IE.Live demo - https://jsbin.com/qayorez/1/edit?html,css,output
Screenshots - the double border focus style seen against backgrounds of varying contrast.
I literally dreamed about this one night.
Comment #16
mgiffordI like the double border focus style approach...
Pretty cool dream @andrewmacpherson :)
Comment #17
kjay commentedAttached is an updated version of the patch in #13 fixing the following issues:
With the exception of IE11, which does not support the negative offset attribute of our outline style, I believe this patch otherwise provides our goals for accessible and consistent treatment for hover and focus styles of elements.
In regards to IE11. @andrewmacpherson thank you for your idea and I did approach this latest patch with the intention of reworking it entirely to integrate your double border style idea. However, I think it will slow us down when we have a patch here that already works if we only accept that IE11 is a less capable browser that fails to add just one of our preferred attributes (the negative offset). As such, I would propose moving the idea of double borders into a follow up issue if all agree this patch finally moves us forward with this important issue.
The other reason for my thinking that we should work on double borders in a follow up issue is that I am not sure how they will work in all cases, and will they therefore result in quite a bit of specific styles for components? For example, this patch applies outline globally, just like the browsers do. We can not add borders or double borders globally for obvious reasons. We would also need to consider how we design with double borders, such as a button that requires no border in normal state, a single border on hover and double border on focus - will this result in sizing issues?
It would be great to have this current work reviewed for accessibility and in general.
Here's a more useful set of screengrabs of the features styled by this patch:
Comment #18
kjay commentedDiscussed in this week's OOTB call to adjust the title to reflect that our focus style work here is for logged out features only.
Comment #19
kjay commentedTagging for badcamp
Comment #20
andrewmacpherson commentedThanks for the updates @kjay.
Re #17:
I took a quick look at the screenshots, but I haven't done a thorough review yet. Some things I noticed...
This is fine, there might be better methods. A limitation of the double border style is we can't control the thickness of the actual lines that are drawn, the user-agent does this.
Re #18:
Whoah, this title change narrows the purpose of the issue significantly, and diminishes the value of the "audit" part. If we decide to defer some improvements to follow-up issues, we need a list of which components have AND have-not been finalized before marking this as fixed. Otherwise we'll have to audit everything again as a logged-in user, to find out which ones got left out. Remember, this issue exists because we already decided to defer some improvements, back when we were aiming to commit Umami in time for the 8.5.x freeze - so this issue IS the follow-up!
Saying it only needs to look good for logged-out users is strange, I think, because an evaluator who follows the installation guide will actually be a logged-in user when it completes.
Comment #21
kjay commentedBig thanks for the review @andrewmacpherson.
We discussed this issue on our Out of the Box call this week and agreed that it would be great if we can get reviews against this patch in order to be committed sooner rather than later. We can then begin creating individual issues for the further work that needs to be done.
As it stands, I believe this patch delivers WCAG compliance and sets out a consistent, global approach for how we style hover and focus interactions (I have tackled both since they are obviously connected in terms of styling).
Regarding changing the title of this issue. We discussed this as being only for the purpose of limiting the current scope of work, especially given the size of the latest patch and the improvements it makes. We will certainly need to create a follow up in order to begin the next round of auditing and fixes. Though I think it makes sense to ensure we split the actual work into smaller task issues.
Importantly, this patch has quite an impact on other open issues and so reviewing and completing on what we have so far would be a real help.
Regarding the issues you’ve raised with the ‘Give it a go and grow your own herbs’ title and the ‘tags: grow your own’. I will double check and follow up very shortly.
Comment #22
smazAt the request of @kjay, I've re-rolled the patch in #17 to apply cleanly to 8.7.x as there had been changes to improve RTL language support that were causing issues.
I'm not sure I've got everything spot on regarding search - some things don't sit right, and I had a bit of a guess at the RTL changes needed. @kjay, can you try the patch & take a look?
Comment #23
smazComment #24
kjay commented@smaz, thank you so much. I think you have it just right and attached is a further patch that addresses @andrewmacpherson's points:
a) Yes, overlooked. I have fixed in the attached patch.
b) Yes, the orange text on pale green for terms links does pass the contrast test so we should be good on that front.
I believe this patch is good for review and hopefully we are there.
Comment #25
kjay commentedComment #26
eli-tNB needs testing in RTL as well as LTR
Comment #27
eli-tFurther work required with RTL as demonstrated by @smaz on the weekly call today.
Comment #28
smazIn RTL, I think there's only one issue: The search button in the header loses the left hand border.
Edit: I also tested this as a logged in user, and all appeared ok - the node edit tabs, contextual links, forms etc.
Comment #29
kjay commentedPatch attached fixes observation by @smaz in #28 that the left border is missing from the pre-header search button in RTL.
Comment #30
markconroy commentedI want to give this patch a great big hug, I LOVE IT.
Thanks so much for all the hard work on this @kjay; getting this committed will be a great bonus AND will unblock other issues that are on hold while we get this complete.
I've spent about 1.5 hours with Keith (and other OOTB Team members) just now on our weekly call going through each page of the website with this patch applied and without this patch applied, and have also read through the entire patch as well as being guided through it with Keith.
I'm happy to RTBC this; it's a massive improvement.
Comment #31
cferthorneyI agree with the RTBC marking. Excellent work Keith - well done!
Comment #32
andrewmacpherson commentedFrom #20 and #21 - Do we have a list of what items have NOT been addressed by the patch here, and will be handled in follow-ups? In particular relating to the issue scope change about logged in/out users.
I think we should have the follow-up list before RTBC. I'm taking it for a spin now to get the list started.
Comment #33
andrewmacpherson commentedThis is a very significant jump forward for inclusive design. Fantastic work everyone!
The latest patch makes focus styles...
I filed a bunch of follow-ups. Comment #22 already noted the search input still needs work. My messy test notes are attached as a text file here.
Some things I checked as a logged-in user. The following do NOT need follow-ups, they are fine:
Maybe we missed something, but at least we now have an idea of what remains, so the audit part is done.
Comment #34
alexpottI think given the discussions about IE11 and its lack of support a comment here is worth it. Ie. we choose to use this despite that because...
Do we need a border-right: inherit; of the rtl version?
These need the
/* LTR */comment.Defining
borderandborder-leftseems off. Plus the last three need/* LTR */Does this need to change border-top-left-radius and border-bottom-left-radius?
Comment #35
alexpottAlso this introduces some CSS style fails.
You can run these yourselves by doing
yarn run lint:cssfrom inside the core directory.Comment #36
kjay commentedThanks for the review @alexpott. New patch attached addressing issues as follows:
1. Comment added
2. Looks like the border style and related RTL class were redundant due to them affecting the white border only, now removed and tested
3. Added the comments
4. I've tidied these border-radius styles so that they are set per corner (rather than overriding a radius on all corners) and added the missing RTL styles for the same
5. Yes, now added.
6. Lint issues resolved.
It's worth noting that we are all set to follow up this issue with a rebuild of the search results form styles as we need to switch to using a better method for layout. That work will follow along the lines of what I previously did for this patch but removed for the sake of limiting the scope for this patch to focus styles rather than layout.
Comment #37
cferthorneyMarking RTBC, I didn’t spot any regressions and the inter diff looks fine.
Comment #38
alexpottI think there are still some missing
/* LTR */:(Here are the changes I would make:
Also why are we applying
background-positiononly in the rtl context...there's no equivalent on the LTR version which seems odd.
Comment #39
alexpottIt is needed then
background: url(../../../../images/svg/search.svg) no-repeat 0.5em center #fff;needs an/* LTR */but I thinkbackground-position: 0.35em;should be removed because when in rtl and clicking in the search box the icon moves :(Comment #40
andrewmacpherson commentedThere's a regression of the toolbar styling. After patches 29 or 26, the Edit button in the toolbar gets a serif font.
It seems this was reported earlier, fixed by some unknown commit, then regresses again here. Since the Umami styles keep leaking into the toolbar, I suggest we accept the regression here, and fix it in a follow-up? The details are in #2987665-9: Toolbar styling is easily disrupted by theme CSS.. If Umami styles are leaking into the toolbar, it's a safe bet other custom themes run into this too. So perhaps this needs more robust CSS in toolbar module itself?
Comment #41
smazWorking on this at the moment.
Comment #42
smazI have:
This would make the related toolbar button issue redundant, so could we add commit credits for those who took a look at & stab at fixing that issue:
https://www.drupal.org/project/drupal/issues/2987665
jogordon - https://www.drupal.org/u/jogordon
mairi - https://www.drupal.org/u/mairi
Eli-T
Cheers
Comment #43
smazScreenshots for before & after my patch for the edit button:
With patch #36:

With patch #42

Comment #44
markconroy commentedThis patch looks good to me, thanks @smaz for the extra work.
Hopefully this gets committed as soon as possible, it touches so much of our CSS, it's holding up progress towards our 8.7.x goals and is also blocking other issues that are in progress as they will need to be re-rolled against this.
Committers, can we have this committed and any other items needed from it created as follow-ups if they are just small items such as adding LTR comments? They will make good issues for new contributors at camps.
Comment #47
alexpottAssigning issue credit as per #42.
Crediting @markconroy and myself for issue review and @emma.maria for creating the issue.
Comment #48
alexpottThe text "search" on the search button on
/search/node(not the one on the header) now moves when your mouse hovers over it. This is a regression introduced by this patch :(The problem is worse when you open the advanced search vertical tab and mouse over the advanced search button as this moves text below. Note your only get advanced search when logged in.
Also the hover state of the search help link on
/search/nodeI think is unintended - such a large area becoming green is because offlex: 1 1Comment #49
markconroy commented@alexpott
We have a follow-up issue for the search page #2977510: Refactor/improve Umami demo's search form CSS for better responsive support
We have never had any design for the search page, so just made it up as we went along to have something acceptable. We plan to redo the search page in future iterations, starting with rationalising the CSS that is there, using issue #2977510: Refactor/improve Umami demo's search form CSS for better responsive support
If the search page items are the only items holding up this, can we have this committed and we'll attend to the search items in the search issue. No one is currently working on that issue or reviewing it until we have this issue committed. I'll set to RTBC again, revert back to Needs work if the search page needs to be fixed as part of this issue.
Comment #50
markconroy commentedSetting to RTBC (I hit 'Needs review' in the last comment by accident)
Comment #51
alexpott@markconroy okay #2977510: Refactor/improve Umami demo's search form CSS for better responsive support can address the massive focus because that could be a design decision (for what it's worth) - but the vertical movement should be fixed here because it is introduced by this issue.
Comment #52
markconroy commentedThanks @alexpott,
We'll get cracking on that asap. Hopefully we'll be ready with a new patch tomorrow.
Comment #53
kjay commentedAs per @markconroy's suggestion in Slack, we could fix these vertical jumps 'for now', both for the input field and the regression currently in this patch, by ensuring that any borders remain the same thickness between focus and hover states for this one search page form.
Patch attached for review that does this. As per the screenshots attached.
Comment #54
andrewmacpherson commentedThe screenshot in #53 - which browser was this from? It doesn't show an outline when the search button has focus; it's just a colour-only change, which would fail WCAG use-of-color.
But when I tested patch #53 manually, I DID see a focus outline for the search button. Here's a screenshot from Firefox 64/mac showing the offset dotted outline (it's the same in Chrome 71/mac and Safari 12). So is screenshot #53 actually happening in any browsers?
Comment #55
kjay commented@andrewmacpherson, no and sorry for the confusion. The screenshot is just showing the focus state for the input field and the hover state for the button because the 1px border change is being proposed to resolve the vertical movement on hover (and therefore focus also).
Comment #56
markconroy commentedThat search input is looking mighty fine to me. Looking forward to making it even better when we get around to doing some serious work on the search page.
Marking RTBC
Comment #57
andrewmacpherson commented#55 Thanks for clarifying that @kjay.
This means #54 is not an issue.
Comment #58
alexpottCommitted 2278ec8 and pushed to 8.7.x. Thanks!
I've only committed this to 8.7.x because of the scope of the change. Might backport after talking to other committers / out-of-the-box team.
Fixed the new toolbar.css so it adheres to our standards.