-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
src/layouts/blank.vue
Outdated
@@ -1,3 +1,9 @@ | |||
<template> | |||
<Nuxt /> | |||
</template> | |||
|
|||
<script> | |||
import '~/utils/safe-focus-visible-polyfill' |
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.
Is there a better way to import this into all layouts client-side only? I tried using a setting in head
object in nuxt-config but I wasn't sure how to get the polyfill file referenced properly 🤔 Would we need to copy it out of node_modules
during the build step or something into the static folder so it would get copied into the build output and we could reference it in a script element in the head?
@@ -0,0 +1 @@ | |||
if (process.client) require('focus-visible') |
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.
focus-visible
breaks SSR so we need to guard it to only be imported on the client.
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 well versed with Nuxt config but could we add this somewhere globally instead of importing in every layout?
@dhruvkb Is there a global root file for Nuxt? I couldn't really find one that is accessible to edit, that's why I placed it in the layouts 🤔 |
There has to be some level above the layouts. I'll dig through the docs. |
@dhruvkb based on https://forum.vuejs.org/t/nuxt-import-css-file-and-js-file/42498 it seems like adding it to I suppose we could use a CDN distribution of it instead of copying it from node_modules? |
If we can do that, it'll be great. But considering the a11y implications of completely forgetting about a browser, we could also merge this as a quickfix for now. |
Well, either way we'll have to deploy the fix which probably won't happen for another couple hours anyway. @zackkrida can you see any reason not to use a CDN version of the polyfill (provided we include all the appropriate integrity checks?) We could use https://unpkg.com/focus-visible@5.2.0/dist/focus-visible.min.js?meta as we already rely on Cloudflare anyway. |
I'm comfortable with unpkg + integrity checks, I don't see any issue there! |
Went the plugin route that @obulat recommended. It's much more sensible! |
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 working well on desktop Safari. A good way for others to test this is comparing the hover state of the homepage content type chooser on localhost vs. https://wordpress.org/openverse/
Fixes
Fixes #723 by @sarayourfriend
Description
Fixes focus-visible for Safari using a polyfill and a postcss plugin.
Testing Instructions
Checkout this branch and run it in Safari and verify that focus rings appear on elements that need them when focused via the keyboard (like buttons in the header).
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin