diff --git a/packages/scratch-gui/src/components/action-menu/action-menu.css b/packages/scratch-gui/src/components/action-menu/action-menu.css index 3c9bfa0ddff..ce2fc087558 100644 --- a/packages/scratch-gui/src/components/action-menu/action-menu.css +++ b/packages/scratch-gui/src/components/action-menu/action-menu.css @@ -29,7 +29,8 @@ button::-moz-focus-inner { border: 0; } -.button:hover { +.button:hover, +.button:focus { background: $extensions-primary; } diff --git a/packages/scratch-gui/src/components/action-menu/action-menu.jsx b/packages/scratch-gui/src/components/action-menu/action-menu.jsx index a09fd906819..a5381788a2e 100644 --- a/packages/scratch-gui/src/components/action-menu/action-menu.jsx +++ b/packages/scratch-gui/src/components/action-menu/action-menu.jsx @@ -1,152 +1,185 @@ +import React, {useState, useRef, useEffect, useCallback} from 'react'; import PropTypes from 'prop-types'; -import React from 'react'; import classNames from 'classnames'; -import bindAll from 'lodash.bindall'; import ReactTooltip from 'react-tooltip'; - import styles from './action-menu.css'; +import {KEY} from '../../lib/navigation-keys'; const CLOSE_DELAY = 300; // ms -class ActionMenu extends React.Component { - constructor (props) { - super(props); - bindAll(this, [ - 'clickDelayer', - 'handleClosePopover', - 'handleToggleOpenState', - 'handleTouchStart', - 'handleTouchOutside', - 'setButtonRef', - 'setContainerRef' - ]); - this.state = { - isOpen: false, - forceHide: false - }; - this.mainTooltipId = `tooltip-${Math.random()}`; - } - componentDidMount () { - // Touch start on the main button is caught to trigger open and not click - this.buttonRef.addEventListener('touchstart', this.handleTouchStart); - // Touch start on document is used to trigger close if it is outside - document.addEventListener('touchstart', this.handleTouchOutside); - } - shouldComponentUpdate (newProps, newState) { - // This check prevents re-rendering while the project is updating. - // @todo check only the state and the title because it is enough to know - // if anything substantial has changed - // This is needed because of the sloppy way the props are passed as a new object, - // which should be refactored. - return newState.isOpen !== this.state.isOpen || - newState.forceHide !== this.state.forceHide || - newProps.title !== this.props.title; - } - componentWillUnmount () { - this.buttonRef.removeEventListener('touchstart', this.handleTouchStart); - document.removeEventListener('touchstart', this.handleTouchOutside); - } - handleClosePopover () { - this.closeTimeoutId = setTimeout(() => { - this.setState({isOpen: false}); - this.closeTimeoutId = null; - }, CLOSE_DELAY); - } - handleToggleOpenState () { +const ActionMenu = ({ + className, + img: mainImg, + title: mainTitle, + moreButtons, + tooltipPlace, + onClick +}) => { + const [forceHide, setForceHide] = useState(false); + const [isExpanded, setIsExpanded] = useState(false); + + const closeTimeoutRef = useRef(null); + const mainTooltipId = useRef(`tooltip-${Math.random()}`).current; + const containerRef = useRef(null); + const buttonRef = useRef(null); + const itemRefs = useRef([]); + + const handleToggleOpenState = useCallback(() => { // Mouse enter back in after timeout was started prevents it from closing. - if (this.closeTimeoutId) { - clearTimeout(this.closeTimeoutId); - this.closeTimeoutId = null; - } else if (!this.state.isOpen) { - this.setState({ - isOpen: true, - forceHide: false - }); + + if (closeTimeoutRef.current) { + clearTimeout(closeTimeoutRef.current); + closeTimeoutRef.current = null; + } else if (!isExpanded) { + setIsExpanded(true); + setForceHide(false); } - } - handleTouchOutside (e) { - if (this.state.isOpen && !this.containerRef.contains(e.target)) { - this.setState({isOpen: false}); - ReactTooltip.hide(); + }, [isExpanded]); + + const handleTouchStart = useCallback(e => { + // Prevent this touch from becoming a click if menu is closed + if (!isExpanded) { + e.preventDefault(); + handleToggleOpenState(); + } + }, [isExpanded, handleToggleOpenState]); + + useEffect(() => { + const buttonEl = buttonRef.current; + if (!buttonEl) return; + + buttonEl.addEventListener('touchstart', handleTouchStart); + return () => { + buttonEl.removeEventListener('touchstart', handleTouchStart); + }; + }, [handleTouchStart]); + + // Handle clicks/touches outside to close menu + useEffect(() => { + const handleTouchOutside = e => { + if (containerRef.current && !containerRef.current.contains(e.target)) { + setIsExpanded(false); + ReactTooltip.hide(); + } + }; + + document.addEventListener('touchstart', handleTouchOutside); + return () => { + document.removeEventListener('touchstart', handleTouchOutside); + }; + }, [containerRef, setIsExpanded]); + + const focusItem = useCallback(item => { + if (item) { + item.focus(); } - } - clickDelayer (fn) { + }, []); + + const handleMove = useCallback(direction => { + const items = itemRefs.current; + if (!items.length) return; + + if (!items.includes(document.activeElement)) { + focusItem(direction === 1 ? items[0] : items[items.length - 1]); + return; + } + + const currentIndex = items.indexOf(document.activeElement); + const nextIndex = (currentIndex + direction + items.length) % items.length; + focusItem(items[nextIndex]); + }, [itemRefs, focusItem]); + + const handleKeyDown = useCallback(e => { + if (e.key === KEY.ARROW_DOWN || e.key === KEY.ARROW_UP) { + e.preventDefault(); + if (!isExpanded) { + setIsExpanded(true); + } + const direction = e.key === KEY.ARROW_UP ? -1 : 1; + handleMove(direction); + } else if (e.key === KEY.TAB || e.key === KEY.ESCAPE) { + setIsExpanded(false); + focusItem(buttonRef.current); + } + }, [handleMove, isExpanded, setIsExpanded]); + + const handleClosePopover = useCallback(() => { + closeTimeoutRef.current = setTimeout(() => { + setIsExpanded(false); + closeTimeoutRef.current = null; + }, CLOSE_DELAY); + }, []); + + const clickDelayer = useCallback( // Return a wrapped action that manages the menu closing. // @todo we may be able to use react-transition for this in the future // for now all this work is to ensure the menu closes BEFORE the // (possibly slow) action is started. - return event => { + fn => (event => { ReactTooltip.hide(); if (fn) fn(event); // Blur the button so it does not keep focus after being clicked // This prevents keyboard events from triggering the button - this.buttonRef.blur(); - this.setState({forceHide: true, isOpen: false}, () => { - setTimeout(() => this.setState({forceHide: false})); - }); - }; - } - handleTouchStart (e) { - // Prevent this touch from becoming a click if menu is closed - if (!this.state.isOpen) { - e.preventDefault(); - this.handleToggleOpenState(); - } - } - setButtonRef (ref) { - this.buttonRef = ref; - } - setContainerRef (ref) { - this.containerRef = ref; - } - render () { - const { - className, - img: mainImg, - title: mainTitle, - moreButtons, - tooltipPlace, - onClick - } = this.props; - - return ( -
setForceHide(false), 0); + }), + [] + ); + + return ( +
+ - -
-
    - {(moreButtons || []).map(({img, title, onClick: handleClick, - fileAccept, fileChange, fileInput, fileMultiple}, keyId) => { + + +
    +
      + {(moreButtons || []).map( + ( + { + img, + title, + onClick: handleClick, + fileAccept, + fileChange, + fileInput, + fileMultiple + }, + keyId + ) => { const isComingSoon = !handleClick; const hasFileInput = fileInput; - const tooltipId = `${this.mainTooltipId}-${title}`; + const tooltipId = `${mainTooltipId}-${title}`; return (
      • ); - })} -
    -
    + } + )} +
- ); - } -} +
+ ); +}; ActionMenu.propTypes = { className: PropTypes.string, img: PropTypes.string, - moreButtons: PropTypes.arrayOf(PropTypes.shape({ - img: PropTypes.string, - title: PropTypes.node.isRequired, - onClick: PropTypes.func, // Optional, "coming soon" if no callback provided - fileAccept: PropTypes.string, // Optional, only for file upload - fileChange: PropTypes.func, // Optional, only for file upload - fileInput: PropTypes.func, // Optional, only for file upload - fileMultiple: PropTypes.bool // Optional, only for file upload - })), + moreButtons: PropTypes.arrayOf( + PropTypes.shape({ + img: PropTypes.string, + title: PropTypes.node.isRequired, + onClick: PropTypes.func, // Optional, "coming soon" if no callback provided + fileAccept: PropTypes.string, // Optional, only for file upload + fileChange: PropTypes.func, // Optional, only for file upload + fileInput: PropTypes.func, // Optional, only for file upload + fileMultiple: PropTypes.bool // Optional, only for file upload + }) + ), onClick: PropTypes.func.isRequired, title: PropTypes.node.isRequired, tooltipPlace: PropTypes.string }; -export default ActionMenu; +export default React.memo(ActionMenu, (prevProps, nextProps) => + // This check prevents re-rendering while the project is updating. + // This is needed because of the sloppy way the props are passed as a new object, + // which should be refactored. + // Only re-render if the title changes + prevProps.title === nextProps.title +); diff --git a/packages/scratch-gui/src/components/stage-selector/stage-selector.jsx b/packages/scratch-gui/src/components/stage-selector/stage-selector.jsx index 12f3fa97dc6..904e6d8e2cf 100644 --- a/packages/scratch-gui/src/components/stage-selector/stage-selector.jsx +++ b/packages/scratch-gui/src/components/stage-selector/stage-selector.jsx @@ -110,7 +110,6 @@ const StageSelector = props => { title: intl.formatMessage(messages.addBackdropFromSurprise), img: surpriseIcon, onClick: onSurpriseBackdropClick - }, { title: intl.formatMessage(messages.addBackdropFromPaint), img: paintIcon, diff --git a/packages/scratch-gui/test/integration/sounds.test.js b/packages/scratch-gui/test/integration/sounds.test.js index 57bf11e861a..5be2271b6e8 100644 --- a/packages/scratch-gui/test/integration/sounds.test.js +++ b/packages/scratch-gui/test/integration/sounds.test.js @@ -173,6 +173,7 @@ describe('Working with sounds', () => { await loadUri(uri); await clickText('Sounds'); + // Just select any button inside the tab so the controls impact the sounds editor const el = await findByXpath('//button[@aria-label="Choose a Sound"]'); await el.sendKeys(Key.chord(cmdCtrl, 'a')); // Select all await findByText('0.85', scope.soundsTab); // Meow sound duration diff --git a/packages/scratch-gui/test/unit/components/action-menu.test.jsx b/packages/scratch-gui/test/unit/components/action-menu.test.jsx new file mode 100644 index 00000000000..cb1f101eb33 --- /dev/null +++ b/packages/scratch-gui/test/unit/components/action-menu.test.jsx @@ -0,0 +1,136 @@ +import '@testing-library/jest-dom'; +import userEvent from '@testing-library/user-event'; +import {render, screen, fireEvent} from '@testing-library/react'; +import ActionMenu from '../../../src/components/action-menu/action-menu.jsx'; +import {KEY} from '../../../src/lib/navigation-keys'; +import React, {act} from 'react'; + +// Mock the CSS module so class names exist +jest.mock('../../../src/components/action-menu/action-menu.css', () => ({ + expanded: 'expanded' +})); + +describe('ActionMenu keyboard navigation', () => { + const mockOnClick = jest.fn(); + const mockMoreButtonClick = jest.fn(); + + const defaultProps = { + title: 'Main Button', + img: 'main-icon.svg', + onClick: mockOnClick, + moreButtons: [ + {title: 'Button 1', img: 'icon1.svg', onClick: mockMoreButtonClick}, + {title: 'Button 2', img: 'icon2.svg', onClick: mockMoreButtonClick}, + {title: 'Button 3', img: 'icon3.svg', onClick: mockMoreButtonClick} + ] + }; + + beforeEach(() => { + mockOnClick.mockClear(); + mockMoreButtonClick.mockClear(); + }); + + test('focus + arrow_down opens menu and arrow_up cycles to last', () => { + render(); + const mainButton = screen.getByRole('button', {name: 'Main Button'}); + + act(() => { + mainButton.focus(); + fireEvent.keyDown(mainButton, {key: KEY.ARROW_DOWN}); + }); + + const firstItem = screen.getByRole('button', {name: 'Button 1'}); + expect(document.activeElement).toBe(firstItem); + + act(() => { + fireEvent.keyDown(firstItem, {key: KEY.ARROW_UP}); + }); + + const lastItem = screen.getByRole('button', {name: 'Button 3'}); + expect(document.activeElement).toBe(lastItem); + + const menuContainer = mainButton.parentElement; + expect(menuContainer).toHaveClass('expanded'); + }); + + test('escape closes menu and returns focus to main button', () => { + render(); + const mainButton = screen.getByRole('button', {name: 'Main Button'}); + + act(() => { + mainButton.focus(); + fireEvent.keyDown(mainButton, {key: KEY.ARROW_DOWN}); + }); + + const firstItem = screen.getByRole('button', {name: 'Button 1'}); + expect(document.activeElement).toBe(firstItem); + + act(() => { + fireEvent.keyDown(firstItem, {key: KEY.ESCAPE}); + }); + + expect(document.activeElement).toBe(mainButton); + + const menuContainer = mainButton.parentElement; + expect(menuContainer).not.toHaveClass('expanded'); + }); + + test('tab closes menu and focuses next element', async () => { + render( + <> + + + + ); + const mainButton = screen.getByRole('button', {name: 'Main Button'}); + const afterButton = screen.getByRole('button', {name: 'After Menu'}); + const user = userEvent.setup(); + + act(() => { + mainButton.focus(); + fireEvent.keyDown(mainButton, {key: KEY.ARROW_DOWN}); + }); + + const firstItem = screen.getByRole('button', {name: 'Button 1'}); + expect(document.activeElement).toBe(firstItem); + + await act(async () => { + await user.tab(); + // Wait 1 second for any menu close animations or timeouts + await new Promise(resolve => setTimeout(resolve, 1000)); + }); + + expect(document.activeElement).toBe(afterButton); + const menuContainer = mainButton.parentElement; + expect(menuContainer).not.toHaveClass('expanded'); + }); + + test('shift + tab closes menu and focuses previous element', async () => { + render( + <> + + + + + ); + + const mainButton = screen.getByRole('button', {name: 'Main Button'}); + const beforeButton = screen.getByRole('button', {name: 'Before Menu'}); + const user = userEvent.setup(); + + act(() => { + mainButton.focus(); + }); + + const menuContainer = mainButton.parentElement; + expect(menuContainer).toHaveClass('expanded'); + await act(async () => { + await user.tab({shift: true}); + // Wait 1 second for any menu close animations or timeouts + await new Promise(resolve => setTimeout(resolve, 1000)); + }); + + expect(document.activeElement).toBe(beforeButton); + expect(menuContainer).not.toHaveClass('expanded'); + }); +});