Skip to content
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

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
8da2776
fix mSafari focus
staszekscp Jul 4, 2023
c26a798
add customBackrop for web
staszekscp Jul 4, 2023
d440d63
fix logic
staszekscp Jul 4, 2023
5ee8d89
Merge branch 'main' into fix-msafari-focus
staszekscp Jul 4, 2023
abb5935
fix
staszekscp Jul 4, 2023
df7c620
remove unnecessary line
staszekscp Jul 4, 2023
d112369
simplify changes
staszekscp Jul 4, 2023
bb55051
align logic
staszekscp Jul 4, 2023
cf2d4f7
remove unnecessary line
staszekscp Jul 4, 2023
2c133cc
adjustments
staszekscp Jul 5, 2023
950cefb
fix android
staszekscp Jul 6, 2023
492afc2
Merge branch 'main' into fix-msafari-focus
staszekscp Jul 6, 2023
80d93c4
Merge branch 'main' into fix-msafari-focus
staszekscp Jul 28, 2023
e86230e
fixes
staszekscp Jul 28, 2023
cdb9821
partial fix
staszekscp Jul 28, 2023
aebf8f8
full fix
staszekscp Jul 28, 2023
9cc2ce3
remove unused import
staszekscp Jul 28, 2023
23ba998
fix offline bar
staszekscp Jul 31, 2023
a90a11b
style odjustments
staszekscp Jul 31, 2023
c93ca14
ignore restricted import
staszekscp Jul 31, 2023
1cd9326
add accessibilityLabel:
staszekscp Jul 31, 2023
6791482
prettier
staszekscp Jul 31, 2023
6aecd99
Merge branch 'main' into fix-msafari-focus
staszekscp Aug 1, 2023
46e510a
apply style changes
staszekscp Aug 1, 2023
a993aab
add changes
staszekscp Aug 3, 2023
312f8ea
Merge branch 'main' into fix-msafari-focus
staszekscp Aug 16, 2023
c1a9993
fix BasePopoverMenu
staszekscp Aug 16, 2023
2bb4b7f
improve code reusibility
staszekscp Aug 16, 2023
1aa5483
add comment for onBackdropPress
staszekscp Aug 16, 2023
0e71b79
prettier
staszekscp Aug 16, 2023
0ac3a07
apply requested changes
staszekscp Aug 18, 2023
08002be
additional fix
staszekscp Aug 18, 2023
1900d08
change name
staszekscp Aug 18, 2023
6858d87
merge main
staszekscp Sep 13, 2023
57fe87b
remove test prop
staszekscp Sep 13, 2023
aaefbdb
fixes
staszekscp Sep 15, 2023
8c079b4
revert changes in package-lock.json
staszekscp Sep 15, 2023
d991008
Merge branch 'main' into fix-msafari-focus
staszekscp Sep 15, 2023
7fc2195
fix style import
staszekscp Sep 15, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions src/components/Form.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import ScrollViewWithContext from './ScrollViewWithContext';
import stylePropTypes from '../styles/stylePropTypes';
import {withNetwork} from './OnyxProvider';
import networkPropTypes from './networkPropTypes';
import * as Browser from '../libs/Browser';
import Visibility from '../libs/Visibility';

const propTypes = {
Expand Down Expand Up @@ -534,6 +535,12 @@ export default compose(
withOnyx({
formState: {
key: (props) => props.formID,
// The change was applied to make sure that Mobile Safari opens keyboard on autofocus.
// While performing some async calls (like to local storage) Safari was not behaving as expected -
// it requires so called "user interaction" to open keyboard - eg. a tap. Some longer async calls
// break the "chain" of user interaction events, which caused mSafari to not open keyboard correctly.
// That's why the isMobileSafari check was used to make sure that this change only applies there.
initWithStoredValues: !Browser.isMobileSafari(),
Copy link
Member

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.

Copy link
Member

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.

},
draftValues: {
key: (props) => `${props.formID}Draft`,
Expand Down
10 changes: 9 additions & 1 deletion src/components/Modal/BaseModal.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ const propTypes = {
const defaultProps = {
...modalDefaultProps,
forwardedRef: () => {},
coverScreen: true,
customBackdrop: null,
};

function BaseModal({
Expand All @@ -48,6 +50,8 @@ function BaseModal({
onLayout,
avoidKeyboard,
forwardedRef,
coverScreen,
customBackdrop,
children,
}) {
const {windowWidth, windowHeight, isSmallScreenWidth} = useWindowDimensions();
Expand Down Expand Up @@ -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
[],
Expand Down Expand Up @@ -181,7 +188,8 @@ function BaseModal({
backdropOpacity={hideBackdrop ? 0 : variables.overlayOpacity}
backdropTransitionOutTiming={0}
hasBackdrop={fullscreen}
coverScreen={fullscreen}
coverScreen={!isSmallScreenWidth || coverScreen}
Copy link
Member

Choose a reason for hiding this comment

The 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 index.js file and pass !isSmallScreenWidth there?

This way, it will be more generic and clear that !isSmallScreenWidth is applicable to web | desktop.

customBackdrop={customBackdrop && customBackdrop(handleBackdropPress)}
style={modalStyle}
deviceHeight={windowHeight}
deviceWidth={windowWidth}
Expand Down
12 changes: 12 additions & 0 deletions src/components/Modal/index.web.js
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();
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these changes be done to desktop as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Expand Down
6 changes: 6 additions & 0 deletions src/components/Modal/modalPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ const propTypes = {
/** Should we announce the Modal visibility changes? */
shouldSetModalVisibility: PropTypes.bool,

/** Should we use coverScreen prop for ReactNativeModal? */
coverScreen: PropTypes.bool,

/** The customBackdrop prop allows the addition of a custom backdrop element behind the modal. */
customBackdrop: PropTypes.oneOfType([PropTypes.func, PropTypes.node]),

/** Callback method fired when the user requests to close the modal */
onClose: PropTypes.func.isRequired,

Expand Down
19 changes: 12 additions & 7 deletions src/components/OptionsSelector/index.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import React, {forwardRef} from 'react';
import BaseOptionsSelector from './BaseOptionsSelector';
import useWindowDimensions from '../../hooks/useWindowDimensions';

const OptionsSelector = forwardRef((props, ref) => (
<BaseOptionsSelector
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
ref={ref}
/>
));
const OptionsSelector = forwardRef((props, ref) => {
const {isSmallScreenWidth} = useWindowDimensions();
return (
<BaseOptionsSelector
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
ref={ref}
shouldDelayFocus={!isSmallScreenWidth}
/>
);
});

OptionsSelector.displayName = 'OptionsSelector';

Expand Down
136 changes: 136 additions & 0 deletions src/components/PopoverMenu/BasePopoverMenu.js
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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** The horizontal and vertical anchors points for the popover */
/** Defines the anchor points for the popover */

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. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Where the popover should be positioned relative to the anchor points. */
/** Sets the popover's position relative to the anchor points */

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shouldNavigateBeforeClosingModal: PropTypes.bool,
/** Indicates whether navigation should occur before closing the modal */
shouldNavigateBeforeClosingModal: PropTypes.bool,


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));
Loading
Loading