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.

Sign up for GitHub

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

mWeb: Keep the focus state of the composer when closing three dots menu in report header #28899

Merged
merged 7 commits into from
Oct 18, 2023
Merged

mWeb: Keep the focus state of the composer when closing three dots menu in report header #28899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c126b5f
Remove focus when closing three dots menu in report header
ginsuma Oct 5, 2023
71ab28c
Remove focus on native apps
ginsuma Oct 5, 2023
c5b5685
Merge branch 'main' into fix/26763-new-ui
ginsuma Oct 9, 2023
063ac7a
Keep focus state on mWeb
ginsuma Oct 9, 2023
5882ae2
Merge branch 'main' into fix/26763-new-ui
ginsuma Oct 11, 2023
7387238
Merge branch 'main' into fix/26763-new-ui
ginsuma Oct 18, 2023
1a7a5dc
Add comment
ginsuma Oct 18, 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
5 changes: 5 additions & 0 deletions src/components/PopoverMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ const propTypes = {
}),

withoutOverlay: PropTypes.bool,

/** Should we announce the Modal visibility changes? */
shouldSetModalVisibility: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -44,6 +47,7 @@ const defaultProps = {
},
anchorRef: () => {},
withoutOverlay: false,
shouldSetModalVisibility: true,
};

function PopoverMenu(props) {
Expand Down Expand Up @@ -89,6 +93,7 @@ function PopoverMenu(props) {
disableAnimation={props.disableAnimation}
fromSidebarMediumScreen={props.fromSidebarMediumScreen}
withoutOverlay={props.withoutOverlay}
shouldSetModalVisibility={props.shouldSetModalVisibility}
>
<View style={isSmallScreenWidth ? {} : styles.createMenuContainer}>
{!_.isEmpty(props.headerText) && <Text style={[styles.createMenuHeaderText, styles.ml3]}>{props.headerText}</Text>}
Expand Down
15 changes: 14 additions & 1 deletion src/components/ThreeDotsMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as Expensicons from '../Icon/Expensicons';
import ThreeDotsMenuItemPropTypes from './ThreeDotsMenuItemPropTypes';
import CONST from '../../CONST';
import PressableWithoutFeedback from '../Pressable/PressableWithoutFeedback';
import * as Browser from '../../libs/Browser';

const propTypes = {
/** Tooltip for the popup icon */
Expand Down Expand Up @@ -48,6 +49,9 @@ const propTypes = {

/** Whether the popover menu should overlay the current view */
shouldOverlay: PropTypes.bool,

/** Should we announce the Modal visibility changes? */
shouldSetModalVisibility: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -61,9 +65,10 @@ const defaultProps = {
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP, // we assume that popover menu opens below the button, anchor is at TOP
},
shouldOverlay: false,
shouldSetModalVisibility: true,
};

function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, menuItems, anchorPosition, anchorAlignment, shouldOverlay}) {
function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, menuItems, anchorPosition, anchorAlignment, shouldOverlay, shouldSetModalVisibility}) {
const [isPopupMenuVisible, setPopupMenuVisible] = useState(false);
const buttonRef = useRef(null);
const {translate} = useLocalize();
Expand Down Expand Up @@ -91,6 +96,13 @@ function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, me
onIconPress();
}
}}
onMouseDown={(e) => {
/* Keep the focus state on mWeb like we did on the native apps. */
if (!Browser.isMobile()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One last request, can you explain here why we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pecanoro, on Web and mWeb, the composer will blur if we click outside the text input. This is expected on Web, but not mWeb. On there, we want to keep the focus state like we did on native apps when closing the menu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, what I meant is, can you add a comment in the code before the if condition explaining that? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pecanoro I added it.

return;
}
e.preventDefault();
}}
ref={buttonRef}
style={[styles.touchableButtonImage, ...iconStyles]}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
Expand All @@ -111,6 +123,7 @@ function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, me
onItemSelected={hidePopoverMenu}
menuItems={menuItems}
withoutOverlay={!shouldOverlay}
shouldSetModalVisibility={shouldSetModalVisibility}
anchorRef={buttonRef}
/>
</>
Expand Down
1 change: 0 additions & 1 deletion src/components/VideoChatButtonAndMenu/BaseVideoChatButtonAndMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ function BaseVideoChatButtonAndMenu(props) {
left: videoChatIconPosition.x - 150,
top: videoChatIconPosition.y + 40,
}}
shouldSetModalVisibility={false}
withoutOverlay
anchorRef={videoChatButtonRef}
>
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ function HeaderView(props) {
<ThreeDotsMenu
anchorPosition={styles.threeDotsPopoverOffset(props.windowWidth)}
menuItems={threeDotMenuItems}
shouldSetModalVisibility={false}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On mWeb, we focus on the main composer by the modal state. This led to the inconsistency between mWeb and native apps.

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

Lines 477 to 481 in 25f7ac1

if (!(willBlurTextInputOnTapOutside && !isNextModalWillOpenRef.current && !modal.isVisible && isFocused && (prevIsModalVisible || !prevIsFocused))) {
return;
}
focus();

/>
)}
</View>
Expand Down