-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat: blur the composer focus when open context menu #47621
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
Changes from all commits
c66dd94
9dd2342
80bd82e
dec716c
5c0ebae
bd065ee
edb8fb2
6b686ba
2cf341b
56499bc
de12e1a
3ab551c
b19ef5c
21d0f80
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 | ||
|---|---|---|---|---|
|
|
@@ -167,6 +167,22 @@ function ReportActionItemMessageEdit( | |||
| }; | ||||
| }, []); | ||||
|
|
||||
| useEffect( | ||||
| // Remove focus callback on unmount to avoid stale callbacks | ||||
| () => { | ||||
| if (textInputRef.current) { | ||||
| ReportActionComposeFocusManager.editComposerRef.current = textInputRef.current; | ||||
| } | ||||
| return () => { | ||||
| if (ReportActionComposeFocusManager.editComposerRef.current !== textInputRef.current) { | ||||
| return; | ||||
| } | ||||
| ReportActionComposeFocusManager.clear(true); | ||||
| }; | ||||
| }, | ||||
| [], | ||||
| ); | ||||
|
|
||||
| // We consider the report action active if it's focused, its emoji picker is open or its context menu is open | ||||
| const isActive = useCallback( | ||||
| () => isFocusedRef.current || EmojiPickerAction.isActive(action.reportActionID) || ReportActionContextMenu.isActiveReportAction(action.reportActionID), | ||||
|
|
@@ -188,14 +204,6 @@ function ReportActionItemMessageEdit( | |||
| }, true); | ||||
| }, [focus]); | ||||
|
|
||||
| useEffect( | ||||
| // Remove focus callback on unmount to avoid stale callbacks | ||||
| () => () => { | ||||
| ReportActionComposeFocusManager.clear(true); | ||||
| }, | ||||
| [], | ||||
| ); | ||||
|
|
||||
| useEffect( | ||||
| () => { | ||||
| if (isInitialMount.current) { | ||||
|
|
@@ -516,6 +524,9 @@ function ReportActionItemMessageEdit( | |||
| style={[styles.textInputCompose, styles.flex1, styles.bgTransparent]} | ||||
| onFocus={() => { | ||||
| setIsFocused(true); | ||||
| if (textInputRef.current) { | ||||
| ReportActionComposeFocusManager.editComposerRef.current = textInputRef.current; | ||||
|
Contributor
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. Let us reset the
Contributor
Author
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 think we shouldn't reset
Contributor
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. Maybe I am missing something here but why would we need
Contributor
Author
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. Ah, when ReportActionComposeFocusManager.clear we can reset
Contributor
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.
Yes, that's what I meant
Contributor
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 check if we also have to set the 47621-mweb-safari-blurissue.mp4
Contributor
Author
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. Let me check this and get back to you.
Contributor
Author
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. @rojiphil after testing, there's a small change needed to fix this issue. As ReportActionComposeFocusManager.clear(); is triggered in both main and edit composer, when users open new edit composer, editComposerRef is set, then the logic clear App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx Line 645 in 14b99ca
-> It’s wrong since the edit composer is still open I think we should set/reset editComposerRef in ReportActionItemMessageEdit. I have pushed the latest commit with the change. |
||||
| } | ||||
| InteractionManager.runAfterInteractions(() => { | ||||
| requestAnimationFrame(() => { | ||||
| reportScrollManager.scrollToIndex(index, true); | ||||
|
|
||||
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.
ReportActionComposeFocusManager.clear();here as we do not want to reset theeditcomposerif focus already exists on another edit item. Here is a test video demonstrating the problem:47621-clear-issue-1.mp4
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.
Here's the result after refactoring
Screen.Recording.2024-10-01.at.11.21.52.mov