From 0677bf85e6a169f5f9789020c4577250b9456ef3 Mon Sep 17 00:00:00 2001 From: Nicholas Boll Date: Wed, 12 Oct 2022 21:30:12 -0600 Subject: [PATCH] ci: Fix popup tests and ci bugs [skip release] (#1846) Fixed multiple ci failures: - Popup tests in React 18 due to different `useLayoutEffect` timing - forward-merge Action not erroring on Chromatic changes - pull-request Action not failing properly - `prerelease/minor` canary builds failing to publish [category:Infrastructure] --- .github/workflows/forward-merge.yml | 1 - .github/workflows/pull-request.yml | 8 ++- .../react/popup/lib/hooks/useReturnFocus.tsx | 52 ++++++++----------- utils/publish-canary.mjs | 2 +- 4 files changed, 29 insertions(+), 34 deletions(-) diff --git a/.github/workflows/forward-merge.yml b/.github/workflows/forward-merge.yml index e087bf05e7..9c45cfbb45 100644 --- a/.github/workflows/forward-merge.yml +++ b/.github/workflows/forward-merge.yml @@ -130,7 +130,6 @@ jobs: appCode: dlpro96xybh storybookBuildDir: docs exitOnceUploaded: false - exitZeroOnChanges: true - name: Start Server run: npx http-server docs -p 9001 & npx wait-on http://localhost:9001 diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index a0be59f16d..970426aded 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -185,7 +185,11 @@ jobs: verify: runs-on: ubuntu-latest + if: always() needs: ['check', 'integration-test', 'visual-test'] steps: - - name: Done - run: echo "Done" + - name: Failure Test + if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') # Test the result of all dependencies + run: exit 1 + - name: Success + run: exit 0 diff --git a/modules/react/popup/lib/hooks/useReturnFocus.tsx b/modules/react/popup/lib/hooks/useReturnFocus.tsx index a5996b959e..b824e0986e 100644 --- a/modules/react/popup/lib/hooks/useReturnFocus.tsx +++ b/modules/react/popup/lib/hooks/useReturnFocus.tsx @@ -3,6 +3,7 @@ import React from 'react'; import {changeFocus, createElemPropsHook, isFocusable} from '@workday/canvas-kit-react/common'; import {usePopupModel} from './usePopupModel'; +import {PopupStack} from '@workday/canvas-kit-popup-stack'; function getScrollParent(element: HTMLElement): HTMLElement { if (element === document.body || !element.parentElement) { @@ -53,19 +54,18 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => { } }, []); - const onKeyUp = React.useCallback((event: KeyboardEvent) => { - requiresFocusChangeRef.current = false; - }, []); - // track mousedown events to determine if the mouse target is a focusable element. If it is, we // should not return focus - const onMouseDown = React.useCallback((event: MouseEvent) => { - elementRef.current = event.target as Element; - }, []); - - const onMouseUp = React.useCallback(() => { - elementRef.current = null; - }, []); + const onMouseDown = React.useCallback( + (event: MouseEvent) => { + if (model.state.stackRef.current && event.target instanceof HTMLElement) { + if (!PopupStack.contains(model.state.stackRef.current, event.target)) { + elementRef.current = event.target; + } + } + }, + [model.state.stackRef] + ); // We use `useLayoutEffect` because the callback will be called _before_ the browser changes // focus. This allows the browser to change focus as normal. For example, if the popup closes @@ -87,17 +87,14 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => { return; } // capture the element here. The refs will be null by the time the cleanup function is called - const element = (model.state.returnFocusRef || model.state.targetRef).current as HTMLElement | null; + const element = (model.state.returnFocusRef || model.state.targetRef) + .current as HTMLElement | null; document.addEventListener('mousedown', onMouseDown, true); - document.addEventListener('mouseup', onMouseUp, true); document.addEventListener('keydown', onKeyDown, true); - document.addEventListener('keyup', onKeyUp, true); return () => { document.removeEventListener('mousedown', onMouseDown, true); - document.removeEventListener('mouseup', onMouseUp, true); document.removeEventListener('keydown', onKeyDown, true); - document.removeEventListener('keyup', onKeyUp, true); if (!element) { return; } @@ -108,12 +105,14 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => { // Don't change focus if user focused on a different element like a `input` or the target // element isn't on at least halfway rendered on the screen. if ( - (elementRef.current && getFocusableElement(elementRef.current)) || - elementRect.top + elementRect.height / 2 < scrollParentRect.top || - elementRect.bottom - elementRect.height / 2 > scrollParentRect.bottom || - elementRect.left + elementRect.width / 2 < scrollParentRect.left || - elementRect.right - elementRect.width / 2 > scrollParentRect.right + (elementRef.current && getFocusableElement(elementRef.current)) || // did the user click on a focusable element? + elementRect.top + elementRect.height / 2 < scrollParentRect.top || // is the top half of the target element visible? + elementRect.bottom - elementRect.height / 2 > scrollParentRect.bottom || // is the bottom half of the target element visible? + elementRect.left + elementRect.width / 2 < scrollParentRect.left || // is the left half of the target element visible? + elementRect.right - elementRect.width / 2 > scrollParentRect.right // is the right half of the target element visible? ) { + // reset the focus element and bail early + elementRef.current = null; return; } @@ -129,17 +128,10 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => { changeFocus(element); }); } + elementRef.current = null; }; - }, [ - model.state.returnFocusRef, - model.state.targetRef, - visible, - onMouseDown, - onMouseUp, - onKeyDown, - onKeyUp, - ]); + }, [model.state.returnFocusRef, model.state.targetRef, visible, onMouseDown, onKeyDown]); return {}; }); diff --git a/utils/publish-canary.mjs b/utils/publish-canary.mjs index 1489c9d185..a3324275c6 100755 --- a/utils/publish-canary.mjs +++ b/utils/publish-canary.mjs @@ -101,7 +101,7 @@ exec('git diff --name-only HEAD HEAD^') bump = 'premajor'; } else { // we'll use `next` for pre minors - preid = 'next'; + preid = `${process.env.GITHUB_RUN_NUMBER || 0}-next`; bump = 'preminor'; } }