-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix mSafari focus #23922
Fix mSafari focus #23922
Changes from all commits
8da2776
c26a798
d440d63
5ee8d89
abb5935
df7c620
d112369
bb55051
cf2d4f7
2c133cc
950cefb
492afc2
80d93c4
e86230e
cdb9821
aebf8f8
9cc2ce3
23ba998
a90a11b
c93ca14
1cd9326
6791482
6aecd99
46e510a
a993aab
312f8ea
c1a9993
2bb4b7f
1aa5483
0e71b79
0ac3a07
08002be
1900d08
6858d87
57fe87b
aaefbdb
8c079b4
d991008
7fc2195
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You were controlling the |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,8 @@ const propTypes = { | |
const defaultProps = { | ||
...modalDefaultProps, | ||
forwardedRef: () => {}, | ||
coverScreen: true, | ||
customBackdrop: null, | ||
}; | ||
|
||
function BaseModal({ | ||
|
@@ -48,6 +50,8 @@ function BaseModal({ | |
onLayout, | ||
avoidKeyboard, | ||
forwardedRef, | ||
coverScreen, | ||
customBackdrop, | ||
children, | ||
}) { | ||
const {windowWidth, windowHeight, isSmallScreenWidth} = useWindowDimensions(); | ||
|
@@ -91,6 +95,9 @@ function BaseModal({ | |
|
||
// To prevent closing any modal already unmounted when this modal still remains as visible state | ||
Modal.setCloseModal(null); | ||
if (shouldSetModalVisibility) { | ||
Modal.setModalVisibility(false); | ||
} | ||
}, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[], | ||
|
@@ -181,7 +188,8 @@ function BaseModal({ | |
backdropOpacity={hideBackdrop ? 0 : variables.overlayOpacity} | ||
backdropTransitionOutTiming={0} | ||
hasBackdrop={fullscreen} | ||
coverScreen={fullscreen} | ||
coverScreen={!isSmallScreenWidth || coverScreen} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we are already passing coverScreen prop here, can we control its value from the parent This way, it will be more generic and clear that |
||
customBackdrop={customBackdrop && customBackdrop(handleBackdropPress)} | ||
style={modalStyle} | ||
deviceHeight={windowHeight} | ||
deviceWidth={windowWidth} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,14 @@ | ||
import React, {useState} from 'react'; | ||
// eslint-disable-next-line no-restricted-imports | ||
import {Pressable} from 'react-native'; | ||
import withWindowDimensions from '../withWindowDimensions'; | ||
import BaseModal from './BaseModal'; | ||
import {propTypes, defaultProps} from './modalPropTypes'; | ||
import * as StyleUtils from '../../styles/StyleUtils'; | ||
import themeColors from '../../styles/themes/default'; | ||
import StatusBar from '../../libs/StatusBar'; | ||
import CONST from '../../CONST'; | ||
import styles from '../../styles/styles'; | ||
|
||
function Modal(props) { | ||
const [previousStatusBarColor, setPreviousStatusBarColor] = useState(); | ||
|
@@ -40,6 +43,15 @@ function Modal(props) { | |
onModalHide={hideModal} | ||
onModalShow={showModal} | ||
avoidKeyboard={false} | ||
coverScreen={false} | ||
customBackdrop={(onBackdropPress) => ( | ||
<Pressable | ||
onPress={onBackdropPress} | ||
style={styles.modalBackdropWeb} | ||
accessibilityLabel="backdrop" | ||
accessibilityRole={CONST.ACCESSIBILITY_ROLE.ADJUSTABLE} | ||
/> | ||
)} | ||
Comment on lines
+46
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't these changes be done to desktop as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bump. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think they are needed, since this is a fix only for mobile safari, we do not open keyboard on desktop, and the app works correctly there anyway |
||
> | ||
{props.children} | ||
</BaseModal> | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,136 @@ | ||||||||
import _ from 'underscore'; | ||||||||
import React, {useRef, useEffect, useCallback} from 'react'; | ||||||||
import PropTypes from 'prop-types'; | ||||||||
import {View} from 'react-native'; | ||||||||
import PopoverWithMeasuredContent from '../PopoverWithMeasuredContent'; | ||||||||
import styles from '../../styles/styles'; | ||||||||
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions'; | ||||||||
import MenuItem from '../MenuItem'; | ||||||||
import {propTypes as createMenuPropTypes, defaultProps as createMenuDefaultProps} from './popoverMenuPropTypes'; | ||||||||
import refPropTypes from '../refPropTypes'; | ||||||||
import Text from '../Text'; | ||||||||
import CONST from '../../CONST'; | ||||||||
import useArrowKeyFocusManager from '../../hooks/useArrowKeyFocusManager'; | ||||||||
import useKeyboardShortcut from '../../hooks/useKeyboardShortcut'; | ||||||||
import useWindowDimensions from '../../hooks/useWindowDimensions'; | ||||||||
|
||||||||
const propTypes = { | ||||||||
...createMenuPropTypes, | ||||||||
...windowDimensionsPropTypes, | ||||||||
|
||||||||
/** The horizontal and vertical anchors points for the popover */ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
anchorPosition: PropTypes.shape({ | ||||||||
horizontal: PropTypes.number.isRequired, | ||||||||
vertical: PropTypes.number.isRequired, | ||||||||
}).isRequired, | ||||||||
|
||||||||
/** Ref of the anchor */ | ||||||||
anchorRef: refPropTypes, | ||||||||
|
||||||||
/** Where the popover should be positioned relative to the anchor points. */ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
anchorAlignment: PropTypes.shape({ | ||||||||
horizontal: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL)), | ||||||||
vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)), | ||||||||
}), | ||||||||
|
||||||||
/** Indicates whether navigation should occur before closing the modal */ | ||||||||
shouldNavigateBeforeClosingModal: PropTypes.bool, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
||||||||
withoutOverlay: PropTypes.bool, | ||||||||
}; | ||||||||
|
||||||||
const defaultProps = { | ||||||||
...createMenuDefaultProps, | ||||||||
anchorAlignment: { | ||||||||
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, | ||||||||
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM, | ||||||||
}, | ||||||||
shouldNavigateBeforeClosingModal: false, | ||||||||
anchorRef: () => {}, | ||||||||
withoutOverlay: false, | ||||||||
}; | ||||||||
|
||||||||
function BasePopoverMenu(props) { | ||||||||
const {isSmallScreenWidth} = useWindowDimensions(); | ||||||||
const selectedItemIndex = useRef(null); | ||||||||
const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: -1, maxIndex: props.menuItems.length - 1, isActive: props.isVisible}); | ||||||||
|
||||||||
const onModalHide = useCallback(() => { | ||||||||
if (selectedItemIndex.current == null) { | ||||||||
return; | ||||||||
} | ||||||||
|
||||||||
setFocusedIndex(-1); | ||||||||
props.menuItems[selectedItemIndex.current].onSelected(); | ||||||||
selectedItemIndex.current = null; | ||||||||
}, [props.menuItems, selectedItemIndex, setFocusedIndex]); | ||||||||
|
||||||||
useEffect(() => { | ||||||||
if (!props.shouldNavigateBeforeClosingModal) { | ||||||||
return; | ||||||||
} | ||||||||
onModalHide(); | ||||||||
}, [onModalHide, props.shouldNavigateBeforeClosingModal]); | ||||||||
|
||||||||
const selectItem = (index) => { | ||||||||
const selectedItem = props.menuItems[index]; | ||||||||
props.onItemSelected(selectedItem, index); | ||||||||
selectedItemIndex.current = index; | ||||||||
}; | ||||||||
|
||||||||
useKeyboardShortcut( | ||||||||
CONST.KEYBOARD_SHORTCUTS.ENTER, | ||||||||
() => { | ||||||||
if (focusedIndex === -1) { | ||||||||
return; | ||||||||
} | ||||||||
selectItem(focusedIndex); | ||||||||
setFocusedIndex(-1); // Reset the focusedIndex on selecting any menu | ||||||||
}, | ||||||||
{isActive: props.isVisible}, | ||||||||
); | ||||||||
|
||||||||
return ( | ||||||||
<PopoverWithMeasuredContent | ||||||||
anchorPosition={props.anchorPosition} | ||||||||
anchorRef={props.anchorRef} | ||||||||
anchorAlignment={props.anchorAlignment} | ||||||||
onClose={props.onClose} | ||||||||
onModalHide={() => { | ||||||||
if (props.shouldNavigateBeforeClosingModal) { | ||||||||
return; | ||||||||
} | ||||||||
onModalHide(); | ||||||||
}} | ||||||||
isVisible={props.isVisible} | ||||||||
animationIn={props.animationIn} | ||||||||
animationOut={props.animationOut} | ||||||||
animationInTiming={props.animationInTiming} | ||||||||
disableAnimation={props.disableAnimation} | ||||||||
fromSidebarMediumScreen={props.fromSidebarMediumScreen} | ||||||||
withoutOverlay={props.withoutOverlay} | ||||||||
> | ||||||||
<View style={isSmallScreenWidth ? {} : styles.createMenuContainer}> | ||||||||
{!_.isEmpty(props.headerText) && <Text style={[styles.createMenuHeaderText, styles.ml3]}>{props.headerText}</Text>} | ||||||||
{_.map(props.menuItems, (item, menuIndex) => ( | ||||||||
<MenuItem | ||||||||
key={item.text} | ||||||||
icon={item.icon} | ||||||||
iconWidth={item.iconWidth} | ||||||||
iconHeight={item.iconHeight} | ||||||||
title={item.text} | ||||||||
description={item.description} | ||||||||
onPress={() => selectItem(menuIndex)} | ||||||||
focused={focusedIndex === menuIndex} | ||||||||
/> | ||||||||
))} | ||||||||
</View> | ||||||||
</PopoverWithMeasuredContent> | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
BasePopoverMenu.propTypes = propTypes; | ||||||||
BasePopoverMenu.defaultProps = defaultProps; | ||||||||
BasePopoverMenu.displayName = 'BasePopoverMenu'; | ||||||||
|
||||||||
export default React.memo(withWindowDimensions(BasePopoverMenu)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break the form draft features on safari. Going to test it first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it doesn't break the form drafts. That is a different key. Trying to see for other states.