Focusable elements across the site have these various focus style scenarios

  1. default browser focus style only
  2. default plus Umami focus styles
  3. 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 :-(
CommentFileSizeAuthor
#54 search-form-button-focus-outline-firefox64-AFTER-patch-2983568-53.png12.95 KBandrewmacpherson
#53 Screen Shot 2019-01-02 at 17.08.41.png37.08 KBkjay
#53 interdiff-42-53.txt581 byteskjay
#53 drupal_core-umami-focus-styles-2983568-53.patch17.36 KBkjay
#43 Screen Shot 2018-12-11 at 19.30.47.png8.02 KBsmaz
#43 Screen Shot 2018-12-11 at 19.30.37.png7.56 KBsmaz
#42 interdiff--36-42.txt2.52 KBsmaz
#42 drupal_core-umami-focus-styles-2983568-42.patch17.19 KBsmaz
#36 interdiff-29-36.txt4.16 KBkjay
#36 drupal_core-umami-focus-styles-2983568-36.patch16 KBkjay
#33 2983568-29-umami-focus-styles-review.txt2.13 KBandrewmacpherson
#29 interdiff-23-29.txt690 byteskjay
#29 drupal_core-umami-focus-styles-2983568-29.patch15.7 KBkjay
#28 Screen Shot 2018-11-23 at 16.18.31.png20.75 KBsmaz
#24 interdiff-22-23.txt333 byteskjay
#24 drupal_core-umami-focus-styles-2983568-23.patch15.66 KBkjay
#22 Screen Shot 2018-11-22 at 15.21.31.png56.96 KBsmaz
#22 drupal_core-umami-focus-styles-2983568-22.patch16.17 KBsmaz
#17 umami-focus-styles-2.png1000.83 KBkjay
#17 interdiff-13-17.txt7.53 KBkjay
#17 drupal_core-umami-focus-styles-2983568-17.patch14.53 KBkjay
#4 Screen Shot 2018-07-04 at 11.17.19.png9.78 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.17.27.png6.28 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.17.39.png7.03 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.17.48.png7.05 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.17.56.png18.75 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.00.png13.89 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.07.png7.38 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.14.png6.31 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.22.png51.89 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.30.png1.39 MBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.41.png1.39 MBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.47.png1.39 MBjohn cook
#4 Screen Shot 2018-07-04 at 11.20.58.png436.49 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.21.07.png436.79 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.21.13.png435.46 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.21.25.png1.13 MBjohn cook
#4 Screen Shot 2018-07-04 at 11.21.36.png1.13 MBjohn cook
#4 Screen Shot 2018-07-04 at 11.21.43.png1.13 MBjohn cook
#4 Screen Shot 2018-07-04 at 11.21.59.png35.17 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.22.06.png14.55 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.26.36.png17.46 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.26.43.png17.37 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.26.53.png12.06 KBjohn cook
#4 Screen Shot 2018-07-04 at 11.27.01.png12.83 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_13_09_04.png2.66 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_00.png1.38 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_08.png1.74 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_15.png7.42 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_23.png1.76 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_27.png1.78 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_32.png1.47 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_06_37.png12.34 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_02.png598.02 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_07.png597.84 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_12.png598.11 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_22.png180.83 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_26.png181.14 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_29.png180.86 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_46.png483.94 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_53.png484.94 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_07_56.png484.41 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_08_09.png12.34 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_08_13.png4.9 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_08_40.png5.38 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_08_44.png5.37 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_08_47.png2.99 KBjohn cook
#5 VirtualBox_IE11 - Win8.1_04_07_2018_12_08_52.png3.5 KBjohn cook
#10 hover and focus.png465.92 KBkjay
#10 drupal_core-umami-focus-styles-2983568-10.patch14.34 KBkjay
#13 umami-focus-styles.png760.2 KBkjay
#13 drupal_core-umami-focus-styles-2983568-13.patch19.08 KBkjay
#15 2983568-umami-banner-cta-double-border-idea-3.png21.93 KBandrewmacpherson
#15 2983568-umami-banner-cta-double-border-idea-2.png22.04 KBandrewmacpherson
#15 2983568-umami-banner-cta-double-border-idea-1.png21.93 KBandrewmacpherson

Comments

emma.maria created an issue. See original summary.

emma.maria’s picture

Issue summary: View changes
emma.maria’s picture

Issue summary: View changes
john cook’s picture

john cook’s picture

Issue summary: View changes
andrewmacpherson’s picture

Issue summary: View changes

Thanks 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).

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

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.

Chrome's is a thick blue line which doesn't always look great (off-centered, a side cut off the border, etc).

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:

  1. The Blink & Webkit user agent focus styles cannot address WCAG SC 1.4.11 Non-text contrast, because they don't have any sympathy for the page background colour. Chrome's #4d90fe provides a contrast of 3.11:1 against white (pass), but only 1.98:1 against the dark grey footer background (fail). I've no idea how it scores against the beige backgrounds. Safari's blue is paler and fails against a white background.
  2. But an even bigger problem with the WebKit & Blink browsers is that they keep changing the colours without so much as a mention in the release notes. Remember when Chrome had an orange outline for the focus ring?

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 :-)

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kjay’s picture

I 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

kjay’s picture

Status: Active » Needs review
StatusFileSize
new465.92 KB
new14.34 KB

Here'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.

Screengrabs of hover and focus style changes to Umami

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:

  • Further accessibility review
  • x-browser tests
andrewmacpherson’s picture

Thanks 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:

  1. IE11 doesn't support outline-offset. However we might get a similar effect with some clever CSS on pseudo-elements. I think you can do this by styling the position, size, and border of :focus:before... I'm sure I have bookmarked some info about this somewhere.
  2.  .view-mode-card .node__link:focus,
     .view-mode-card .node__link:hover {
       text-decoration: underline;
    +  outline: none;
       color: #000;
     }

    How does this focus style work? The "view article" link has an outline in the screenshot from #10, but we have outline: none here.

  3. box-shadow is 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...
  4.  .form-search:focus {
    -  margin: 0 0 -2px -2px;
    -  padding: 5px 8px 5px 32px;
    +  border: 1px solid #dbdbdb;
       outline: none;
    +  box-shadow: inset 0 0 0 2px #00836d;
     }
    

    The border: 1px solid #dbdbdb is 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:

  1. Some buttons have a positive outline offset, others have a negative outline offset. Likewise, general text fields have a positive outline-offset, but the header search field gets an inset indicator. Could we use one or the other approach for consistency? (EDIT: came back to finish this sentence, doh.)
  2. Some colour combinations need checking. For instance, there's a pink button background which gets an inset dotted green outline.
    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.
kjay’s picture

Assigned: Unassigned » kjay

Thanks @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.

kjay’s picture

Here'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.

Umami focus styles examples 2
View image

kjay’s picture

Assigned: kjay » Unassigned
andrewmacpherson’s picture

@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 negative outline-offset doesn'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.

Double-border button focus style against an almost-black page background.

Double-border button focus style against an deep pink page background.

Double-border button focus style against an almost-white page background.

I literally dreamed about this one night.

mgifford’s picture

I like the double border focus style approach...

Pretty cool dream @andrewmacpherson :)

kjay’s picture

Attached is an updated version of the patch in #13 fixing the following issues:

  1. Logo incorrectly had the default link background green tint on focus
  2. A class for 'menu-block' was being added to a twig file, this was not used
  3. After discussion in this week's OOTB call, we agreed to de-scope the work in patch #13 to restyle the search results page search form. This patch returns the form to a near identical state of that in 8.6.x today. I will create a follow up issue in which I will propose the design/theming changes and note the issues that are broken on this form and need addressing

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:

Screengrabs of the focus style elements for Umami

kjay’s picture

Title: Audit and improve focus styles across the Umami theme » Audit and improve focus styles across the Umami theme for logged out users

Discussed in this week's OOTB call to adjust the title to reflect that our focus style work here is for logged out features only.

kjay’s picture

Issue tags: +badcamp 2018

Tagging for badcamp

andrewmacpherson’s picture

Issue summary: View changes

Thanks 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...

  • The scope of this issue is focus styles, but many hover styles are changed too. This is fine, so long as all hover styles satisfy WCAG success criteria 1.4.3 Contrast (minimum) and 1.4.11 Non-text contrast. I haven't confirmed the contrast in this quick review.
  • The "give it a go and grow your own herbs" link doesn't have an outline like other links do. This looks like an omission.
  • The "tags: grow your own" has a colour change where we see orange text on a pale green background. It may pass WCAG contrast in the full-colour space, but still be difficult for a person with a red-green colour blindness. In #11 I mentioned this problem for a green/pink combination; this green/orange combination is another example I seem to have missed earlier. (I don't actually know how to test these anymore; I used to have a Firefox extension for checking contrast for colour blindness, but it no longer works with Firefox Quantum.)

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.

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.

kjay’s picture

Big 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.

smaz’s picture

At 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?

Umami search results

smaz’s picture

Status: Needs review » Needs work
kjay’s picture

@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.

kjay’s picture

Status: Needs work » Needs review
eli-t’s picture

NB needs testing in RTL as well as LTR

eli-t’s picture

Status: Needs review » Needs work

Further work required with RTL as demonstrated by @smaz on the weekly call today.

smaz’s picture

StatusFileSize
new20.75 KB

In RTL, I think there's only one issue: The search button in the header loses the left hand border.

Umami search button RTL issue

Edit: I also tested this as a logged in user, and all appeared ok - the node edit tabs, contextual links, forms etc.

kjay’s picture

Status: Needs work » Needs review
StatusFileSize
new15.7 KB
new690 bytes

Patch attached fixes observation by @smaz in #28 that the left border is missing from the pre-header search button in RTL.

markconroy’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

cferthorney’s picture

I agree with the RTBC marking. Excellent work Keith - well done!

andrewmacpherson’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup

From #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.

andrewmacpherson’s picture

This is a very significant jump forward for inclusive design. Fantastic work everyone!

The latest patch makes focus styles...

  • Enormously clearer in Blink (& WebKit?), replacing the default blue focus outline. That user-agent style passed WCAG 2.0 by the letter of the law, but utterly failed WCAG 2.1. The footer region suffered particularly.
  • More consistent among theme components, so it's easier to follow focus all around the site. We now have just a handful of special focus styles (e.g. banner block CTA), instead of a wildly mixed bag of focus styles.
  • More consistent between browsers. This isn't actually an accessibility requirement, it's just the easiest way to address the previous points while satisfying WCAG 2.1 "Non-text contrast".

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:

  • Primary/secondary tabs
  • Toolbar experimental profile warning
  • The links in message styles all have contrast which passes WCAG 2.1 in the full-colour space. (It would be nice to assess these for contrast in simulated colour-blindness, because of the mix of green, orange, and pink. However I'm not sure what tools are available for that, so I have not filed a follow-up issue.)

Maybe we missed something, but at least we now have an idea of what remains, so the audit part is done.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/profiles/demo_umami/themes/umami/css/base.css
    @@ -13,6 +13,13 @@ html {
    +  outline-offset: 2px;
    

    I 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...

  2. +++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
    @@ -61,37 +60,61 @@
    +  .form-search {
    +    width: 14em;
    +    border-right: none;
    +  }
    +  [dir=rtl] .form-search {
    +    border-left: none;
    +  }
    

    Do we need a border-right: inherit; of the rtl version?

  3. +++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
    @@ -61,37 +60,61 @@
    +  border-top-left-radius: 2px;
    +  border-bottom-left-radius: 2px;
    

    These need the /* LTR */ comment.

  4. +++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
    @@ -104,23 +127,46 @@
    +  border: 1px solid #dbdbdb;
    +  border-top-right-radius: 3px;
    +  border-bottom-right-radius: 3px;
    +  border-left: none;
    

    Defining border and border-left seems off. Plus the last three need /* LTR */

  5. +++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
    @@ -104,23 +127,46 @@
    +  [dir=rtl] .search-block-form .form-submit,
    +  [dir=rtl] .search-form .form-submit {
    +    border-top-right-radius: 0;
    +    border-bottom-right-radius: 0;
    +  }
    

    Does this need to change border-top-left-radius and border-bottom-left-radius?

alexpott’s picture

Also this introduces some CSS style fails.

You can run these yourselves by doing yarn run lint:css from inside the core directory.

profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css
 47:18  ✖  Expected "#ffffff" to be "#fff"   color-hex-length

profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
  78:21  ✖  Expected "#ffffff" to be "#fff"              color-hex-length
  86:1   ✖  Expected empty line before at-rule           at-rule-empty-line-before
  95:1   ✖  Unexpected empty line before closing brace   block-closing-brace-empty-line-before
 149:21  ✖  Expected "#ffffff" to be "#fff"              color-hex-length
 156:1   ✖  Expected empty line before at-rule           at-rule-empty-line-before

profiles/demo_umami/themes/umami/css/components/navigation/more-link/more-link.css
 21:3  ✖  Unexpected duplicate "text-decoration"   declaration-block-no-duplicate-properties
kjay’s picture

Status: Needs work » Needs review
StatusFileSize
new16 KB
new4.16 KB

Thanks 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.

cferthorney’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC, I didn’t spot any regressions and the inter diff looks fine.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think there are still some missing /* LTR */ :(

Here are the changes I would make:

diff --git a/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css b/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
index 0db86aa69c..08f8e55546 100644
--- a/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
+++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/search/search.css
@@ -132,9 +132,9 @@
   margin-bottom: 0;
   border-top: 1px solid #dbdbdb;
   border-bottom: 1px solid #dbdbdb;
-  border-right: 1px solid #dbdbdb;
-  border-top-right-radius: 3px;
-  border-bottom-right-radius: 3px;
+  border-right: 1px solid #dbdbdb; /* LTR */
+  border-top-right-radius: 3px; /* LTR */
+  border-bottom-right-radius: 3px; /* LTR */
 }
 [dir=rtl] .search-block-form .form-actions {
   border-top-right-radius: 0;
@@ -159,8 +159,8 @@
 @media screen and (min-width: 48em) {
   .search-block-form .form-submit,
   .search-form .form-submit {
-    border-top-left-radius: 0;
-    border-bottom-left-radius: 0;
+    border-top-left-radius: 0; /* LTR */
+    border-bottom-left-radius: 0; /* LTR */
   }
   [dir=rtl] .search-block-form .form-submit,
   [dir=rtl] .search-form .form-submit {

Also why are we applying background-position only in the rtl context...

[dir=rtl] .form-search:focus {
  background-position: 0.35em;
  border-top-right-radius: 2px;
  border-bottom-right-radius: 2px;
  border-top-left-radius: 0;
  border-bottom-left-radius: 0;
}

there's no equivalent on the LTR version which seems odd.

alexpott’s picture

It is needed then background: url(../../../../images/svg/search.svg) no-repeat 0.5em center #fff; needs an /* LTR */ but I think background-position: 0.35em; should be removed because when in rtl and clicking in the search box the icon moves :(

andrewmacpherson’s picture

There'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?

smaz’s picture

Assigned: Unassigned » smaz

Working on this at the moment.

smaz’s picture

Assigned: smaz » Unassigned
Category: Bug report » Task
Status: Needs work » Needs review
StatusFileSize
new17.19 KB
new2.52 KB

I have:

  1. Added the LTR comments as suggested by @alexpott
  2. Tested the background-position: 0.35em; issue - indeed, this style was no longer required, so I've removed it.
  3. Fixed the toolbar edit button issue, as it was caused by this patch. We've extended the selectors for styling buttons, and that affected the button in the toolbar. After a brief chat with @markconroy, we decided to add a toolbar.css file in the components folder, rather than bury it in base.css or something. I set the font-family to be inherit, which is what normalize.css does.

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

smaz’s picture

Screenshots for before & after my patch for the edit button:

With patch #36:
Toolbar edit button with incorrect font

With patch #42
Toolbar edit button with correct font

markconroy’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

alexpott credited jogordon.

alexpott credited mairi.

alexpott’s picture

Assigning issue credit as per #42.

Crediting @markconroy and myself for issue review and @emma.maria for creating the issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The 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/node I think is unintended - such a large area becoming green is because of flex: 1 1

markconroy’s picture

Status: Needs work » Needs review

@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.

markconroy’s picture

Status: Needs review » Reviewed & tested by the community

Setting to RTBC (I hit 'Needs review' in the last comment by accident)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@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.

markconroy’s picture

Thanks @alexpott,

We'll get cracking on that asap. Hopefully we'll be ready with a new patch tomorrow.

kjay’s picture

Status: Needs work » Needs review
StatusFileSize
new17.36 KB
new581 bytes
new37.08 KB

As 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.

andrewmacpherson’s picture

The 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?

Screenshot - search form button has focus, with an offset dotted outline.

kjay’s picture

@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).

markconroy’s picture

Status: Needs review » Reviewed & tested by the community

That 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

andrewmacpherson’s picture

#55 Thanks for clarifying that @kjay.

This means #54 is not an issue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

diff --git a/core/profiles/demo_umami/themes/umami/css/components/toolbar/toolbar.css b/core/profiles/demo_umami/themes/umami/css/components/toolbar/toolbar.css
index faf77fca63..3886aef364 100644
--- a/core/profiles/demo_umami/themes/umami/css/components/toolbar/toolbar.css
+++ b/core/profiles/demo_umami/themes/umami/css/components/toolbar/toolbar.css
@@ -1,7 +1,11 @@
 /**
+ * @file
+ * This file is used to style the admin toolbar.
+ *
  * Button styles in /css/base.css change the font for the 'Edit' button
  * in the admin toolbar - set this back to inherit, which normalize.css does.
  */
+
 .toolbar button {
   font-family: inherit;
-}
\ No newline at end of file
+}

Fixed the new toolbar.css so it adheres to our standards.

  • alexpott committed 2278ec8 on 8.7.x
    Issue #2983568 by kjay, smaz, John Cook, andrewmacpherson, alexpott,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.