From 69c9bbb7ead5b543be97dfc6a960dad44bcd8a67 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 16 Oct 2024 13:44:25 +0300 Subject: [PATCH 01/30] Converted `Popup` to functional component --- Popup/Popup.js | 618 ++++++++++++++++++++++++------------------------- 1 file changed, 301 insertions(+), 317 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index ab3ab660a..430e25a12 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -11,9 +11,10 @@ import {is} from '@enact/core/keymap'; import {on, off} from '@enact/core/dispatcher'; +import {setDefaultProps} from '@enact/core/util'; import FloatingLayer from '@enact/ui/FloatingLayer'; import kind from '@enact/core/kind'; -import {Component} from 'react'; +import {useCallback, useEffect, useRef, useState} from 'react'; import PropTypes from 'prop-types'; import Spotlight, {getDirection} from '@enact/spotlight'; import Pause from '@enact/spotlight/Pause'; @@ -278,6 +279,15 @@ const OpenState = { OPEN: 2 }; +const popupDefaultProps = { + noAnimation: false, + noAutoDismiss: false, + open: false, + position: 'bottom', + scrimType: 'translucent', + spotlightRestrict: 'self-only' +}; + /** * A stateful component that renders a popup in a * {@link ui/FloatingLayer.FloatingLayer|FloatingLayer}. @@ -288,261 +298,31 @@ const OpenState = { * @ui * @public */ -class Popup extends Component { - - static propTypes = /** @lends sandstone/Popup.Popup.prototype */ { - /** - * Prevents closing the popup via 5-way navigation out of the content. - * - * @type {Boolean} - * @default false - * @private - */ - no5WayClose: PropTypes.bool, - - /** - * Disables transition animation. - * - * @type {Boolean} - * @default false - * @public - */ - noAnimation: PropTypes.bool, - - /** - * Indicates that the popup will not trigger `onClose` when the user presses the cancel/back (e.g. `ESC`) key or - * taps outside of the popup. - * - * @type {Boolean} - * @default false - * @public - */ - noAutoDismiss: PropTypes.bool, - - /** - * Called on: - * - * * pressing `ESC` key, - * * clicking on outside the boundary of the popup, or - * * moving spotlight focus outside the boundary of the popup when `spotlightRestrict` is - * `'self-first'`. - * - * Event payload: - * - * * pressing `ESC` key, carries `detail` property containing `inputType` value of `'key'`. - * * clicking outside the boundary of the popup, carries `detail` property containing - * `inputType` value of `'click'`. - * - * It is the responsibility of the callback to set the `open` property to `false`. - * - * @type {Function} - * @public - */ - onClose: PropTypes.func, - - /** - * Called after hide transition has completed, and immediately with no transition. - * - * @type {Function} - * @public - */ - onHide: PropTypes.func, - - /** - * Called when a key is pressed. - * - * @type {Function} - * @public - */ - onKeyDown: PropTypes.func, +const Popup = (props) => { + const {noAnimation, noAutoDismiss, open, scrimType, ...rest} = setDefaultProps(props, popupDefaultProps); - /** - * Called after show transition has completed, and immediately with no transition. - * - * Note: The function does not run if Popup is initially opened and - * {@link sandstone/Popup.PopupBase.noAnimation|noAnimation} is `true`. - * - * @type {Function} - * @public - */ - onShow: PropTypes.func, + const [activator, setActivator] = useState(null); + const [floatLayerOpen, setFloatLayerOpen] = useState(open ?? false); + const [popupOpen, setPopupOpen] = useState(OpenState.CLOSED); + const [prevOpen, setPrevOpen] = useState(!open); - /** - * Controls the visibility of the Popup. - * - * By default, the Popup and its contents are not rendered until `open`. - * - * @type {Boolean} - * @default false - * @public - */ - open: PropTypes.bool, + const containerId = useRef(Spotlight.add()); + const paused = useRef(new Pause('Popup')); + const prevActivator = useRef(null); - /** - * Position of the Popup on the screen. - * - * @type {('bottom'|'center'|'fullscreen'|'left'|'right'|'top')} - * @default 'bottom' - * @public - */ - position: PropTypes.oneOf(['bottom', 'center', 'fullscreen', 'left', 'right', 'top']), - - /** - * Scrim type. - * - * * Values: `'transparent'`, `'translucent'`, or `'none'`. - * - * `'none'` is not compatible with `spotlightRestrict` of `'self-only'`, use a transparent scrim - * to prevent mouse focus when using popup. - * - * @type {('transparent'|'translucent'|'none')} - * @default 'translucent' - * @public - */ - scrimType: PropTypes.oneOf(['transparent', 'translucent', 'none']), - - /** - * Restricts or prioritizes navigation when focus attempts to leave the popup. - * - * * Values: `'self-first'`, or `'self-only'`. - * - * When using `self-first`, attempts to leave the popup via 5-way will fire `onClose` based - * on the following values of `position`: - * - * * `'bottom'` - When leaving via 5-way up - * * `'top'` - When leaving via 5-way down - * * `'left'` - When leaving via 5-way right - * * `'right'` - When leaving via 5-way left - * * `'center'` - When leaving via any 5-way direction - * - * Note: If `onClose` is not set, then this has no effect on 5-way navigation. If the popup - * has no spottable children, 5-way navigation will cause the Popup to fire `onClose`. - * - * @type {('self-first'|'self-only')} - * @default 'self-only' - * @public - */ - spotlightRestrict: PropTypes.oneOf(['self-first', 'self-only']) - }; - - static defaultProps = { - noAnimation: false, - noAutoDismiss: false, - open: false, - position: 'bottom', - scrimType: 'translucent', - spotlightRestrict: 'self-only' - }; - - static getDerivedStateFromProps (props, state) { - if (props.open !== state.prevOpen) { - if (props.open) { - return { - popupOpen: props.noAnimation || state.floatLayerOpen ? OpenState.OPEN : OpenState.CLOSED, - floatLayerOpen: true, - activator: Spotlight.getCurrent(), - prevOpen: props.open - }; - } else { - // Disables the spotlight conatiner of popup when `noAnimation` set - if (props.noAnimation) { - const node = getContainerNode(state.containerId); - if (node) { - node.dataset['spotlightContainerDisabled'] = true; - } - } - - return { - popupOpen: OpenState.CLOSED, - floatLayerOpen: state.popupOpen === OpenState.OPEN ? !props.noAnimation : false, - activator: props.noAnimation ? null : state.activator, - prevOpen: props.open - }; - } - } - return null; - } - - constructor (props) { - super(props); - this.paused = new Pause('Popup'); - this.state = { - floatLayerOpen: this.props.open, - popupOpen: this.props.open ? OpenState.OPEN : OpenState.CLOSED, - prevOpen: this.props.open, - containerId: Spotlight.add(), - activator: null - }; - checkScrimNone(this.props); - } - - // Spot the content after it's mounted. - componentDidMount () { - if (this.props.open) { - // If the popup is open on mount, we need to pause spotlight so nothing steals focus - // while the popup is rendering. - this.paused.pause(); - if (getContainerNode(this.state.containerId)) { - this.spotPopupContent(); - } - } - } - - componentDidUpdate (prevProps, prevState) { - if (this.props.open !== prevProps.open) { - if (!this.props.noAnimation) { - if (!this.props.open && this.state.popupOpen === OpenState.CLOSED) { - // If the popup is supposed to be closed (!this.props.open) and is actually - // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This - // can occur when quickly toggling a Popup open and closed. - this.paused.resume(); - } else { - // Otherwise, we pause spotlight so it is locked until the popup is ready - this.paused.pause(); - } - } else if (this.props.open) { - forwardShow(null, this.props); - this.spotPopupContent(); - } else if (prevProps.open) { - forwardHide(null, this.props); - this.spotActivator(prevState.activator); - } - } - - checkScrimNone(this.props); - } - - componentWillUnmount () { - if (this.props.open) { - off('keydown', this.handleKeyDown); - } - Spotlight.remove(this.state.containerId); - } - - handleFloatingLayerOpen = () => { - if (!this.props.noAnimation && this.state.popupOpen !== OpenState.OPEN) { - this.setState({ - popupOpen: OpenState.OPENING - }); - } else if (this.state.popupOpen === OpenState.OPEN && this.props.open) { - this.spotPopupContent(); - } - }; - - handleKeyDown = (ev) => { - const {onClose, no5WayClose, position, spotlightRestrict} = this.props; + const handleKeyDown = useCallback((ev) => { + const {onClose, no5WayClose, position, spotlightRestrict} = props; if (no5WayClose) return; - const {containerId} = this.state; const keyCode = ev.keyCode; const direction = getDirection(keyCode); - const spottables = Spotlight.getSpottableDescendants(containerId).length; + const spottables = Spotlight.getSpottableDescendants(containerId.current).length; const current = Spotlight.getCurrent(); - if (direction && (!spottables || current && getContainerNode(containerId).contains(current))) { + if (direction && (!spottables || current && getContainerNode(containerId.current).contains(current))) { // explicitly restrict navigation in order to manage focus state when attempting to leave the popup - Spotlight.set(containerId, {restrict: 'self-only'}); + Spotlight.set(containerId.current, {restrict: 'self-only'}); if (onClose) { let focusChanged; @@ -569,60 +349,31 @@ class Popup extends Component { ev.stopPropagation(); // set the pointer mode to false on keydown Spotlight.setPointerMode(false); - forwardCustom('onClose')(null, this.props); + forwardCustom('onClose')(null, props); } } } - }; - - handleDismiss = (ev) => { - forwardCustom('onClose', () => ({detail: ev?.detail}))(null, this.props); - }; - - handlePopupHide = (ev) => { - forwardHide(ev, this.props); - - this.setState({ - floatLayerOpen: false, - activator: null - }); + }, [props]); - if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === this.state.containerId) { - this.spotActivator(this.state.activator); - } - }; - - handlePopupShow = (ev) => { - forwardShow(ev, this.props); - - this.setState({ - popupOpen: OpenState.OPEN - }); - - if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === this.state.containerId) { - this.spotPopupContent(); - } - }; - - spotActivator = (activator) => { - this.paused.resume(); + const spotActivator = useCallback((evActivator) => { + paused.current.resume(); // only spot the activator if the popup is closed - if (this.props.open) return; + if (open) return; const current = Spotlight.getCurrent(); - const containerNode = getContainerNode(this.state.containerId); + const containerNode = getContainerNode(containerId.current); const lastContainerId = getLastContainer(); - off('keydown', this.handleKeyDown); + off('keydown', handleKeyDown); // if there is no currently-spotted control or it is wrapped by the popup's container, we // know it's safe to change focus if (!current || (containerNode && containerNode.contains(current))) { // attempt to set focus to the activator, if available if (!Spotlight.isPaused()) { - if (activator) { - if (!Spotlight.focus(activator)) { + if (evActivator) { + if (!Spotlight.focus(evActivator)) { Spotlight.focus(); } } else { @@ -632,19 +383,17 @@ class Popup extends Component { } } } - }; + }, [handleKeyDown, open]); - spotPopupContent = () => { - this.paused.resume(); + const spotPopupContent = useCallback(() => { + paused.current.resume(); // only spot the activator if the popup is open - if (!this.props.open) return; - - const {containerId} = this.state; + if (!open) return; - on('keydown', this.handleKeyDown); + on('keydown', handleKeyDown); - if (!Spotlight.isPaused() && !Spotlight.focus(containerId)) { + if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { const current = Spotlight.getCurrent(); // In cases where the container contains no spottable controls or we're in pointer-mode, focus @@ -653,36 +402,271 @@ class Popup extends Component { if (current) { current.blur(); } - Spotlight.setActiveContainer(containerId); + Spotlight.setActiveContainer(containerId.current); } - }; + }, [handleKeyDown, open]); - render () { - const {noAutoDismiss, scrimType, ...rest} = this.props; + const handleFloatingLayerOpen = useCallback(() => { + if (!noAnimation && popupOpen !== OpenState.OPEN) { + setPopupOpen(OpenState.OPENING); + } else if (popupOpen === OpenState.OPEN && open) { + spotPopupContent(); + } + }, [noAnimation, open, popupOpen, spotPopupContent]); - delete rest.no5WayClose; - delete rest.onClose; + const handleDismiss = useCallback((ev) => { + forwardCustom('onClose', () => ({detail: ev?.detail}))(null, props); + }, [props]); - return ( - - = OpenState.OPENING} - spotlightId={this.state.containerId} - /> - - ); - } -} + const handlePopupHide = useCallback((ev) => { + forwardHide(ev, props); + + setFloatLayerOpen(false); + setActivator(null); + + if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { + spotActivator(activator); + } + }, []); + + const handlePopupShow = useCallback((ev) => { + forwardShow(ev, props); + + setPopupOpen(OpenState.OPEN); + + if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { + spotPopupContent(); + } + }, [props, spotPopupContent]); + + useEffect(() => { + if (open !== prevOpen) { + if (open) { + setPopupOpen(noAnimation || floatLayerOpen ? OpenState.OPEN : OpenState.CLOSED); + setFloatLayerOpen(true); + setActivator(Spotlight.getCurrent()); + setPrevOpen(open); + } else { + // Disables the spotlight conatiner of popup when `noAnimation` set + if (noAnimation) { + const node = getContainerNode(containerId.current); + if (node) { + node.dataset['spotlightContainerDisabled'] = true; + } + } + + setPopupOpen(OpenState.CLOSED); + setFloatLayerOpen(popupOpen === OpenState.OPEN ? !noAnimation : false); + setActivator(noAnimation ? null : activator); + setPrevOpen(open); + } + } + }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); + + useEffect(() => { + prevActivator.current = activator; + }, [activator]); + + // Spot the content after it's mounted. + useEffect(() => { + if (open) { + // If the popup is open on mount, we need to pause spotlight so nothing steals focus + // while the popup is rendering. + paused.current.pause(); + if (getContainerNode(containerId.current)) { + spotPopupContent(); + } + } + + const id = containerId.current; + return () => { + if (open) { + off('keydown', handleKeyDown); + } + Spotlight.remove(id); + }; + }, [handleKeyDown, open, spotPopupContent]); + + useEffect(() => { + if (open !== floatLayerOpen) { + if (!noAnimation) { + if (!open && popupOpen === OpenState.CLOSED) { + // If the popup is supposed to be closed (!this.props.open) and is actually + // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This + // can occur when quickly toggling a Popup open and closed. + paused.current.resume(); + } else { + // Otherwise, we pause spotlight so it is locked until the popup is ready + paused.current.pause(); + } + } else if (open) { + forwardShow(null, props); + spotPopupContent(); + } else if (floatLayerOpen) { + forwardHide(null, props); + spotActivator(prevActivator.current); + } + } + + checkScrimNone(props); + }, [floatLayerOpen, noAnimation, open, popupOpen, props, spotActivator, spotPopupContent]); + + return ( + + = OpenState.OPENING} + spotlightId={containerId.current} + /> + + ); +}; + +Popup.displayName = "Popup"; +Popup.propTypes = /** @lends sandstone/Popup.Popup.prototype */ { + /** + * Prevents closing the popup via 5-way navigation out of the content. + * + * @type {Boolean} + * @default false + * @private + */ + no5WayClose: PropTypes.bool, + + /** + * Disables transition animation. + * + * @type {Boolean} + * @default false + * @public + */ + noAnimation: PropTypes.bool, + + /** + * Indicates that the popup will not trigger `onClose` when the user presses the cancel/back (e.g. `ESC`) key or + * taps outside of the popup. + * + * @type {Boolean} + * @default false + * @public + */ + noAutoDismiss: PropTypes.bool, + + /** + * Called on: + * + * * pressing `ESC` key, + * * clicking on outside the boundary of the popup, or + * * moving spotlight focus outside the boundary of the popup when `spotlightRestrict` is + * `'self-first'`. + * + * Event payload: + * + * * pressing `ESC` key, carries `detail` property containing `inputType` value of `'key'`. + * * clicking outside the boundary of the popup, carries `detail` property containing + * `inputType` value of `'click'`. + * + * It is the responsibility of the callback to set the `open` property to `false`. + * + * @type {Function} + * @public + */ + onClose: PropTypes.func, + + /** + * Called after hide transition has completed, and immediately with no transition. + * + * @type {Function} + * @public + */ + onHide: PropTypes.func, + + /** + * Called when a key is pressed. + * + * @type {Function} + * @public + */ + onKeyDown: PropTypes.func, + + /** + * Called after show transition has completed, and immediately with no transition. + * + * Note: The function does not run if Popup is initially opened and + * {@link sandstone/Popup.PopupBase.noAnimation|noAnimation} is `true`. + * + * @type {Function} + * @public + */ + onShow: PropTypes.func, + + /** + * Controls the visibility of the Popup. + * + * By default, the Popup and its contents are not rendered until `open`. + * + * @type {Boolean} + * @default false + * @public + */ + open: PropTypes.bool, + + /** + * Position of the Popup on the screen. + * + * @type {('bottom'|'center'|'fullscreen'|'left'|'right'|'top')} + * @default 'bottom' + * @public + */ + position: PropTypes.oneOf(['bottom', 'center', 'fullscreen', 'left', 'right', 'top']), + + /** + * Scrim type. + * + * * Values: `'transparent'`, `'translucent'`, or `'none'`. + * + * `'none'` is not compatible with `spotlightRestrict` of `'self-only'`, use a transparent scrim + * to prevent mouse focus when using popup. + * + * @type {('transparent'|'translucent'|'none')} + * @default 'translucent' + * @public + */ + scrimType: PropTypes.oneOf(['transparent', 'translucent', 'none']), + + /** + * Restricts or prioritizes navigation when focus attempts to leave the popup. + * + * * Values: `'self-first'`, or `'self-only'`. + * + * When using `self-first`, attempts to leave the popup via 5-way will fire `onClose` based + * on the following values of `position`: + * + * * `'bottom'` - When leaving via 5-way up + * * `'top'` - When leaving via 5-way down + * * `'left'` - When leaving via 5-way right + * * `'right'` - When leaving via 5-way left + * * `'center'` - When leaving via any 5-way direction + * + * Note: If `onClose` is not set, then this has no effect on 5-way navigation. If the popup + * has no spottable children, 5-way navigation will cause the Popup to fire `onClose`. + * + * @type {('self-first'|'self-only')} + * @default 'self-only' + * @public + */ + spotlightRestrict: PropTypes.oneOf(['self-first', 'self-only']) +}; +Popup.defaultPropValues = popupDefaultProps; export default Popup; export {Popup, PopupBase}; From e5cfb7486a8181890f99c6eb923a303b3bb4e551 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 16 Oct 2024 13:48:08 +0300 Subject: [PATCH 02/30] Fixed lint warnings --- Popup/Popup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 430e25a12..6b77fa3af 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -427,7 +427,7 @@ const Popup = (props) => { if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { spotActivator(activator); } - }, []); + }, [activator, props, spotActivator]); const handlePopupShow = useCallback((ev) => { forwardShow(ev, props); From 2f723956dbb1a20c06c14ad0d776107451c149b8 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 16 Oct 2024 14:25:23 +0300 Subject: [PATCH 03/30] Deleted `...rest` props --- Popup/Popup.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 6b77fa3af..67142294b 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -406,6 +406,10 @@ const Popup = (props) => { } }, [handleKeyDown, open]); + const handleDismiss = useCallback((ev) => { + forwardCustom('onClose', () => ({detail: ev?.detail}))(null, props); + }, [props]); + const handleFloatingLayerOpen = useCallback(() => { if (!noAnimation && popupOpen !== OpenState.OPEN) { setPopupOpen(OpenState.OPENING); @@ -414,10 +418,6 @@ const Popup = (props) => { } }, [noAnimation, open, popupOpen, spotPopupContent]); - const handleDismiss = useCallback((ev) => { - forwardCustom('onClose', () => ({detail: ev?.detail}))(null, props); - }, [props]); - const handlePopupHide = useCallback((ev) => { forwardHide(ev, props); @@ -511,6 +511,9 @@ const Popup = (props) => { checkScrimNone(props); }, [floatLayerOpen, noAnimation, open, popupOpen, props, spotActivator, spotPopupContent]); + delete rest.no5WayClose; + delete rest.onClose; + return ( Date: Thu, 17 Oct 2024 11:26:31 +0300 Subject: [PATCH 04/30] Fixed focus issues --- Popup/Popup.js | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 67142294b..b620214c6 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -303,12 +303,13 @@ const Popup = (props) => { const [activator, setActivator] = useState(null); const [floatLayerOpen, setFloatLayerOpen] = useState(open ?? false); - const [popupOpen, setPopupOpen] = useState(OpenState.CLOSED); + const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(!open); - const containerId = useRef(Spotlight.add()); - const paused = useRef(new Pause('Popup')); + const containerId = useRef(null); + const paused = useRef(null); const prevActivator = useRef(null); + const prevProps = useRef(null); const handleKeyDown = useCallback((ev) => { const {onClose, no5WayClose, position, spotlightRestrict} = props; @@ -439,6 +440,15 @@ const Popup = (props) => { } }, [props, spotPopupContent]); + useEffect(() => { + paused.current = new Pause('Popup'); + containerId.current = Spotlight.add(); + }, []); + + useEffect(() => { + prevActivator.current = activator; + }, [activator]); + useEffect(() => { if (open !== prevOpen) { if (open) { @@ -463,10 +473,6 @@ const Popup = (props) => { } }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); - useEffect(() => { - prevActivator.current = activator; - }, [activator]); - // Spot the content after it's mounted. useEffect(() => { if (open) { @@ -488,7 +494,7 @@ const Popup = (props) => { }, [handleKeyDown, open, spotPopupContent]); useEffect(() => { - if (open !== floatLayerOpen) { + if (open !== prevProps.current?.open) { if (!noAnimation) { if (!open && popupOpen === OpenState.CLOSED) { // If the popup is supposed to be closed (!this.props.open) and is actually @@ -502,14 +508,15 @@ const Popup = (props) => { } else if (open) { forwardShow(null, props); spotPopupContent(); - } else if (floatLayerOpen) { + } else if (prevProps.current?.open) { forwardHide(null, props); spotActivator(prevActivator.current); } + prevProps.current = props; } checkScrimNone(props); - }, [floatLayerOpen, noAnimation, open, popupOpen, props, spotActivator, spotPopupContent]); + }, [noAnimation, open, popupOpen, props, spotActivator, spotPopupContent]); delete rest.no5WayClose; delete rest.onClose; From 91ce7aa650107718d21ff78c59d5774217f7226d Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 17 Oct 2024 21:10:27 +0300 Subject: [PATCH 05/30] Fixed cleanup function --- Popup/Popup.js | 72 ++++++++++++++++++++++---------------------------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index b620214c6..1aaa440c4 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -299,20 +299,22 @@ const popupDefaultProps = { * @public */ const Popup = (props) => { - const {noAnimation, noAutoDismiss, open, scrimType, ...rest} = setDefaultProps(props, popupDefaultProps); + const allComponentProps = setDefaultProps(props, popupDefaultProps); + const {noAnimation, open} = allComponentProps; + const {noAutoDismiss, scrimType, ...rest} = allComponentProps; const [activator, setActivator] = useState(null); const [floatLayerOpen, setFloatLayerOpen] = useState(open ?? false); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(!open); - const containerId = useRef(null); - const paused = useRef(null); + const containerId = useRef(Spotlight.add()); + const paused = useRef(new Pause('Popup')); const prevActivator = useRef(null); const prevProps = useRef(null); const handleKeyDown = useCallback((ev) => { - const {onClose, no5WayClose, position, spotlightRestrict} = props; + const {onClose, no5WayClose, position, spotlightRestrict} = allComponentProps; if (no5WayClose) return; @@ -350,11 +352,11 @@ const Popup = (props) => { ev.stopPropagation(); // set the pointer mode to false on keydown Spotlight.setPointerMode(false); - forwardCustom('onClose')(null, props); + forwardCustom('onClose')(null, allComponentProps); } } } - }, [props]); + }, [allComponentProps]); const spotActivator = useCallback((evActivator) => { paused.current.resume(); @@ -408,8 +410,8 @@ const Popup = (props) => { }, [handleKeyDown, open]); const handleDismiss = useCallback((ev) => { - forwardCustom('onClose', () => ({detail: ev?.detail}))(null, props); - }, [props]); + forwardCustom('onClose', () => ({detail: ev?.detail}))(null, allComponentProps); + }, [allComponentProps]); const handleFloatingLayerOpen = useCallback(() => { if (!noAnimation && popupOpen !== OpenState.OPEN) { @@ -420,7 +422,7 @@ const Popup = (props) => { }, [noAnimation, open, popupOpen, spotPopupContent]); const handlePopupHide = useCallback((ev) => { - forwardHide(ev, props); + forwardHide(ev, allComponentProps); setFloatLayerOpen(false); setActivator(null); @@ -428,26 +430,17 @@ const Popup = (props) => { if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { spotActivator(activator); } - }, [activator, props, spotActivator]); + }, [activator, allComponentProps, spotActivator]); const handlePopupShow = useCallback((ev) => { - forwardShow(ev, props); + forwardShow(ev, allComponentProps); setPopupOpen(OpenState.OPEN); if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { spotPopupContent(); } - }, [props, spotPopupContent]); - - useEffect(() => { - paused.current = new Pause('Popup'); - containerId.current = Spotlight.add(); - }, []); - - useEffect(() => { - prevActivator.current = activator; - }, [activator]); + }, [allComponentProps, spotPopupContent]); useEffect(() => { if (open !== prevOpen) { @@ -473,25 +466,25 @@ const Popup = (props) => { } }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); - // Spot the content after it's mounted. useEffect(() => { - if (open) { - // If the popup is open on mount, we need to pause spotlight so nothing steals focus - // while the popup is rendering. - paused.current.pause(); - if (getContainerNode(containerId.current)) { - spotPopupContent(); - } - } + prevActivator.current = activator; + }, [activator]); + + useEffect(() => { + checkScrimNone(allComponentProps); + prevProps.current = allComponentProps; + }, [allComponentProps]); - const id = containerId.current; + useEffect(() => { return () => { if (open) { off('keydown', handleKeyDown); } - Spotlight.remove(id); + if (!open) { + Spotlight.remove(containerId.current); + } }; - }, [handleKeyDown, open, spotPopupContent]); + }, [handleKeyDown, open]); useEffect(() => { if (open !== prevProps.current?.open) { @@ -506,20 +499,19 @@ const Popup = (props) => { paused.current.pause(); } } else if (open) { - forwardShow(null, props); + forwardShow(null, allComponentProps); spotPopupContent(); } else if (prevProps.current?.open) { - forwardHide(null, props); + forwardHide(null, allComponentProps); spotActivator(prevActivator.current); } - prevProps.current = props; } - checkScrimNone(props); - }, [noAnimation, open, popupOpen, props, spotActivator, spotPopupContent]); + checkScrimNone(allComponentProps); + }, [allComponentProps, noAnimation, open, popupOpen, spotActivator, spotPopupContent]); - delete rest.no5WayClose; - delete rest.onClose; + delete allComponentProps.no5WayClose; + delete allComponentProps.onClose; return ( Date: Thu, 17 Oct 2024 21:17:59 +0300 Subject: [PATCH 06/30] Fixed lint warnings --- Popup/Popup.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 1aaa440c4..c5a6341cd 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -476,12 +476,13 @@ const Popup = (props) => { }, [allComponentProps]); useEffect(() => { + const id = containerId.current; return () => { if (open) { off('keydown', handleKeyDown); } if (!open) { - Spotlight.remove(containerId.current); + Spotlight.remove(id); } }; }, [handleKeyDown, open]); From 461e1a42aed46f023f896512397563f6c5fae0fd Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Fri, 18 Oct 2024 10:21:36 +0300 Subject: [PATCH 07/30] Extracted props from `handleKeyDown` --- Popup/Popup.js | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index c5a6341cd..45ec14f99 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -300,13 +300,13 @@ const popupDefaultProps = { */ const Popup = (props) => { const allComponentProps = setDefaultProps(props, popupDefaultProps); - const {noAnimation, open} = allComponentProps; - const {noAutoDismiss, scrimType, ...rest} = allComponentProps; + const {noAnimation, open, position, spotlightRestrict} = allComponentProps; + const {noAutoDismiss, no5WayClose, onClose, scrimType, ...rest} = allComponentProps; const [activator, setActivator] = useState(null); - const [floatLayerOpen, setFloatLayerOpen] = useState(open ?? false); + const [floatLayerOpen, setFloatLayerOpen] = useState(open); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); - const [prevOpen, setPrevOpen] = useState(!open); + const [prevOpen, setPrevOpen] = useState(open); const containerId = useRef(Spotlight.add()); const paused = useRef(new Pause('Popup')); @@ -314,8 +314,6 @@ const Popup = (props) => { const prevProps = useRef(null); const handleKeyDown = useCallback((ev) => { - const {onClose, no5WayClose, position, spotlightRestrict} = allComponentProps; - if (no5WayClose) return; const keyCode = ev.keyCode; @@ -356,7 +354,7 @@ const Popup = (props) => { } } } - }, [allComponentProps]); + }, [allComponentProps, no5WayClose, onClose, position, spotlightRestrict]); const spotActivator = useCallback((evActivator) => { paused.current.resume(); @@ -511,9 +509,6 @@ const Popup = (props) => { checkScrimNone(allComponentProps); }, [allComponentProps, noAnimation, open, popupOpen, spotActivator, spotPopupContent]); - delete allComponentProps.no5WayClose; - delete allComponentProps.onClose; - return ( Date: Fri, 18 Oct 2024 17:47:11 +0300 Subject: [PATCH 08/30] Code refactor --- Popup/Popup.js | 115 ++++++++++++++++++++++++++----------------------- 1 file changed, 61 insertions(+), 54 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 45ec14f99..adf21bc3e 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -309,9 +309,8 @@ const Popup = (props) => { const [prevOpen, setPrevOpen] = useState(open); const containerId = useRef(Spotlight.add()); + const componentMounted = useRef(false); const paused = useRef(new Pause('Popup')); - const prevActivator = useRef(null); - const prevProps = useRef(null); const handleKeyDown = useCallback((ev) => { if (no5WayClose) return; @@ -407,6 +406,54 @@ const Popup = (props) => { } }, [handleKeyDown, open]); + const getDerivedStateFromProps = useCallback(() => { + if (open !== prevOpen) { + if (open) { + setPopupOpen(noAnimation || floatLayerOpen ? OpenState.OPEN : OpenState.CLOSED); + setFloatLayerOpen(true); + setActivator(Spotlight.getCurrent()); + setPrevOpen(open); + } else { + // Disables the spotlight conatiner of popup when `noAnimation` set + if (noAnimation) { + const node = getContainerNode(containerId.current); + if (node) { + node.dataset['spotlightContainerDisabled'] = true; + } + } + + setPopupOpen(OpenState.CLOSED); + setFloatLayerOpen(popupOpen === OpenState.OPEN ? !noAnimation : false); + setActivator(noAnimation ? null : activator); + setPrevOpen(open); + } + } + }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); + + const handleComponentUpdate = useCallback(() => { + if (open !== prevOpen) { + if (!noAnimation) { + if (!open && popupOpen === OpenState.CLOSED) { + // If the popup is supposed to be closed (!this.props.open) and is actually + // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This + // can occur when quickly toggling a Popup open and closed. + paused.current.resume(); + } else { + // Otherwise, we pause spotlight so it is locked until the popup is ready + paused.current.pause(); + } + } else if (open) { + forwardShow(null, allComponentProps); + spotPopupContent(); + } else if (prevOpen) { + forwardHide(null, allComponentProps); + spotActivator(activator); + } + } + + checkScrimNone(allComponentProps); + }, [activator, allComponentProps, noAnimation, open, popupOpen, prevOpen, spotActivator, spotPopupContent]); + const handleDismiss = useCallback((ev) => { forwardCustom('onClose', () => ({detail: ev?.detail}))(null, allComponentProps); }, [allComponentProps]); @@ -441,74 +488,34 @@ const Popup = (props) => { }, [allComponentProps, spotPopupContent]); useEffect(() => { - if (open !== prevOpen) { - if (open) { - setPopupOpen(noAnimation || floatLayerOpen ? OpenState.OPEN : OpenState.CLOSED); - setFloatLayerOpen(true); - setActivator(Spotlight.getCurrent()); - setPrevOpen(open); - } else { - // Disables the spotlight conatiner of popup when `noAnimation` set - if (noAnimation) { - const node = getContainerNode(containerId.current); - if (node) { - node.dataset['spotlightContainerDisabled'] = true; - } - } + componentMounted.current = true; - setPopupOpen(OpenState.CLOSED); - setFloatLayerOpen(popupOpen === OpenState.OPEN ? !noAnimation : false); - setActivator(noAnimation ? null : activator); - setPrevOpen(open); - } - } - }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); + return () => { + componentMounted.current = false; + }; + }, []); useEffect(() => { - prevActivator.current = activator; - }, [activator]); + getDerivedStateFromProps(); + }, [getDerivedStateFromProps, handleComponentUpdate]); useEffect(() => { checkScrimNone(allComponentProps); - prevProps.current = allComponentProps; }, [allComponentProps]); useEffect(() => { const id = containerId.current; + return () => { - if (open) { - off('keydown', handleKeyDown); - } - if (!open) { + if (componentMounted.current === false) { + if (open) { + off('keydown', handleKeyDown); + } Spotlight.remove(id); } }; }, [handleKeyDown, open]); - useEffect(() => { - if (open !== prevProps.current?.open) { - if (!noAnimation) { - if (!open && popupOpen === OpenState.CLOSED) { - // If the popup is supposed to be closed (!this.props.open) and is actually - // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This - // can occur when quickly toggling a Popup open and closed. - paused.current.resume(); - } else { - // Otherwise, we pause spotlight so it is locked until the popup is ready - paused.current.pause(); - } - } else if (open) { - forwardShow(null, allComponentProps); - spotPopupContent(); - } else if (prevProps.current?.open) { - forwardHide(null, allComponentProps); - spotActivator(prevActivator.current); - } - } - - checkScrimNone(allComponentProps); - }, [allComponentProps, noAnimation, open, popupOpen, spotActivator, spotPopupContent]); - return ( Date: Fri, 18 Oct 2024 17:52:10 +0300 Subject: [PATCH 09/30] Added target for the `on` and `off` listeners --- Popup/Popup.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index adf21bc3e..0c5b6be7b 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -365,7 +365,7 @@ const Popup = (props) => { const containerNode = getContainerNode(containerId.current); const lastContainerId = getLastContainer(); - off('keydown', handleKeyDown); + off('keydown', handleKeyDown, containerNode); // if there is no currently-spotted control or it is wrapped by the popup's container, we // know it's safe to change focus @@ -391,7 +391,7 @@ const Popup = (props) => { // only spot the activator if the popup is open if (!open) return; - on('keydown', handleKeyDown); + on('keydown', handleKeyDown, getContainerNode(containerId.current)); if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { const current = Spotlight.getCurrent(); @@ -509,7 +509,7 @@ const Popup = (props) => { return () => { if (componentMounted.current === false) { if (open) { - off('keydown', handleKeyDown); + off('keydown', handleKeyDown, getContainerNode(id)); } Spotlight.remove(id); } From f36c0875b00d2fa96b93007c95790c3fd44b549b Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Mon, 21 Oct 2024 21:57:08 +0300 Subject: [PATCH 10/30] Created reference for `handleKeyDown` function --- Popup/Popup.js | 47 +++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 0c5b6be7b..8b9dd1d9d 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -312,7 +312,7 @@ const Popup = (props) => { const componentMounted = useRef(false); const paused = useRef(new Pause('Popup')); - const handleKeyDown = useCallback((ev) => { + const handleKeyDown = (ev) => { if (no5WayClose) return; const keyCode = ev.keyCode; @@ -353,27 +353,29 @@ const Popup = (props) => { } } } - }, [allComponentProps, no5WayClose, onClose, position, spotlightRestrict]); + }; - const spotActivator = useCallback((evActivator) => { + const handleKeyDownRef = useRef(handleKeyDown); + + const spotActivator = useCallback(() => { paused.current.resume(); // only spot the activator if the popup is closed if (open) return; const current = Spotlight.getCurrent(); - const containerNode = getContainerNode(containerId.current); + const containerNode = getContainerNode(containerId); const lastContainerId = getLastContainer(); - off('keydown', handleKeyDown, containerNode); + off('keydown', handleKeyDownRef.current); // if there is no currently-spotted control or it is wrapped by the popup's container, we // know it's safe to change focus if (!current || (containerNode && containerNode.contains(current))) { // attempt to set focus to the activator, if available if (!Spotlight.isPaused()) { - if (evActivator) { - if (!Spotlight.focus(evActivator)) { + if (activator) { + if (!Spotlight.focus(activator)) { Spotlight.focus(); } } else { @@ -383,7 +385,7 @@ const Popup = (props) => { } } } - }, [handleKeyDown, open]); + }, [activator, open]); const spotPopupContent = useCallback(() => { paused.current.resume(); @@ -391,7 +393,7 @@ const Popup = (props) => { // only spot the activator if the popup is open if (!open) return; - on('keydown', handleKeyDown, getContainerNode(containerId.current)); + on('keydown', handleKeyDownRef.current); if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { const current = Spotlight.getCurrent(); @@ -404,7 +406,7 @@ const Popup = (props) => { } Spotlight.setActiveContainer(containerId.current); } - }, [handleKeyDown, open]); + }, [open]); const getDerivedStateFromProps = useCallback(() => { if (open !== prevOpen) { @@ -433,7 +435,7 @@ const Popup = (props) => { const handleComponentUpdate = useCallback(() => { if (open !== prevOpen) { if (!noAnimation) { - if (!open && popupOpen === OpenState.CLOSED) { + if (!open && popupOpen === OpenState.OPENING || !open && popupOpen === OpenState.OPEN) { // If the popup is supposed to be closed (!this.props.open) and is actually // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This // can occur when quickly toggling a Popup open and closed. @@ -447,12 +449,12 @@ const Popup = (props) => { spotPopupContent(); } else if (prevOpen) { forwardHide(null, allComponentProps); - spotActivator(activator); + spotActivator(); } } checkScrimNone(allComponentProps); - }, [activator, allComponentProps, noAnimation, open, popupOpen, prevOpen, spotActivator, spotPopupContent]); + }, [allComponentProps, noAnimation, open, popupOpen, prevOpen, spotActivator, spotPopupContent]); const handleDismiss = useCallback((ev) => { forwardCustom('onClose', () => ({detail: ev?.detail}))(null, allComponentProps); @@ -473,9 +475,9 @@ const Popup = (props) => { setActivator(null); if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { - spotActivator(activator); + spotActivator(); } - }, [activator, allComponentProps, spotActivator]); + }, [allComponentProps, spotActivator]); const handlePopupShow = useCallback((ev) => { forwardShow(ev, allComponentProps); @@ -497,24 +499,29 @@ const Popup = (props) => { useEffect(() => { getDerivedStateFromProps(); - }, [getDerivedStateFromProps, handleComponentUpdate]); + }, [getDerivedStateFromProps]); + + useEffect(() => { + handleComponentUpdate(); + }, [handleComponentUpdate]); useEffect(() => { checkScrimNone(allComponentProps); }, [allComponentProps]); useEffect(() => { - const id = containerId.current; + const idRef = containerId.current; + const keyDownRef = handleKeyDownRef.current; return () => { if (componentMounted.current === false) { if (open) { - off('keydown', handleKeyDown, getContainerNode(id)); + off('keydown', keyDownRef); } - Spotlight.remove(id); + Spotlight.remove(idRef); } }; - }, [handleKeyDown, open]); + }, [open]); return ( Date: Tue, 22 Oct 2024 10:10:54 +0300 Subject: [PATCH 11/30] Created reference for `containerId` only when component is mounted --- Popup/Popup.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 8b9dd1d9d..438caa5f5 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -308,7 +308,7 @@ const Popup = (props) => { const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); - const containerId = useRef(Spotlight.add()); + const containerId = useRef(null); const componentMounted = useRef(false); const paused = useRef(new Pause('Popup')); @@ -364,7 +364,7 @@ const Popup = (props) => { if (open) return; const current = Spotlight.getCurrent(); - const containerNode = getContainerNode(containerId); + const containerNode = getContainerNode(containerId.current); const lastContainerId = getLastContainer(); off('keydown', handleKeyDownRef.current); @@ -490,6 +490,7 @@ const Popup = (props) => { }, [allComponentProps, spotPopupContent]); useEffect(() => { + containerId.current = Spotlight.add(); componentMounted.current = true; return () => { From b562900e2acfd4775929d536c89fa2877dbb2121 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Tue, 22 Oct 2024 10:21:28 +0300 Subject: [PATCH 12/30] Component refactor --- Popup/Popup.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 438caa5f5..a7dccd14f 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -308,8 +308,8 @@ const Popup = (props) => { const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); - const containerId = useRef(null); const componentMounted = useRef(false); + const containerId = useRef(null); const paused = useRef(new Pause('Popup')); const handleKeyDown = (ev) => { @@ -490,8 +490,8 @@ const Popup = (props) => { }, [allComponentProps, spotPopupContent]); useEffect(() => { - containerId.current = Spotlight.add(); componentMounted.current = true; + containerId.current = Spotlight.add(); return () => { componentMounted.current = false; @@ -500,11 +500,8 @@ const Popup = (props) => { useEffect(() => { getDerivedStateFromProps(); - }, [getDerivedStateFromProps]); - - useEffect(() => { handleComponentUpdate(); - }, [handleComponentUpdate]); + }, [getDerivedStateFromProps, handleComponentUpdate]); useEffect(() => { checkScrimNone(allComponentProps); From 0426396638264dff9bb6233044f5619b87052af5 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Tue, 22 Oct 2024 11:45:19 +0300 Subject: [PATCH 13/30] Added `useEffect` to spot the Popup content after it's mounted and open --- Popup/Popup.js | 51 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index a7dccd14f..413a83882 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -304,12 +304,12 @@ const Popup = (props) => { const {noAutoDismiss, no5WayClose, onClose, scrimType, ...rest} = allComponentProps; const [activator, setActivator] = useState(null); + const [containerId, setContainerId] = useState(null); const [floatLayerOpen, setFloatLayerOpen] = useState(open); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); const componentMounted = useRef(false); - const containerId = useRef(null); const paused = useRef(new Pause('Popup')); const handleKeyDown = (ev) => { @@ -317,12 +317,12 @@ const Popup = (props) => { const keyCode = ev.keyCode; const direction = getDirection(keyCode); - const spottables = Spotlight.getSpottableDescendants(containerId.current).length; + const spottables = Spotlight.getSpottableDescendants(containerId).length; const current = Spotlight.getCurrent(); - if (direction && (!spottables || current && getContainerNode(containerId.current).contains(current))) { + if (direction && (!spottables || current && getContainerNode(containerId).contains(current))) { // explicitly restrict navigation in order to manage focus state when attempting to leave the popup - Spotlight.set(containerId.current, {restrict: 'self-only'}); + Spotlight.set(containerId, {restrict: 'self-only'}); if (onClose) { let focusChanged; @@ -364,7 +364,7 @@ const Popup = (props) => { if (open) return; const current = Spotlight.getCurrent(); - const containerNode = getContainerNode(containerId.current); + const containerNode = getContainerNode(containerId); const lastContainerId = getLastContainer(); off('keydown', handleKeyDownRef.current); @@ -385,7 +385,7 @@ const Popup = (props) => { } } } - }, [activator, open]); + }, [activator, containerId, open]); const spotPopupContent = useCallback(() => { paused.current.resume(); @@ -395,7 +395,7 @@ const Popup = (props) => { on('keydown', handleKeyDownRef.current); - if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { + if (!Spotlight.isPaused() && !Spotlight.focus(containerId)) { const current = Spotlight.getCurrent(); // In cases where the container contains no spottable controls or we're in pointer-mode, focus @@ -404,9 +404,9 @@ const Popup = (props) => { if (current) { current.blur(); } - Spotlight.setActiveContainer(containerId.current); + Spotlight.setActiveContainer(containerId); } - }, [open]); + }, [containerId, open]); const getDerivedStateFromProps = useCallback(() => { if (open !== prevOpen) { @@ -418,7 +418,7 @@ const Popup = (props) => { } else { // Disables the spotlight conatiner of popup when `noAnimation` set if (noAnimation) { - const node = getContainerNode(containerId.current); + const node = getContainerNode(containerId); if (node) { node.dataset['spotlightContainerDisabled'] = true; } @@ -430,7 +430,7 @@ const Popup = (props) => { setPrevOpen(open); } } - }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); + }, [activator, containerId, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); const handleComponentUpdate = useCallback(() => { if (open !== prevOpen) { @@ -474,30 +474,42 @@ const Popup = (props) => { setFloatLayerOpen(false); setActivator(null); - if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { + if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId) { spotActivator(); } - }, [allComponentProps, spotActivator]); + }, [allComponentProps, containerId, spotActivator]); const handlePopupShow = useCallback((ev) => { forwardShow(ev, allComponentProps); setPopupOpen(OpenState.OPEN); - if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { + if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId) { spotPopupContent(); } - }, [allComponentProps, spotPopupContent]); + }, [allComponentProps, containerId, spotPopupContent]); useEffect(() => { componentMounted.current = true; - containerId.current = Spotlight.add(); + setContainerId(Spotlight.add()); return () => { componentMounted.current = false; }; }, []); + // Spot the content after it's mounted. + useEffect(() => { + if (open) { + // If the popup is open on mount, we need to pause spotlight so nothing steals focus + // while the popup is rendering. + paused.current.pause(); + if (getContainerNode(containerId)) { + spotPopupContent(); + } + } + }, [containerId, open, spotPopupContent]); + useEffect(() => { getDerivedStateFromProps(); handleComponentUpdate(); @@ -508,7 +520,6 @@ const Popup = (props) => { }, [allComponentProps]); useEffect(() => { - const idRef = containerId.current; const keyDownRef = handleKeyDownRef.current; return () => { @@ -516,10 +527,10 @@ const Popup = (props) => { if (open) { off('keydown', keyDownRef); } - Spotlight.remove(idRef); + Spotlight.remove(containerId); } }; - }, [open]); + }, [open, containerId]); return ( { onHide={handlePopupHide} onShow={handlePopupShow} open={popupOpen >= OpenState.OPENING} - spotlightId={containerId.current} + spotlightId={containerId} /> ); From 96a62b469945269c8ab701c18eca967383838a8f Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Tue, 22 Oct 2024 12:22:42 +0300 Subject: [PATCH 14/30] Added default value for `containerId` --- Popup/Popup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 413a83882..33dc61759 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -304,7 +304,7 @@ const Popup = (props) => { const {noAutoDismiss, no5WayClose, onClose, scrimType, ...rest} = allComponentProps; const [activator, setActivator] = useState(null); - const [containerId, setContainerId] = useState(null); + const [containerId, setContainerId] = useState(Spotlight.add()); const [floatLayerOpen, setFloatLayerOpen] = useState(open); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); From c3a3ade20a63513b9ccffbf643098f59ccb0c108 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 23 Oct 2024 13:18:53 +0300 Subject: [PATCH 15/30] Added `Pause` class in `useState` instead of `useRef` --- Popup/Popup.js | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 33dc61759..80aa5d1a4 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -306,11 +306,11 @@ const Popup = (props) => { const [activator, setActivator] = useState(null); const [containerId, setContainerId] = useState(Spotlight.add()); const [floatLayerOpen, setFloatLayerOpen] = useState(open); + const [paused, setPaused] = useState(new Pause('Popup')); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); const componentMounted = useRef(false); - const paused = useRef(new Pause('Popup')); const handleKeyDown = (ev) => { if (no5WayClose) return; @@ -358,7 +358,7 @@ const Popup = (props) => { const handleKeyDownRef = useRef(handleKeyDown); const spotActivator = useCallback(() => { - paused.current.resume(); + paused.resume(); // only spot the activator if the popup is closed if (open) return; @@ -373,7 +373,7 @@ const Popup = (props) => { // know it's safe to change focus if (!current || (containerNode && containerNode.contains(current))) { // attempt to set focus to the activator, if available - if (!Spotlight.isPaused()) { + if (Spotlight.isPaused()) { if (activator) { if (!Spotlight.focus(activator)) { Spotlight.focus(); @@ -385,10 +385,10 @@ const Popup = (props) => { } } } - }, [activator, containerId, open]); + }, [activator, containerId, open, paused]); const spotPopupContent = useCallback(() => { - paused.current.resume(); + paused.resume(); // only spot the activator if the popup is open if (!open) return; @@ -406,7 +406,7 @@ const Popup = (props) => { } Spotlight.setActiveContainer(containerId); } - }, [containerId, open]); + }, [containerId, open, paused]); const getDerivedStateFromProps = useCallback(() => { if (open !== prevOpen) { @@ -439,10 +439,10 @@ const Popup = (props) => { // If the popup is supposed to be closed (!this.props.open) and is actually // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This // can occur when quickly toggling a Popup open and closed. - paused.current.resume(); + paused.resume(); } else { // Otherwise, we pause spotlight so it is locked until the popup is ready - paused.current.pause(); + paused.pause(); } } else if (open) { forwardShow(null, allComponentProps); @@ -454,7 +454,7 @@ const Popup = (props) => { } checkScrimNone(allComponentProps); - }, [allComponentProps, noAnimation, open, popupOpen, prevOpen, spotActivator, spotPopupContent]); + }, [allComponentProps, noAnimation, open, paused, popupOpen, prevOpen, spotActivator, spotPopupContent]); const handleDismiss = useCallback((ev) => { forwardCustom('onClose', () => ({detail: ev?.detail}))(null, allComponentProps); @@ -492,6 +492,7 @@ const Popup = (props) => { useEffect(() => { componentMounted.current = true; setContainerId(Spotlight.add()); + setPaused(new Pause('Popup')); return () => { componentMounted.current = false; @@ -500,15 +501,15 @@ const Popup = (props) => { // Spot the content after it's mounted. useEffect(() => { - if (open) { + if (open && componentMounted.current) { // If the popup is open on mount, we need to pause spotlight so nothing steals focus // while the popup is rendering. - paused.current.pause(); + paused.pause(); if (getContainerNode(containerId)) { spotPopupContent(); } } - }, [containerId, open, spotPopupContent]); + }, [containerId, open, paused, spotPopupContent]); useEffect(() => { getDerivedStateFromProps(); From c9ca4221dff7a99140a64a745acd166f3c77ec97 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 23 Oct 2024 15:07:03 +0300 Subject: [PATCH 16/30] Changed condition for `spotActivator` --- Popup/Popup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 80aa5d1a4..e8c141eb0 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -373,7 +373,7 @@ const Popup = (props) => { // know it's safe to change focus if (!current || (containerNode && containerNode.contains(current))) { // attempt to set focus to the activator, if available - if (Spotlight.isPaused()) { + if (!paused.isPaused()) { if (activator) { if (!Spotlight.focus(activator)) { Spotlight.focus(); From 293f95cb3c94c9bf65d8750b3b37014cf91c645b Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 23 Oct 2024 17:07:15 +0300 Subject: [PATCH 17/30] Changed condition for `spotActivator` --- Popup/Popup.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index e8c141eb0..5d9076d5e 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -373,7 +373,7 @@ const Popup = (props) => { // know it's safe to change focus if (!current || (containerNode && containerNode.contains(current))) { // attempt to set focus to the activator, if available - if (!paused.isPaused()) { + if (!Spotlight.isPaused()) { if (activator) { if (!Spotlight.focus(activator)) { Spotlight.focus(); @@ -395,6 +395,8 @@ const Popup = (props) => { on('keydown', handleKeyDownRef.current); + console.log(getContainerNode(containerId)); + if (!Spotlight.isPaused() && !Spotlight.focus(containerId)) { const current = Spotlight.getCurrent(); @@ -501,7 +503,7 @@ const Popup = (props) => { // Spot the content after it's mounted. useEffect(() => { - if (open && componentMounted.current) { + if (open && prevOpen) { // If the popup is open on mount, we need to pause spotlight so nothing steals focus // while the popup is rendering. paused.pause(); @@ -509,7 +511,7 @@ const Popup = (props) => { spotPopupContent(); } } - }, [containerId, open, paused, spotPopupContent]); + }, [containerId, open, paused, prevOpen, spotPopupContent]); useEffect(() => { getDerivedStateFromProps(); From 1729bb790c0c97dd55e87073cc7297b251dc34d4 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 23 Oct 2024 17:18:18 +0300 Subject: [PATCH 18/30] Fixed lint warnings --- Popup/Popup.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 5d9076d5e..37c195a71 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -395,8 +395,6 @@ const Popup = (props) => { on('keydown', handleKeyDownRef.current); - console.log(getContainerNode(containerId)); - if (!Spotlight.isPaused() && !Spotlight.focus(containerId)) { const current = Spotlight.getCurrent(); From 5c06c2f0be4d5a7be3d934d988680f96936b36e3 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Wed, 23 Oct 2024 17:43:39 +0300 Subject: [PATCH 19/30] Moved `Pause` class into `useRef` --- Popup/Popup.js | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 37c195a71..2bfdfd24c 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -306,11 +306,11 @@ const Popup = (props) => { const [activator, setActivator] = useState(null); const [containerId, setContainerId] = useState(Spotlight.add()); const [floatLayerOpen, setFloatLayerOpen] = useState(open); - const [paused, setPaused] = useState(new Pause('Popup')); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); const componentMounted = useRef(false); + const paused = useRef(new Pause('Popup')); const handleKeyDown = (ev) => { if (no5WayClose) return; @@ -358,7 +358,7 @@ const Popup = (props) => { const handleKeyDownRef = useRef(handleKeyDown); const spotActivator = useCallback(() => { - paused.resume(); + paused.current.resume(); // only spot the activator if the popup is closed if (open) return; @@ -385,10 +385,10 @@ const Popup = (props) => { } } } - }, [activator, containerId, open, paused]); + }, [activator, containerId, open]); const spotPopupContent = useCallback(() => { - paused.resume(); + paused.current.resume(); // only spot the activator if the popup is open if (!open) return; @@ -406,7 +406,7 @@ const Popup = (props) => { } Spotlight.setActiveContainer(containerId); } - }, [containerId, open, paused]); + }, [containerId, open]); const getDerivedStateFromProps = useCallback(() => { if (open !== prevOpen) { @@ -439,10 +439,10 @@ const Popup = (props) => { // If the popup is supposed to be closed (!this.props.open) and is actually // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This // can occur when quickly toggling a Popup open and closed. - paused.resume(); + paused.current.resume(); } else { // Otherwise, we pause spotlight so it is locked until the popup is ready - paused.pause(); + paused.current.pause(); } } else if (open) { forwardShow(null, allComponentProps); @@ -454,7 +454,7 @@ const Popup = (props) => { } checkScrimNone(allComponentProps); - }, [allComponentProps, noAnimation, open, paused, popupOpen, prevOpen, spotActivator, spotPopupContent]); + }, [allComponentProps, noAnimation, open, popupOpen, prevOpen, spotActivator, spotPopupContent]); const handleDismiss = useCallback((ev) => { forwardCustom('onClose', () => ({detail: ev?.detail}))(null, allComponentProps); @@ -492,7 +492,6 @@ const Popup = (props) => { useEffect(() => { componentMounted.current = true; setContainerId(Spotlight.add()); - setPaused(new Pause('Popup')); return () => { componentMounted.current = false; @@ -504,12 +503,12 @@ const Popup = (props) => { if (open && prevOpen) { // If the popup is open on mount, we need to pause spotlight so nothing steals focus // while the popup is rendering. - paused.pause(); + paused.current.pause(); if (getContainerNode(containerId)) { spotPopupContent(); } } - }, [containerId, open, paused, prevOpen, spotPopupContent]); + }, [containerId, open, prevOpen, spotPopupContent]); useEffect(() => { getDerivedStateFromProps(); From a1a2e858ce5009cfa4437eadb930d8764d4c408d Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 24 Oct 2024 11:25:04 +0300 Subject: [PATCH 20/30] Fixed focus issues on Popup and refactored the code --- Popup/Popup.js | 72 +++++++++++++++++++++----------------------------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 2bfdfd24c..df4a07d3e 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -304,12 +304,11 @@ const Popup = (props) => { const {noAutoDismiss, no5WayClose, onClose, scrimType, ...rest} = allComponentProps; const [activator, setActivator] = useState(null); - const [containerId, setContainerId] = useState(Spotlight.add()); const [floatLayerOpen, setFloatLayerOpen] = useState(open); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); - const componentMounted = useRef(false); + const containerId = useRef(Spotlight.add()); const paused = useRef(new Pause('Popup')); const handleKeyDown = (ev) => { @@ -317,12 +316,12 @@ const Popup = (props) => { const keyCode = ev.keyCode; const direction = getDirection(keyCode); - const spottables = Spotlight.getSpottableDescendants(containerId).length; + const spottables = Spotlight.getSpottableDescendants(containerId.current).length; const current = Spotlight.getCurrent(); - if (direction && (!spottables || current && getContainerNode(containerId).contains(current))) { + if (direction && (!spottables || current && getContainerNode(containerId.current).contains(current))) { // explicitly restrict navigation in order to manage focus state when attempting to leave the popup - Spotlight.set(containerId, {restrict: 'self-only'}); + Spotlight.set(containerId.current, {restrict: 'self-only'}); if (onClose) { let focusChanged; @@ -364,7 +363,7 @@ const Popup = (props) => { if (open) return; const current = Spotlight.getCurrent(); - const containerNode = getContainerNode(containerId); + const containerNode = getContainerNode(containerId.current); const lastContainerId = getLastContainer(); off('keydown', handleKeyDownRef.current); @@ -385,7 +384,7 @@ const Popup = (props) => { } } } - }, [activator, containerId, open]); + }, [activator, open]); const spotPopupContent = useCallback(() => { paused.current.resume(); @@ -395,7 +394,7 @@ const Popup = (props) => { on('keydown', handleKeyDownRef.current); - if (!Spotlight.isPaused() && !Spotlight.focus(containerId)) { + if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { const current = Spotlight.getCurrent(); // In cases where the container contains no spottable controls or we're in pointer-mode, focus @@ -404,9 +403,9 @@ const Popup = (props) => { if (current) { current.blur(); } - Spotlight.setActiveContainer(containerId); + Spotlight.setActiveContainer(containerId.current); } - }, [containerId, open]); + }, [open]); const getDerivedStateFromProps = useCallback(() => { if (open !== prevOpen) { @@ -418,7 +417,7 @@ const Popup = (props) => { } else { // Disables the spotlight conatiner of popup when `noAnimation` set if (noAnimation) { - const node = getContainerNode(containerId); + const node = getContainerNode(containerId.current); if (node) { node.dataset['spotlightContainerDisabled'] = true; } @@ -430,7 +429,7 @@ const Popup = (props) => { setPrevOpen(open); } } - }, [activator, containerId, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); + }, [activator, floatLayerOpen, noAnimation, open, popupOpen, prevOpen]); const handleComponentUpdate = useCallback(() => { if (open !== prevOpen) { @@ -474,41 +473,43 @@ const Popup = (props) => { setFloatLayerOpen(false); setActivator(null); - if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId) { + if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { spotActivator(); } - }, [allComponentProps, containerId, spotActivator]); + }, [allComponentProps, spotActivator]); const handlePopupShow = useCallback((ev) => { forwardShow(ev, allComponentProps); setPopupOpen(OpenState.OPEN); - if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId) { + if (!ev.currentTarget || ev.currentTarget.getAttribute('data-spotlight-id') === containerId.current) { spotPopupContent(); } - }, [allComponentProps, containerId, spotPopupContent]); - - useEffect(() => { - componentMounted.current = true; - setContainerId(Spotlight.add()); - - return () => { - componentMounted.current = false; - }; - }, []); + }, [allComponentProps, spotPopupContent]); // Spot the content after it's mounted. useEffect(() => { - if (open && prevOpen) { + if (open) { // If the popup is open on mount, we need to pause spotlight so nothing steals focus // while the popup is rendering. paused.current.pause(); - if (getContainerNode(containerId)) { + if (getContainerNode(containerId.current)) { spotPopupContent(); } } - }, [containerId, open, prevOpen, spotPopupContent]); + + const idRef = containerId.current; + const keyDownRef = handleKeyDownRef.current; + + return () => { + if (open) { + off('keydown', keyDownRef); + } + Spotlight.remove(idRef); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); useEffect(() => { getDerivedStateFromProps(); @@ -519,19 +520,6 @@ const Popup = (props) => { checkScrimNone(allComponentProps); }, [allComponentProps]); - useEffect(() => { - const keyDownRef = handleKeyDownRef.current; - - return () => { - if (componentMounted.current === false) { - if (open) { - off('keydown', keyDownRef); - } - Spotlight.remove(containerId); - } - }; - }, [open, containerId]); - return ( { onHide={handlePopupHide} onShow={handlePopupShow} open={popupOpen >= OpenState.OPENING} - spotlightId={containerId} + spotlightId={containerId.current} /> ); From 8d29d81b6b78d1379f9d10344b30c42386faa97f Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 24 Oct 2024 14:49:10 +0300 Subject: [PATCH 21/30] Fixed focus issues on Input --- Popup/Popup.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index df4a07d3e..f567b3a02 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -507,7 +507,7 @@ const Popup = (props) => { off('keydown', keyDownRef); } Spotlight.remove(idRef); - } + }; // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -516,10 +516,6 @@ const Popup = (props) => { handleComponentUpdate(); }, [getDerivedStateFromProps, handleComponentUpdate]); - useEffect(() => { - checkScrimNone(allComponentProps); - }, [allComponentProps]); - return ( { data-webos-voice-exclusive onHide={handlePopupHide} onShow={handlePopupShow} - open={popupOpen >= OpenState.OPENING} + open={popupOpen >= OpenState.OPENING && open} spotlightId={containerId.current} /> From 36162d3ea65e6cd10592e076ac1681d3a44c5d90 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 24 Oct 2024 21:25:33 +0300 Subject: [PATCH 22/30] Forced `Spotlight` to focus the last focused element when the `Popup` is closed --- Popup/Popup.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Popup/Popup.js b/Popup/Popup.js index f567b3a02..0c2a7eed7 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -511,6 +511,12 @@ const Popup = (props) => { // eslint-disable-next-line react-hooks/exhaustive-deps }, []); + useEffect(() => { + if (!open && activator) { + Spotlight.focus(activator) + } + }, [activator, open]); + useEffect(() => { getDerivedStateFromProps(); handleComponentUpdate(); From 4f96a9c333aed016e9f22cc0d54575f42b53f895 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 24 Oct 2024 21:44:53 +0300 Subject: [PATCH 23/30] Fixed lint warnings --- Popup/Popup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 0c2a7eed7..a1ba82cc1 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -513,7 +513,7 @@ const Popup = (props) => { useEffect(() => { if (!open && activator) { - Spotlight.focus(activator) + Spotlight.focus(activator); } }, [activator, open]); From 421211b0b5cb562f67e4ff2fd481e226ceb68477 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Fri, 25 Oct 2024 10:29:36 +0300 Subject: [PATCH 24/30] Added condition to focus `activator` when the `Popup` is closed and removed unnecessary `useEffect` --- Popup/Popup.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index a1ba82cc1..d39a32214 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -372,7 +372,7 @@ const Popup = (props) => { // know it's safe to change focus if (!current || (containerNode && containerNode.contains(current))) { // attempt to set focus to the activator, if available - if (!Spotlight.isPaused()) { + if (!Spotlight.isPaused() || !paused.current.isPaused()) { if (activator) { if (!Spotlight.focus(activator)) { Spotlight.focus(); @@ -511,12 +511,6 @@ const Popup = (props) => { // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - useEffect(() => { - if (!open && activator) { - Spotlight.focus(activator); - } - }, [activator, open]); - useEffect(() => { getDerivedStateFromProps(); handleComponentUpdate(); From 120af6384a796533e9f1020e7162039eff769eb0 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Fri, 25 Oct 2024 11:55:13 +0300 Subject: [PATCH 25/30] Changed the comment for `handleComponentUpdate` function --- Popup/Popup.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index d39a32214..5416d492b 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -435,8 +435,8 @@ const Popup = (props) => { if (open !== prevOpen) { if (!noAnimation) { if (!open && popupOpen === OpenState.OPENING || !open && popupOpen === OpenState.OPEN) { - // If the popup is supposed to be closed (!this.props.open) and is actually - // fully closed (OpenState.CLOSED), we must resume spotlight navigation. This + // If the popup is supposed to be closed (!open) and is actually not fully + // closed (OpenState.OPENING or OpenState.OPEN), we must resume spotlight navigation. This // can occur when quickly toggling a Popup open and closed. paused.current.resume(); } else { From 7b5e6c74e1c2642a774dc704d4f542f25a21145a Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 31 Oct 2024 13:56:00 +0200 Subject: [PATCH 26/30] Review fixes --- Popup/Popup.js | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 5416d492b..b60dd9870 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -300,10 +300,13 @@ const popupDefaultProps = { */ const Popup = (props) => { const allComponentProps = setDefaultProps(props, popupDefaultProps); - const {noAnimation, open, position, spotlightRestrict} = allComponentProps; - const {noAutoDismiss, no5WayClose, onClose, scrimType, ...rest} = allComponentProps; + const {noAnimation, noAutoDismiss, no5WayClose, onClose, open, position, scrimType, spotlightRestrict, ...rest} = allComponentProps; + + // Assigned the needed props to the rest object for the child component + Object.assign(rest, {noAnimation, position, spotlightRestrict}); const [activator, setActivator] = useState(null); + const [addedEventListener, setAddedEventListener] = useState(false); const [floatLayerOpen, setFloatLayerOpen] = useState(open); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); @@ -311,7 +314,7 @@ const Popup = (props) => { const containerId = useRef(Spotlight.add()); const paused = useRef(new Pause('Popup')); - const handleKeyDown = (ev) => { + const handleKeyDown = useCallback((ev) => { if (no5WayClose) return; const keyCode = ev.keyCode; @@ -352,7 +355,7 @@ const Popup = (props) => { } } } - }; + }, [allComponentProps, no5WayClose, onClose, position, spotlightRestrict]); const handleKeyDownRef = useRef(handleKeyDown); @@ -367,6 +370,7 @@ const Popup = (props) => { const lastContainerId = getLastContainer(); off('keydown', handleKeyDownRef.current); + setAddedEventListener(false); // if there is no currently-spotted control or it is wrapped by the popup's container, we // know it's safe to change focus @@ -393,6 +397,7 @@ const Popup = (props) => { if (!open) return; on('keydown', handleKeyDownRef.current); + setAddedEventListener(true); if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { const current = Spotlight.getCurrent(); @@ -505,6 +510,7 @@ const Popup = (props) => { return () => { if (open) { off('keydown', keyDownRef); + setAddedEventListener(false); } Spotlight.remove(idRef); }; @@ -516,6 +522,15 @@ const Popup = (props) => { handleComponentUpdate(); }, [getDerivedStateFromProps, handleComponentUpdate]); + // Remove the keydown listener and add a new listener when the handleKeyDown function is re-created + useEffect(() => { + if (addedEventListener) { + off('keydown', handleKeyDownRef.current); + handleKeyDownRef.current = handleKeyDown; + on('keydown', handleKeyDownRef.current); + } + }, [addedEventListener, handleKeyDown]); + return ( { data-webos-voice-exclusive onHide={handlePopupHide} onShow={handlePopupShow} - open={popupOpen >= OpenState.OPENING && open} + open={popupOpen >= OpenState.OPENING} spotlightId={containerId.current} /> From ddd7a63f9cf047ad896a03eb6d8f33737782cfd2 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Thu, 31 Oct 2024 15:07:19 +0200 Subject: [PATCH 27/30] Review fixes --- Popup/Popup.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index b60dd9870..e43935b6c 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -370,7 +370,6 @@ const Popup = (props) => { const lastContainerId = getLastContainer(); off('keydown', handleKeyDownRef.current); - setAddedEventListener(false); // if there is no currently-spotted control or it is wrapped by the popup's container, we // know it's safe to change focus @@ -397,7 +396,6 @@ const Popup = (props) => { if (!open) return; on('keydown', handleKeyDownRef.current); - setAddedEventListener(true); if (!Spotlight.isPaused() && !Spotlight.focus(containerId.current)) { const current = Spotlight.getCurrent(); @@ -510,7 +508,6 @@ const Popup = (props) => { return () => { if (open) { off('keydown', keyDownRef); - setAddedEventListener(false); } Spotlight.remove(idRef); }; @@ -524,12 +521,12 @@ const Popup = (props) => { // Remove the keydown listener and add a new listener when the handleKeyDown function is re-created useEffect(() => { - if (addedEventListener) { + if (open && handleKeyDownRef.current !== handleKeyDown) { off('keydown', handleKeyDownRef.current); handleKeyDownRef.current = handleKeyDown; - on('keydown', handleKeyDownRef.current); + on('keydown', handleKeyDown); } - }, [addedEventListener, handleKeyDown]); + }, [handleKeyDown, open]); return ( Date: Thu, 31 Oct 2024 15:13:04 +0200 Subject: [PATCH 28/30] Fixed lint warnings --- Popup/Popup.js | 1 - 1 file changed, 1 deletion(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index e43935b6c..776446ab6 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -306,7 +306,6 @@ const Popup = (props) => { Object.assign(rest, {noAnimation, position, spotlightRestrict}); const [activator, setActivator] = useState(null); - const [addedEventListener, setAddedEventListener] = useState(false); const [floatLayerOpen, setFloatLayerOpen] = useState(open); const [popupOpen, setPopupOpen] = useState(open ? OpenState.OPEN : OpenState.CLOSED); const [prevOpen, setPrevOpen] = useState(open); From 93c8dcb9267b56976b6dd30403df0eedc1714181 Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Fri, 1 Nov 2024 11:21:41 +0200 Subject: [PATCH 29/30] Review fixes --- Popup/Popup.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index 776446ab6..f92f847c9 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -501,14 +501,12 @@ const Popup = (props) => { } } - const idRef = containerId.current; - const keyDownRef = handleKeyDownRef.current; - return () => { if (open) { - off('keydown', keyDownRef); + off('keydown', handleKeyDownRef.current); } - Spotlight.remove(idRef); + // eslint-disable-next-line react-hooks/exhaustive-deps + Spotlight.remove(containerId.current); }; // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -518,7 +516,7 @@ const Popup = (props) => { handleComponentUpdate(); }, [getDerivedStateFromProps, handleComponentUpdate]); - // Remove the keydown listener and add a new listener when the handleKeyDown function is re-created + // Remove the keydown listener and add a new listener when the handleKeyDown function is re-created and Popup is open useEffect(() => { if (open && handleKeyDownRef.current !== handleKeyDown) { off('keydown', handleKeyDownRef.current); From 5b0556ddc7e6326f7c26d978882a24462d4de6fd Mon Sep 17 00:00:00 2001 From: Ion Andrusciac Date: Tue, 5 Nov 2024 12:58:59 +0200 Subject: [PATCH 30/30] Fixed `eventListeners` --- Popup/Popup.js | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/Popup/Popup.js b/Popup/Popup.js index f92f847c9..77d5a61b3 100644 --- a/Popup/Popup.js +++ b/Popup/Popup.js @@ -302,7 +302,7 @@ const Popup = (props) => { const allComponentProps = setDefaultProps(props, popupDefaultProps); const {noAnimation, noAutoDismiss, no5WayClose, onClose, open, position, scrimType, spotlightRestrict, ...rest} = allComponentProps; - // Assigned the needed props to the rest object for the child component + // Assign the needed props to the rest object for the child component Object.assign(rest, {noAnimation, position, spotlightRestrict}); const [activator, setActivator] = useState(null); @@ -311,6 +311,8 @@ const Popup = (props) => { const [prevOpen, setPrevOpen] = useState(open); const containerId = useRef(Spotlight.add()); + const handleKeyDownRef = useRef(null); + const openRef = useRef(open); const paused = useRef(new Pause('Popup')); const handleKeyDown = useCallback((ev) => { @@ -356,13 +358,11 @@ const Popup = (props) => { } }, [allComponentProps, no5WayClose, onClose, position, spotlightRestrict]); - const handleKeyDownRef = useRef(handleKeyDown); - const spotActivator = useCallback(() => { paused.current.resume(); // only spot the activator if the popup is closed - if (open) return; + if (open && handleKeyDownRef.current === handleKeyDown) return; const current = Spotlight.getCurrent(); const containerNode = getContainerNode(containerId.current); @@ -386,13 +386,13 @@ const Popup = (props) => { } } } - }, [activator, open]); + }, [activator, handleKeyDown, open]); const spotPopupContent = useCallback(() => { paused.current.resume(); // only spot the activator if the popup is open - if (!open) return; + if (!open && handleKeyDownRef.current === handleKeyDown) return; on('keydown', handleKeyDownRef.current); @@ -407,7 +407,7 @@ const Popup = (props) => { } Spotlight.setActiveContainer(containerId.current); } - }, [open]); + }, [handleKeyDown, open]); const getDerivedStateFromProps = useCallback(() => { if (open !== prevOpen) { @@ -502,7 +502,7 @@ const Popup = (props) => { } return () => { - if (open) { + if (openRef.current) { off('keydown', handleKeyDownRef.current); } // eslint-disable-next-line react-hooks/exhaustive-deps @@ -521,10 +521,14 @@ const Popup = (props) => { if (open && handleKeyDownRef.current !== handleKeyDown) { off('keydown', handleKeyDownRef.current); handleKeyDownRef.current = handleKeyDown; - on('keydown', handleKeyDown); + on('keydown', handleKeyDownRef.current); } }, [handleKeyDown, open]); + useEffect(() => { + openRef.current = open; + }, [open]); + return (