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

Jump to bottom

Consistent key press focus logic between composer and emoji picker #31899

Merged
merged 9 commits into from
Dec 5, 2023
Merged

Consistent key press focus logic between composer and emoji picker #31899

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3bea1b5
consistent key press focus logic between composer and emoji picker
dukenv0307 Nov 27, 2023
bdbdc8c
Merge branch 'main' into fix/30867
dukenv0307 Nov 29, 2023
1efd85d
fix ts check
dukenv0307 Nov 29, 2023
bddf958
fix lint
dukenv0307 Nov 29, 2023
cb738e9
not focus with space key
dukenv0307 Nov 29, 2023
d0f3f30
fix ts
dukenv0307 Nov 29, 2023
ee9c915
merge main
dukenv0307 Nov 30, 2023
363ccfa
merge main
dukenv0307 Dec 5, 2023
3d4bd06
remove unnecessary condition
dukenv0307 Dec 5, 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
3 changes: 2 additions & 1 deletion src/components/EmojiPicker/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import compose from '@libs/compose';
import * as EmojiUtils from '@libs/EmojiUtils';
import getOperatingSystem from '@libs/getOperatingSystem';
import isEnterWhileComposition from '@libs/KeyboardShortcut/isEnterWhileComposition';
import * as ReportUtils from '@libs/ReportUtils';
import styles from '@styles/styles';
import * as StyleUtils from '@styles/StyleUtils';
import * as User from '@userActions/User';
Expand Down Expand Up @@ -316,7 +317,7 @@ function EmojiPickerMenu(props) {
// Enable keyboard movement if tab or enter is pressed or if shift is pressed while the input
// is not focused, so that the navigation and tab cycling can be done using the keyboard without
// interfering with the input behaviour.
if (keyBoardEvent.key === 'Tab' || keyBoardEvent.key === 'Enter' || (keyBoardEvent.key === 'Shift' && searchInputRef.current && !searchInputRef.current.isFocused())) {
if (!ReportUtils.shouldAutoFocusOnKeyPress(keyBoardEvent, searchInputRef)) {
setIsUsingKeyboardMovement(true);
return;
}
Expand Down
11 changes: 11 additions & 0 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import lodashEscape from 'lodash/escape';
import lodashFindLastIndex from 'lodash/findLastIndex';
import lodashIntersection from 'lodash/intersection';
import lodashIsEqual from 'lodash/isEqual';
import React from 'react';
import {TextInput} from 'react-native';
import Onyx, {OnyxCollection, OnyxEntry, OnyxUpdate} from 'react-native-onyx';
import {SvgProps} from 'react-native-svg';
import {ValueOf} from 'type-fest';
Expand Down Expand Up @@ -4216,6 +4218,14 @@ function shouldDisableWelcomeMessage(report: OnyxEntry<Report>, policy: OnyxEntr
return isMoneyRequestReport(report) || isArchivedRoom(report) || !isChatRoom(report) || isChatThread(report) || !PolicyUtils.isPolicyAdmin(policy);
}

function shouldAutoFocusOnKeyPress(event: KeyboardEvent, inputRef: React.RefObject<TextInput>): boolean {
if (event.key === 'Tab' || event.key === 'Enter' || (event.key === 'Shift' && inputRef.current && !inputRef.current.isFocused())) {
return false;
}

return true;
}

export {
getReportParticipantsTitle,
isReportMessageAttachment,
Expand Down Expand Up @@ -4381,6 +4391,7 @@ export {
getRoom,
shouldDisableWelcomeMessage,
canEditWriteCapability,
shouldAutoFocusOnKeyPress,
};

export type {OptionData};
17 changes: 4 additions & 13 deletions src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,18 +436,7 @@ function ComposerWithSuggestions({
return;
}

// If the key pressed is non-character keys like Enter, Shift, ... do not focus
if (e.key.length > 1) {
return;
}

// If a key is pressed in combination with Meta, Control or Alt do not focus
if (e.metaKey || e.ctrlKey || e.altKey) {
return;
}

// If the space key is pressed, do not focus
if (e.code === 'Space') {
dukenv0307 marked this conversation as resolved.
Show resolved Hide resolved
if (!ReportUtils.shouldAutoFocusOnKeyPress(e, textInputRef)) {
return;
}

Expand All @@ -457,7 +446,9 @@ function ComposerWithSuggestions({
}

focus();
replaceSelectionWithText(e.key, false);
if (e.key.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this condition could potencially change the behaviour - regardless of e.key.length value we will focus the input. Before we would return early from the the function without trigerring focus()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertKozik Because now we allow focus for option, cmd, ... which isn't letter key so we should add this condition to prevent replace text when we focus on text input with this key.

Copy link
Contributor

@robertKozik robertKozik Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Wasn't the main part of the issue to not focus on modifier keys? Copying from the expected behaviour: The Emoji search field is not focused when clicking on Modifier keys

So we would need this condition to be shared between composer and emoji picker as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertKozik You're right. Updated with the correct expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

D we still need this condition @dukenv0307 ? Beforehand in shouldAutoFocusOnKeyPress we are checking whether e.key.length >1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we don't need this any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertKozik Updated.

replaceSelectionWithText(e.key, false);
}
},
[checkComposerVisibility, focus, replaceSelectionWithText],
);
Expand Down
Loading