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

perf: optimise transition on Desktop when click on start chat #33055

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
220 changes: 130 additions & 90 deletions src/components/Button/index.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import {useIsFocused} from '@react-navigation/native';
import {useFocusEffect} from '@react-navigation/native';
import type {ForwardedRef} from 'react';
import React, {useCallback} from 'react';
import React, {memo, useCallback, useMemo, useRef} from 'react';
import type {GestureResponderEvent, StyleProp, TextStyle, ViewStyle} from 'react-native';
import {ActivityIndicator, View} from 'react-native';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback';
import Text from '@components/Text';
import withNavigationFallback from '@components/withNavigationFallback';
import useActiveElement from '@hooks/useActiveElement';
import useActiveElementRole from '@hooks/useActiveElementRole';
import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
Expand Down Expand Up @@ -118,6 +118,57 @@ type ButtonProps = (ButtonWithText | ChildrenProps) & {
accessibilityLabel?: string;
};

type KeyboardShortcutComponentProps = Pick<ButtonProps, 'isDisabled' | 'isLoading' | 'onPress' | 'pressOnEnter' | 'allowBubble' | 'enterKeyEventListenerPriority'>;

const accessibilityRoles: string[] = Object.values(CONST.ACCESSIBILITY_ROLE);

const KeyboardShortcutComponent = memo(
({isDisabled = false, isLoading = false, onPress = () => {}, pressOnEnter, allowBubble, enterKeyEventListenerPriority}: KeyboardShortcutComponentProps) => {
const isFocused = useRef(false);
const activeElementRole = useActiveElementRole();

const shouldDisableEnterShortcut = useMemo(() => accessibilityRoles.includes(activeElementRole ?? '') && activeElementRole !== CONST.ACCESSIBILITY_ROLE.TEXT, [activeElementRole]);

useFocusEffect(
useCallback(() => {
isFocused.current = true;

return () => {
isFocused.current = false;
};
}, []),
);

const keyboardShortcutCallback = useCallback(
(event?: GestureResponderEvent | KeyboardEvent) => {
if (!validateSubmitShortcut(isDisabled, isLoading, event)) {
return;
}
onPress();
},
// eslint-disable-next-line react-hooks/exhaustive-deps
[isDisabled, isLoading],
);

const config = useMemo(
() => ({
isActive: pressOnEnter && !shouldDisableEnterShortcut && isFocused.current,
shouldBubble: allowBubble,
priority: enterKeyEventListenerPriority,
shouldPreventDefault: false,
}),
// eslint-disable-next-line react-hooks/exhaustive-deps
[shouldDisableEnterShortcut],
);

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, keyboardShortcutCallback, config);

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like regression came from this change.
Let's try to fix this before being deploy blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@situchan PR created here. Will be ready for review shortly 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, app deployed already and now this is deploy blocker.
I can do quick review if needed.
cc: @Julesssss

return null;
},
);

KeyboardShortcutComponent.displayName = 'KeyboardShortcutComponent';

function Button(
{
allowBubble = false,
Expand Down Expand Up @@ -164,27 +215,6 @@ function Button(
) {
const theme = useTheme();
const styles = useThemeStyles();
const isFocused = useIsFocused();
const activeElement = useActiveElement();
const accessibilityRoles: string[] = Object.values(CONST.ACCESSIBILITY_ROLE);
const shouldDisableEnterShortcut = accessibilityRoles.includes(activeElement?.role ?? '') && activeElement?.role !== CONST.ACCESSIBILITY_ROLE.TEXT;

const keyboardShortcutCallback = useCallback(
(event?: GestureResponderEvent | KeyboardEvent) => {
if (!validateSubmitShortcut(isDisabled, isLoading, event)) {
return;
}
onPress();
},
[isDisabled, isLoading, onPress],
);

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, keyboardShortcutCallback, {
isActive: pressOnEnter && !shouldDisableEnterShortcut && isFocused,
shouldBubble: allowBubble,
priority: enterKeyEventListenerPriority,
shouldPreventDefault: false,
});

const renderContent = () => {
if ('children' in rest) {
Expand Down Expand Up @@ -247,72 +277,82 @@ function Button(
};

return (
<PressableWithFeedback
ref={ref}
onPress={(event) => {
if (event?.type === 'click') {
const currentTarget = event?.currentTarget as HTMLElement;
currentTarget?.blur();
}

if (shouldEnableHapticFeedback) {
HapticFeedback.press();
}
return onPress(event);
}}
onLongPress={(event) => {
if (isLongPressDisabled) {
return;
}
if (shouldEnableHapticFeedback) {
HapticFeedback.longPress();
}
onLongPress(event);
}}
onPressIn={onPressIn}
onPressOut={onPressOut}
onMouseDown={onMouseDown}
disabled={isLoading || isDisabled}
wrapperStyle={[
isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
style,
]}
style={[
styles.button,
small ? styles.buttonSmall : undefined,
medium ? styles.buttonMedium : undefined,
large ? styles.buttonLarge : undefined,
success ? styles.buttonSuccess : undefined,
danger ? styles.buttonDanger : undefined,
isDisabled && (success || danger) ? styles.buttonOpacityDisabled : undefined,
isDisabled && !danger && !success ? styles.buttonDisabled : undefined,
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
'text' in rest && (rest?.icon || rest?.shouldShowRightIcon) ? styles.alignItemsStretch : undefined,
innerStyles,
]}
hoverStyle={[
shouldUseDefaultHover && !isDisabled ? styles.buttonDefaultHovered : undefined,
success && !isDisabled ? styles.buttonSuccessHovered : undefined,
danger && !isDisabled ? styles.buttonDangerHovered : undefined,
]}
id={id}
accessibilityLabel={accessibilityLabel}
role={CONST.ROLE.BUTTON}
hoverDimmingValue={1}
>
{renderContent()}
{isLoading && (
<ActivityIndicator
color={success || danger ? theme.textLight : theme.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</PressableWithFeedback>
<>
<KeyboardShortcutComponent
isDisabled={isDisabled}
isLoading={isLoading}
allowBubble={allowBubble}
onPress={onPress}
pressOnEnter={pressOnEnter}
enterKeyEventListenerPriority={enterKeyEventListenerPriority}
/>
<PressableWithFeedback
ref={ref}
onPress={(event) => {
if (event?.type === 'click') {
const currentTarget = event?.currentTarget as HTMLElement;
currentTarget?.blur();
}

if (shouldEnableHapticFeedback) {
HapticFeedback.press();
}
return onPress(event);
}}
onLongPress={(event) => {
if (isLongPressDisabled) {
return;
}
if (shouldEnableHapticFeedback) {
HapticFeedback.longPress();
}
onLongPress(event);
}}
onPressIn={onPressIn}
onPressOut={onPressOut}
onMouseDown={onMouseDown}
disabled={isLoading || isDisabled}
wrapperStyle={[
isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
style,
]}
style={[
styles.button,
small ? styles.buttonSmall : undefined,
medium ? styles.buttonMedium : undefined,
large ? styles.buttonLarge : undefined,
success ? styles.buttonSuccess : undefined,
danger ? styles.buttonDanger : undefined,
isDisabled && (success || danger) ? styles.buttonOpacityDisabled : undefined,
isDisabled && !danger && !success ? styles.buttonDisabled : undefined,
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
'text' in rest && (rest?.icon || rest?.shouldShowRightIcon) ? styles.alignItemsStretch : undefined,
innerStyles,
]}
hoverStyle={[
shouldUseDefaultHover && !isDisabled ? styles.buttonDefaultHovered : undefined,
success && !isDisabled ? styles.buttonSuccessHovered : undefined,
danger && !isDisabled ? styles.buttonDangerHovered : undefined,
]}
id={id}
accessibilityLabel={accessibilityLabel}
role={CONST.ROLE.BUTTON}
hoverDimmingValue={1}
>
{renderContent()}
{isLoading && (
<ActivityIndicator
color={success || danger ? theme.textLight : theme.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</PressableWithFeedback>
</>
);
}

Expand Down
16 changes: 9 additions & 7 deletions src/components/PopoverMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -127,20 +127,22 @@ function PopoverMenu({
{isActive: isVisible},
);

const onModalHide = () => {
setFocusedIndex(-1);
if (selectedItemIndex.current !== null) {
menuItems[selectedItemIndex.current].onSelected();
selectedItemIndex.current = null;
}
};

return (
<PopoverWithMeasuredContent
anchorPosition={anchorPosition}
anchorRef={anchorRef}
anchorAlignment={anchorAlignment}
onClose={onClose}
isVisible={isVisible}
onModalHide={() => {
setFocusedIndex(-1);
if (selectedItemIndex.current !== null) {
menuItems[selectedItemIndex.current].onSelected();
selectedItemIndex.current = null;
}
}}
onModalHide={onModalHide}
animationIn={animationIn}
animationOut={animationOut}
animationInTiming={animationInTiming}
Expand Down
6 changes: 3 additions & 3 deletions src/components/SelectionList/BaseSelectionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import SectionList from '@components/SectionList';
import Text from '@components/Text';
import TextInput from '@components/TextInput';
import withKeyboardState, {keyboardStatePropTypes} from '@components/withKeyboardState';
import useActiveElement from '@hooks/useActiveElement';
import useActiveElementRole from '@hooks/useActiveElementRole';
import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
import useLocalize from '@hooks/useLocalize';
import useStyleUtils from '@hooks/useStyleUtils';
Expand Down Expand Up @@ -73,7 +73,7 @@ function BaseSelectionList({
const focusTimeoutRef = useRef(null);
const shouldShowTextInput = Boolean(textInputLabel);
const shouldShowSelectAll = Boolean(onSelectAll);
const activeElement = useActiveElement();
const activeElementRole = useActiveElementRole();
const isFocused = useIsFocused();
const [maxToRenderPerBatch, setMaxToRenderPerBatch] = useState(shouldUseDynamicMaxToRenderPerBatch ? 0 : CONST.MAX_TO_RENDER_PER_BATCH.DEFAULT);
const [isInitialSectionListRender, setIsInitialSectionListRender] = useState(true);
Expand Down Expand Up @@ -155,7 +155,7 @@ function BaseSelectionList({
const [focusedIndex, setFocusedIndex] = useState(() => _.findIndex(flattenedSections.allOptions, (option) => option.keyForList === initiallyFocusedOptionKey));

// Disable `Enter` shortcut if the active element is a button or checkbox
const disableEnterShortcut = activeElement && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElement.role);
const disableEnterShortcut = activeElementRole && [CONST.ROLE.BUTTON, CONST.ROLE.CHECKBOX].includes(activeElementRole);

/**
* Scrolls to the desired item index in the section list
Expand Down
8 changes: 0 additions & 8 deletions src/hooks/useActiveElement/index.native.ts

This file was deleted.

3 changes: 0 additions & 3 deletions src/hooks/useActiveElement/types.ts

This file was deleted.

8 changes: 8 additions & 0 deletions src/hooks/useActiveElementRole/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type UseActiveElementRole from './types';

/**
* Native doesn't have the DOM, so we just return null.
*/
const useActiveElementRole: UseActiveElementRole = () => null;

export default useActiveElementRole;
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
import {useEffect, useState} from 'react';
import type UseActiveElement from './types';
import {useEffect, useRef} from 'react';
import type UseActiveElementRole from './types';

/**
* Listens for the focusin and focusout events and sets the DOM activeElement to the state.
* On native, we just return null.
*/
const useActiveElement: UseActiveElement = () => {
const [active, setActive] = useState(document.activeElement);
const useActiveElementRole: UseActiveElementRole = () => {
const activeRoleRef = useRef(document?.activeElement?.role);

const handleFocusIn = () => {
setActive(document.activeElement);
activeRoleRef.current = document?.activeElement?.role;
};

const handleFocusOut = () => {
setActive(null);
activeRoleRef.current = null;
};

useEffect(() => {
Expand All @@ -26,7 +26,7 @@ const useActiveElement: UseActiveElement = () => {
};
}, []);

return active;
return activeRoleRef.current;
};

export default useActiveElement;
export default useActiveElementRole;
3 changes: 3 additions & 0 deletions src/hooks/useActiveElementRole/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
type UseActiveElementRole = () => string | null | undefined;

export default UseActiveElementRole;
Loading
Loading