From 7a3b30bd6aa5759e17d65e98b72e3ae4f63f803c Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Wed, 21 Aug 2024 12:49:17 -0500 Subject: [PATCH 1/8] Make highlight Actions menu items (Edit and Delete) buttons [DISCO-307] For clarity, changed `ol` to `menu` Also simplified return value of DropdownItem. --- src/app/components/Dropdown.spec.tsx | 7 ++-- src/app/components/Dropdown.tsx | 32 ++++++++++--------- .../__snapshots__/DotMenu.spec.tsx.snap | 24 ++++++++------ .../__snapshots__/Dropdown.spec.tsx.snap | 24 ++++++++------ .../__snapshots__/ContextMenu.spec.tsx.snap | 18 ++++++----- .../__snapshots__/DisplayNote.spec.tsx.snap | 17 +++++----- 6 files changed, 68 insertions(+), 54 deletions(-) diff --git a/src/app/components/Dropdown.spec.tsx b/src/app/components/Dropdown.spec.tsx index 2013417131..217249e328 100644 --- a/src/app/components/Dropdown.spec.tsx +++ b/src/app/components/Dropdown.spec.tsx @@ -149,9 +149,10 @@ describe('Dropdown', () => { ); renderer.act(() => { - const [button1, button2] = component.root.findAllByType('a'); - button1.props.onClick(mockEv); - button2.props.onClick(mockEv); + const items = component.root.findAll((i) => i.props.onClick && i.type === 'button'); + + items.forEach((i) => i.props.onClick(mockEv)); + expect(items.length).toBe(2); }); expect(mockEv.preventDefault).toHaveBeenCalledTimes(2); diff --git a/src/app/components/Dropdown.tsx b/src/app/components/Dropdown.tsx index de52e7a683..3a146cade1 100644 --- a/src/app/components/Dropdown.tsx +++ b/src/app/components/Dropdown.tsx @@ -172,13 +172,14 @@ function TrappingDropdownList(props: object) { useTrapTabNavigation(ref); return ( -
    + ); } // tslint:disable-next-line:variable-name export const DropdownList = styled(TrappingDropdownList)` + list-style: none; margin: 0; padding: 0.6rem 0; background: ${theme.color.neutral.formBackground}; @@ -237,22 +238,23 @@ const DropdownItemContent = ({ }); return {(msg) => href - ? {prefix}{msg} - /* - this should be a button but Safari and firefox don't support focusing buttons - which breaks the tab transparent dropdown - https://bugs.webkit.org/show_bug.cgi?id=22261 - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus - */ - // eslint-disable-next-line jsx-a11y/anchor-is-valid - : {prefix}{msg} + // Safari support tab-navigation of buttons; this operates with space or Enter + : } ; }; @@ -261,9 +263,9 @@ const DropdownItemContent = ({ export const DropdownItem = ({ariaMessage, ...contentProps}: DropdownItemProps) => { const intl = useIntl(); - return ariaMessage - ?
    2. - :
    3. ; + return
  3. + +
    4. ; }; interface CommonDropdownProps { diff --git a/src/app/components/__snapshots__/DotMenu.spec.tsx.snap b/src/app/components/__snapshots__/DotMenu.spec.tsx.snap index b3fb1c0479..d5b3f2312f 100644 --- a/src/app/components/__snapshots__/DotMenu.spec.tsx.snap +++ b/src/app/components/__snapshots__/DotMenu.spec.tsx.snap @@ -136,6 +136,7 @@ exports[`Dropdown matches snapshot 1`] = ` } .c12 { + list-style: none; margin: 0; padding: 0.6rem 0; background: #f5f5f5; @@ -224,28 +225,29 @@ exports[`Dropdown matches snapshot 1`] = ` -
    1. - Delete - +
    2. Edit
      3. -
    +     -
    1. - Delete - +
    2. Edit
      3. -
    + -
    1. - Delete - +
    2. Edit
      3. -
    + -
    1. - Delete - +
    2. Edit
      3. -
    + `; diff --git a/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap b/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap index 7874183ab1..628f1b536b 100644 --- a/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap +++ b/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap @@ -513,6 +513,7 @@ exports[`ContextMenu match snapshot when open 1`] = ` } .c13 { + list-style: none; margin: 0; padding: 0.6rem 0; background: #f5f5f5; @@ -644,7 +645,7 @@ exports[`ContextMenu match snapshot when open 1`] = `
    -
    1. @@ -967,10 +968,10 @@ exports[`ContextMenu match snapshot when open 1`] = `
    2. - Add note - +
    3. - Delete - +
    4. @@ -1035,7 +1037,7 @@ exports[`ContextMenu match snapshot when open 1`] = ` Go to highlight
      5. -
    +
    diff --git a/src/app/content/highlights/components/__snapshots__/DisplayNote.spec.tsx.snap b/src/app/content/highlights/components/__snapshots__/DisplayNote.spec.tsx.snap index dd7f4e6b6d..5fda3f3c8f 100644 --- a/src/app/content/highlights/components/__snapshots__/DisplayNote.spec.tsx.snap +++ b/src/app/content/highlights/components/__snapshots__/DisplayNote.spec.tsx.snap @@ -419,6 +419,7 @@ exports[`DisplayNote matches snapshot when focused with opened dropdown 1`] = ` } .c12 { + list-style: none; margin: 0; padding: 0.6rem 0; background: #f5f5f5; @@ -612,28 +613,28 @@ exports[`DisplayNote matches snapshot when focused with opened dropdown 1`] = ` -
    1. - Edit - +
    2. - Delete - +
      3. -
    + ); diff --git a/src/app/reactUtils.ts b/src/app/reactUtils.ts index c66c004960..55ecd50054 100644 --- a/src/app/reactUtils.ts +++ b/src/app/reactUtils.ts @@ -22,7 +22,7 @@ function isHidden(el: HTMLElement) { return el.offsetWidth === 0 && el.offsetHeight === 0; } -const focusableItemQuery = [ +export const focusableItemQuery = [ 'button', 'input', 'select', 'textarea', '[href]', '[tabindex]:not([tabindex="-1"]', ].map((s) => s.includes('[') ? s : `${s}:not([disabled])`).join(','); From 6e2155239daa2f4f9deeae6f5a7a6dda85c98f4b Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Wed, 28 Aug 2024 11:14:40 -0500 Subject: [PATCH 4/8] Focus follows mouse --- src/app/components/Dropdown.spec.tsx | 8 ++++++++ src/app/components/Dropdown.tsx | 9 ++++++++- src/app/components/__snapshots__/DotMenu.spec.tsx.snap | 4 ++++ src/app/components/__snapshots__/Dropdown.spec.tsx.snap | 4 ++++ .../SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap | 3 +++ .../components/__snapshots__/DisplayNote.spec.tsx.snap | 2 ++ 6 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/app/components/Dropdown.spec.tsx b/src/app/components/Dropdown.spec.tsx index 217249e328..27a2fc994d 100644 --- a/src/app/components/Dropdown.spec.tsx +++ b/src/app/components/Dropdown.spec.tsx @@ -107,6 +107,7 @@ describe('Dropdown', () => { const useOnEscSpy = jest.spyOn(reactUtils, 'useOnEsc'); const focus = jest.fn(); + const focus2 = jest.fn(); const addEventListener = jest.fn(); const removeEventListener = jest.fn(); const createNodeMock = () => ({focus, addEventListener, removeEventListener}); @@ -126,12 +127,19 @@ describe('Dropdown', () => { expect(() => component.root.findByType(DropdownList)).not.toThrow(); + renderer.act(() => { + const buttons = component.root.findAllByType('button'); + + buttons[1].props.onMouseEnter({target: {focus: focus2}}); + }); + renderer.act(() => { useOnEscSpy.mock.calls[0][1](); }); expect(() => component.root.findByType(DropdownList)).toThrow(); expect(focus).toHaveBeenCalled(); + expect(focus2).toHaveBeenCalled(); useOnEscSpy.mockClear(); }); diff --git a/src/app/components/Dropdown.tsx b/src/app/components/Dropdown.tsx index 294e8dec02..314b7c82fb 100644 --- a/src/app/components/Dropdown.tsx +++ b/src/app/components/Dropdown.tsx @@ -245,7 +245,12 @@ const DropdownItemContent = ({ 'data-analytics-label': dataAnalyticsLabel, 'data-analytics-region': dataAnalyticsRegion, }); - return + const focusMe = React.useCallback( + ({target: me}) => me.focus(), + [] + ); + +return {(msg) => href ? {prefix}{msg} // Safari support tab-navigation of buttons; this operates with space or Enter @@ -260,6 +266,7 @@ const DropdownItemContent = ({ type='button' tabIndex={0} onClick={onClick ? flow(preventDefault, onClick) : preventDefault} + onMouseEnter={focusMe} {...analyticsDataProps} > {prefix}{msg} diff --git a/src/app/components/__snapshots__/DotMenu.spec.tsx.snap b/src/app/components/__snapshots__/DotMenu.spec.tsx.snap index d5b3f2312f..82f30f67f8 100644 --- a/src/app/components/__snapshots__/DotMenu.spec.tsx.snap +++ b/src/app/components/__snapshots__/DotMenu.spec.tsx.snap @@ -231,6 +231,7 @@ exports[`Dropdown matches snapshot 1`] = `