Fix internal header menu (#2106)

* Add failing e2e tests * Add focus trap and scroll lock cleanups * Use `new_header` for failing e2e tests * Deactivate focus trap for other modals * Move viewport size from package to README.md * Reuse the `setCookies` function for enabling new header
WordPress · Jan 31, 2023 · 13ea928 · 13ea928
1 parent d2b7573
commit 13ea928
Show file tree

Hide file tree

Showing 11 changed files with 122 additions and 51 deletions.
diff --git a/src/components/VHeader/VHeaderInternal.vue b/src/components/VHeader/VHeaderInternal.vue
@@ -40,6 +40,7 @@
         </VPopoverContent>
         <VModalContent
           v-else-if="!isSm"
+          ref="modalContentRef"
           aria-labelledby="menu-button"
           :hide="closePageMenu"
           variant="full"
@@ -121,6 +122,7 @@ export default defineComponent({
   setup(_, { emit }) {
     const menuButtonRef = ref<InstanceType<typeof VIconButton> | null>(null)
     const nodeRef = ref<HTMLElement | null>(null)
+    const modalContentRef = ref<InstanceType<typeof VModalContent> | null>(null)
 
     const uiStore = useUiStore()
 
@@ -138,6 +140,10 @@ export default defineComponent({
 
     const lockBodyScroll = computed(() => !isSm.value)
 
+    const deactivateFocusTrap = computed(
+      () => modalContentRef.value?.deactivateFocusTrap
+    )
+
     const {
       close: closePageMenu,
       open: openPageMenu,
@@ -148,6 +154,7 @@ export default defineComponent({
       nodeRef,
       lockBodyScroll,
       emit,
+      deactivateFocusTrap,
     })
 
     // When clicking on an internal link in the modal, close the modal
@@ -159,6 +166,7 @@ export default defineComponent({
 
     return {
       menuButtonRef,
+      modalContentRef,
       nodeRef,
 
       closeIcon,

diff --git a/src/components/VModal/VInputModal.vue b/src/components/VModal/VInputModal.vue
@@ -66,19 +66,21 @@ export default defineComponent({
   ],
   setup(props, { attrs, emit }) {
     const focusTrapRef = ref<ComponentInstance | null>(null)
+    const nodeRef = ref<HTMLElement | null>(null)
+    const dialogRef = ref<HTMLElement | null>(null)
 
     const visibleRef = toRef(props, "isActive")
 
-    const nodeRef = ref<HTMLElement | null>(null)
+    const deactivateRef = ref()
+
     const { close } = useDialogControl({
       visibleRef,
       nodeRef,
       emit: emit as SetupContext["emit"],
+      deactivateFocusTrap: deactivateRef,
     })
 
-    const dialogRef = ref<HTMLElement | null>(null)
-
-    const { onKeyDown, onBlur } = useDialogContent({
+    const { onKeyDown, onBlur, deactivateFocusTrap } = useDialogContent({
       dialogElements: {
         dialogRef,
         triggerElementRef: ref(null),
@@ -95,6 +97,7 @@ export default defineComponent({
       emit: emit as SetupContext["emit"],
       attrs,
     })
+    deactivateRef.value = deactivateFocusTrap
 
     return {
       focusTrapRef,

diff --git a/src/components/VModal/VModal.vue b/src/components/VModal/VModal.vue
@@ -15,6 +15,7 @@
     </div>
     <VModalContent
       v-if="triggerRef"
+      ref="modalContentRef"
       :visible="visibleRef"
       :trigger-element="triggerRef"
       :hide-on-esc="hideOnEsc"
@@ -167,13 +168,17 @@ export default defineComponent({
       typeof props.visible === "undefined" ? undefined : toRef(props, "visible")
 
     const nodeRef = ref<null | HTMLElement>(null)
-
+    const modalContentRef = ref<InstanceType<typeof VModalContent> | null>(null)
     const triggerContainerRef = ref<HTMLElement | null>(null)
 
     const triggerRef = computed(
       () => triggerContainerRef.value?.firstChild as HTMLElement | undefined
     )
 
+    const deactivateFocusTrap = computed(
+      () => modalContentRef.value?.deactivateFocusTrap
+    )
+
     const {
       close,
       open,
@@ -182,12 +187,15 @@ export default defineComponent({
       visible: visibleRef,
     } = useDialogControl({
       visibleRef: visiblePropRef,
+      lockBodyScroll: true,
       nodeRef,
       emit: emit as SetupContext["emit"],
+      deactivateFocusTrap,
     })
 
     return {
       nodeRef,
+      modalContentRef,
       visibleRef,
       triggerContainerRef,
       triggerRef,
@@ -196,6 +204,7 @@ export default defineComponent({
       open,
       onTriggerClick,
       triggerA11yProps,
+      deactivateFocusTrap,
     }
   },
 })

diff --git a/src/components/VModal/VModalContent.vue b/src/components/VModal/VModalContent.vue
@@ -152,7 +152,7 @@ export default defineComponent({
       () => props.initialFocusElement || closeButton.value?.$el
     )
     const dialogRef = ref<HTMLElement | null>(null)
-    const { onKeyDown, onBlur } = useDialogContent({
+    const { onKeyDown, onBlur, deactivateFocusTrap } = useDialogContent({
       dialogElements: {
         dialogRef,
         initialFocusElementRef: initialFocusElement,
@@ -177,6 +177,7 @@ export default defineComponent({
       onBlur,
       closeIcon,
       closeButton,
+      deactivateFocusTrap,
     }
   },
 })

diff --git a/src/composables/use-dialog-content.ts b/src/composables/use-dialog-content.ts
@@ -51,7 +51,7 @@ export function useDialogContent({
     dialogRef,
     visibleRef,
   })
-  useFocusOnShow({
+  const { deactivateFocusTrap } = useFocusOnShow({
     dialogRef,
     visibleRef,
     initialFocusElementRef,
@@ -90,5 +90,5 @@ export function useDialogContent({
     focusOnBlur(event)
   }
 
-  return { onKeyDown, onBlur }
+  return { onKeyDown, onBlur, deactivateFocusTrap }
 }
diff --git a/src/composables/use-dialog-control.ts b/src/composables/use-dialog-control.ts
@@ -9,18 +9,23 @@ import {
   watch,
 } from "@nuxtjs/composition-api"
 
+import { MaybeComputedRef, resolveUnref } from "@vueuse/core"
+
 import { useBodyScrollLock } from "~/composables/use-body-scroll-lock"
 
+type Fn = () => void
 export function useDialogControl({
   visibleRef,
   nodeRef,
   lockBodyScroll,
   emit,
+  deactivateFocusTrap,
 }: {
   visibleRef?: Ref<boolean>
   nodeRef?: Ref<HTMLElement | null>
   lockBodyScroll?: ComputedRef<boolean> | boolean
   emit: SetupContext["emit"]
+  deactivateFocusTrap?: MaybeComputedRef<Fn | undefined>
 }) {
   const internallyControlled = typeof visibleRef === "undefined"
   const internalVisibleRef = internallyControlled ? ref(false) : visibleRef
@@ -30,12 +35,17 @@ export function useDialogControl({
     "aria-haspopup": "dialog",
   })
 
-  watch(internalVisibleRef, (visible) => {
+  watch(internalVisibleRef, (visible, _, onCleanup) => {
     triggerA11yProps["aria-expanded"] = visible
     if (shouldLockBodyScroll.value) {
       visible ? lock() : unlock()
     }
     if (!internallyControlled) emit(visible ? "open" : "close")
+    onCleanup(() => {
+      if (shouldLockBodyScroll.value) {
+        unlock()
+      }
+    })
   })
 
   let lock = () => {
@@ -49,14 +59,15 @@ export function useDialogControl({
     lock = bodyScroll.lock
     unlock = bodyScroll.unlock
   }
-  const shouldLockBodyScroll = computed(() => unref(lockBodyScroll) ?? true)
-  watch(shouldLockBodyScroll, (shouldLock) => {
-    shouldLock ? lock() : unlock()
-  })
+  const shouldLockBodyScroll = computed(() => unref(lockBodyScroll) ?? false)
 
   const open = () => (internalVisibleRef.value = true)
 
-  const close = () => (internalVisibleRef.value = false)
+  const close = () => {
+    const fn = resolveUnref(deactivateFocusTrap)
+    if (fn) fn()
+    internalVisibleRef.value = false
+  }
 
   const onTriggerClick = () => {
     internalVisibleRef.value = !internalVisibleRef.value

diff --git a/src/composables/use-focus-on-show.ts b/src/composables/use-focus-on-show.ts
@@ -1,12 +1,14 @@
 import { nextTick, watch, Ref } from "@nuxtjs/composition-api"
 
+import { tryOnScopeDispose, unrefElement } from "@vueuse/core"
+
 import { warn } from "~/utils/console"
 import {
   ensureFocus,
   getFirstTabbableIn,
   hasFocusWithin,
 } from "~/utils/reakit-utils/focus"
-import { useFocusTrap } from "~/composables/use-focus-trap"
+import { useFocusTrap, UseFocusTrapReturn } from "~/composables/use-focus-trap"
 
 export const noFocusableElementWarning =
   "It's recommended to have at least one tabbable element inside dialog. The dialog element has been automatically focused. If this is the intended behavior, pass `tabIndex={0}` to the dialog element to disable this warning."
@@ -29,37 +31,39 @@ export const useFocusOnShow = ({
   trapFocusRef,
   initialFocusElementRef,
 }: Props) => {
-  const { activate: activateFocusTrap, deactivate: deactivateFocusTrap } =
-    trapFocusRef.value
-      ? useFocusTrap(dialogRef, {
-          // Prevent FocusTrap from trying to focus the first element.
-          // We already do that in a more flexible, adaptive way in our Dialog composables.
-          initialFocus: false,
-          // if set to true, focus-trap prevents the default for the keyboard event, and we cannot handle it in our composables.
-          escapeDeactivates: false,
-        })
-      : {
-          activate: () => {
-            /** */
-          },
-          deactivate: () => {
-            /** */
-          },
-        }
+  let activateFocusTrap = () => {
+    /** */
+  }
+  let deactivateFocusTrap = () => {
+    /** */
+  }
+  let trap: UseFocusTrapReturn | undefined
+  if (trapFocusRef.value) {
+    trap = useFocusTrap(dialogRef, {
+      // Prevent FocusTrap from trying to focus the first element.
+      // We already do that in a more flexible, adaptive way in our Dialog composables.
+      initialFocus: false,
+      // if set to true, focus-trap prevents the default for the keyboard event, and we cannot handle it in our composables.
+      escapeDeactivates: false,
+    })
 
-  watch(
+    activateFocusTrap = trap.activate
+    deactivateFocusTrap = trap.deactivate
+  }
+
+  const stopWatcher = watch(
     [
-      dialogRef,
+      () => unrefElement(dialogRef),
       visibleRef,
       autoFocusOnShowRef,
       initialFocusElementRef,
     ] as const,
-    ([dialog, visible, autoFocusOnShow, initialFocusElement]) => {
+    ([dialog, visible, autoFocusOnShow, initialFocusElement], _, onCleanup) => {
       if (!dialog || !visible) {
-        deactivateFocusTrap()
+        if (trap?.hasFocus) deactivateFocusTrap()
         return
       }
-      if (!dialog || !visible || !autoFocusOnShow) return
+      if (!autoFocusOnShow) return
 
       nextTick(() => {
         const isActive = () => hasFocusWithin(dialog)
@@ -83,6 +87,15 @@ export const useFocusOnShow = ({
         }
         activateFocusTrap()
       })
+
+      onCleanup(() => {
+        deactivateFocusTrap()
+      })
     }
   )
+  tryOnScopeDispose(() => {
+    deactivateFocusTrap()
+    stopWatcher()
+  })
+  return { deactivateFocusTrap }
 }
diff --git a/src/composables/use-focus-trap.ts b/src/composables/use-focus-trap.ts
@@ -98,7 +98,7 @@ export function useFocusTrap(
     }
   }
 
-  watch(
+  const stopWatcher = watch(
     () => unrefElement(target),
     (el) => {
       if (!el) return
@@ -122,11 +122,13 @@ export function useFocusTrap(
       // Focus if immediate is set to true
       if (immediate) activate()
     },
-    { flush: "sync" }
+    { flush: "post" }
   )
 
   // Cleanup on unmount
-  tryOnScopeDispose(() => deactivate())
+  tryOnScopeDispose(() => {
+    stopWatcher()
+  })
 
   return {
     hasFocus,

diff --git a/test/playwright/README.md b/test/playwright/README.md
@@ -221,3 +221,10 @@ in a format that can be used in end-to-end tests.
 
 Note that this does _not_ run the server for you; you must run the Nuxt server
 using `pnpm start` or `pnpm dev` separately before running the codegen script.
+
+To generate tests for a non-default breakpoint, set the viewport size using the
+`--viewport-size` flag. For example, to test the `xs` breakpoint, run:
+
+```
+pnpm run test:playwright:gen --viewport-size=340,600"
+```