-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: Composer not focused on click while editor's emoji modal is open #28238
Changes from 16 commits
0f02054
86d30a4
27f6a05
24d1fa9
d262d65
f9a54af
83f7745
801066c
0472812
e7c2fb5
b2dede8
c6b17d5
3f4e9d7
fb99586
6bcd089
b1025c1
3439d49
f100c71
f51ed18
fdfec45
97338fc
759b4a5
a20eace
bb9f370
66ce543
0ae8452
f1b4e20
fe16fc0
f6b25c8
21d8617
df83fc8
599fe46
031ab6c
dc73745
b41e23c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -384,16 +384,19 @@ function ComposerWithSuggestions({ | |
focusWithDelay(textInputRef.current)(shouldDelay); | ||
}, []); | ||
|
||
const setUpComposeFocusManager = useCallback(() => { | ||
// This callback is used in the contextMenuActions to manage giving focus back to the compose input. | ||
ReportActionComposeFocusManager.onComposerFocus(() => { | ||
if (!willBlurTextInputOnTapOutside || !isFocused) { | ||
return; | ||
} | ||
const setUpComposeFocusManager = useCallback( | ||
(isMainComposer = true) => { | ||
// This callback is used in the contextMenuActions to manage giving focus back to the compose input. | ||
ReportActionComposeFocusManager.onComposerFocus(() => { | ||
if (!willBlurTextInputOnTapOutside || !isFocused) { | ||
return; | ||
} | ||
|
||
focus(false); | ||
}, true); | ||
}, [focus, isFocused]); | ||
focus(false); | ||
}, isMainComposer); | ||
}, | ||
[focus, isFocused], | ||
); | ||
|
||
/** | ||
* Check if the composer is visible. Returns true if the composer is not covered up by emoji picker or menu. False otherwise. | ||
|
@@ -517,7 +520,10 @@ function ComposerWithSuggestions({ | |
onKeyPress={triggerHotkeyActions} | ||
style={[styles.textInputCompose, isComposerFullSize ? styles.textInputFullCompose : styles.flex4]} | ||
maxLines={maxComposerLines} | ||
onFocus={onFocus} | ||
onFocus={() => { | ||
setUpComposeFocusManager(false); | ||
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. Can you please explain this? I didn't understand that comment. Why are we setting isMainComposer false even though it is the main composer? Please add a comment here. 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. This comment explains from the code perspective. Conceptually, the 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. What is general focus or main focus callback?
What does it mean? 😃 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. In
As a result, I know the 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. This is a mess. I think we should refactor this to make it clear in each step what we are doing. These functions naming and the pattern are confusing not the code. 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. we should find a way to set the order to the focus callbacks. Logic should say that now this callback should be called first if both |
||
onFocus(); | ||
}} | ||
onBlur={onBlur} | ||
onClick={setShouldBlockSuggestionCalcToFalse} | ||
onPasteFile={displayFileInModal} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -130,6 +130,28 @@ function ReportActionItemMessageEdit(props) { | |||||||||||||||||||||||||
[props.action.reportActionID], | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||
* Focus the composer text input | ||||||||||||||||||||||||||
* @param {Boolean} [shouldDelay=false] Impose delay before focusing the composer | ||||||||||||||||||||||||||
* @memberof ReportActionCompose | ||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||
const focus = useCallback((shouldDelay = false) => { | ||||||||||||||||||||||||||
focusWithDelay(textInputRef.current)(shouldDelay); | ||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||
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. Why did you recreate this if it was already present below? If you want to use
Suggested change
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. This was referenced from App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js Lines 383 to 385 in 30b3992
Anw, the former implementation without 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. Actually, focusWithDelay is a curry function that returns a callback that accepts
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. This would cause lint error: |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const setUpComposeFocusManager = useCallback(() => { | ||||||||||||||||||||||||||
ReportActionComposeFocusManager.onComposerFocus(() => { | ||||||||||||||||||||||||||
focus(true); | ||||||||||||||||||||||||||
}, false); | ||||||||||||||||||||||||||
}, [focus]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
useEffect( | ||||||||||||||||||||||||||
() => () => { | ||||||||||||||||||||||||||
ReportActionComposeFocusManager.clear(); | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
[], | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
tienifr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||
// For mobile Safari, updating the selection prop on an unfocused input will cause it to automatically gain focus | ||||||||||||||||||||||||||
// and subsequent programmatic focus shifts (e.g., modal focus trap) to show the blue frame (:focus-visible style), | ||||||||||||||||||||||||||
|
@@ -305,11 +327,6 @@ function ReportActionItemMessageEdit(props) { | |||||||||||||||||||||||||
[deleteDraft, isKeyboardShown, isSmallScreenWidth, publishDraft], | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||
* Focus the composer text input | ||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||
const focus = focusWithDelay(textInputRef.current); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||
<> | ||||||||||||||||||||||||||
<View style={[styles.chatItemMessage, styles.flexRow]}> | ||||||||||||||||||||||||||
|
@@ -365,6 +382,7 @@ function ReportActionItemMessageEdit(props) { | |||||||||||||||||||||||||
setIsFocused(true); | ||||||||||||||||||||||||||
reportScrollManager.scrollToIndex({animated: true, index: props.index}, true); | ||||||||||||||||||||||||||
setShouldShowComposeInputKeyboardAware(false); | ||||||||||||||||||||||||||
setUpComposeFocusManager(); | ||||||||||||||||||||||||||
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. This will cause recursion. 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. In other words, We will resubscribe to focusHandler(ReportActionComposeFocusManager) every time, it gets focused where focus might be triggered from the same focushandler(ReportActionComposeFocusManager). 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. The purpose of this is to let the last focused composer to re-gain focus:
This also explains why I did the same with 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 this could cause recursion because 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. From code perspective, previously the 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. Speaking of this, I realized that we should not subscribe the focus callback when composer mounts but when it's focused instead. Subscribe on mount is redundant. 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. Please add a comment here on why calling this function is necessary. |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Clear active report action when another action gets focused | ||||||||||||||||||||||||||
if (!EmojiPickerAction.isActive(props.action.reportActionID)) { | ||||||||||||||||||||||||||
|
@@ -390,12 +408,13 @@ function ReportActionItemMessageEdit(props) { | |||||||||||||||||||||||||
<EmojiPickerButton | ||||||||||||||||||||||||||
isDisabled={props.shouldDisableEmojiPicker} | ||||||||||||||||||||||||||
onModalHide={() => { | ||||||||||||||||||||||||||
setIsFocused(true); | ||||||||||||||||||||||||||
focus(true); | ||||||||||||||||||||||||||
ReportActionComposeFocusManager.focus(); | ||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||
onEmojiSelected={addEmojiToTextBox} | ||||||||||||||||||||||||||
nativeID={emojiButtonID} | ||||||||||||||||||||||||||
emojiPickerID={props.action.reportActionID} | ||||||||||||||||||||||||||
// Let the pressed composer re-gain focus on modal hide | ||||||||||||||||||||||||||
tienifr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
onPress={setUpComposeFocusManager} | ||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||
</View> | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
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.
Can you explain this?
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.
Just a note: Explained here #28238 (comment).