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

Fix the emoji picker does not close when editing message is deleted #22984

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Fix the emoji picker does not close when editing message is deleted #22984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e54486
Fix the emoji picker does not close when editing message is deleted
hoangzinh Jul 17, 2023
376aa82
Dont mark as required prop
hoangzinh Jul 17, 2023
e0f14cd
Show chat compose when editing a message and open emoji picker
hoangzinh Jul 17, 2023
08f8abf
Update comment
hoangzinh Jul 17, 2023
2906498
Update comment
hoangzinh Jul 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
10 changes: 9 additions & 1 deletion src/components/EmojiPicker/EmojiPickerButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@ const propTypes = {
/** Id to use for the emoji picker button */
nativeID: PropTypes.string,

/**
* ReportAction for EmojiPicker.
*/
reportAction: PropTypes.shape({
reportActionID: PropTypes.string,
}),

...withLocalizePropTypes,
};

const defaultProps = {
isDisabled: false,
nativeID: '',
reportAction: {},
};

function EmojiPickerButton(props) {
Expand All @@ -33,7 +41,7 @@ function EmojiPickerButton(props) {
ref={(el) => (emojiPopoverAnchor = el)}
style={({hovered, pressed}) => [styles.chatItemEmojiButton, StyleUtils.getButtonBackgroundColorStyle(getButtonState(hovered, pressed))]}
disabled={props.isDisabled}
onPress={() => EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor)}
onPress={() => EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor, undefined, () => {}, props.reportAction)}
nativeID={props.nativeID}
accessibilityLabel={props.translate('reportActionCompose.emoji')}
>
Expand Down
9 changes: 6 additions & 3 deletions src/pages/home/report/ReportActionItemMessageEdit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import useLocalize from '../../../hooks/useLocalize';
import useKeyboardState from '../../../hooks/useKeyboardState';
import useWindowDimensions from '../../../hooks/useWindowDimensions';
import useReportScrollManager from '../../../hooks/useReportScrollManager';
import * as EmojiPickerAction from '../../../libs/actions/EmojiPickerAction';

const propTypes = {
/** All the data of the action */
Expand Down Expand Up @@ -118,16 +119,17 @@ function ReportActionItemMessageEdit(props) {
});

return () => {
// Skip if this is not the focused message so the other edit composer stays focused
if (!isFocusedRef.current) {
// Skip if this is not the focused message so the other edit composer stays focused.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sort of confused by this comment. Do we mean something like this?

Suggested change
// Skip if this is not the focused message so the other edit composer stays focused.
// Skip showing the main composer if the context menu is active for this reportAction. We want the "Edit composer" text input stay focused in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nope @marcaaron . I'm not really full understand the first comment as well. But I guess what they mean is if we have multiple Editing messages, and if the current edit message is not focus when unmount => we can skip show main composer to let another editing message still be focused.

Back to our case, I would like to say if the current edit message is not focus by opening its EmojiPicker, we should treat as it's still focus and show main composer (because we are still editing this message)

// In small screen devices, when EmojiPicker is shown, the current edit message will lose focus, we need to check this case as well.
if (!isFocusedRef.current && !EmojiPickerAction.isActiveReportAction(props.action.reportActionID)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be || instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we go with || it should be !(isFocusedRef.current || EmojiPickerAction.isActiveReportAction(..)) then. So my intention here is would like to cover a case in small screen device, when we edit a message, then open Emoji, the edit composer will be lost focus => With current implement, when we hide the EmojiPicker, the main composer won't be show in small screen devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm not getting the logic. Can you please explain further more clearly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. With our change according to my proposal, we reveal another bug that I think we should fix it in this PR.

Step to reproduce: on small screen devices:

  1. Login with userA’s account
  2. Create a public room with userA workspace
  3. Copy public room URL
  4. Open other browser and login with userB’s account
  5. Join the room by pasting room URL

  6. Send a message in room from userB
  7. Click on “Edit comment” message after sent
  8. Open emoji picker
  9. Switch back to UserA's account and delete the message sent by UserB
    We can observe that EmojiPicker is hided but the main composer is hide as well.
Screen.Recording.2023-07-18.at.20.42.56.mov

So previously in the componentWillUnmount logic of ReportActionItemMessageEdit:

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 120 to 129 in 38bc5b3

return () => {
// Skip if this is not the focused message so the other edit composer stays focused
if (!isFocusedRef.current) {
return;
}
// Show the main composer when the focused message is deleted from another client
// to prevent the main composer stays hidden until we swtich to another chat.
ComposerActions.setShouldShowComposeInput(true);
};

We will check

  • If we're focusing on a Edit message => When it's unmounted due to deleted
  • If we're out focus on a Edit message => We're already show the main composer => Not need to show main composer anymore.

But with our change, when an Edit message is out focus, it might be there is a EmojiPicker is opening. So when we unmount an Edit message, we should check if there is an EmojiPicker related to the current Edit message is opening, we should show the main composer as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. This makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

This caused a regression (case 3). If we focus on message A, open it's emoji picker and close it, then focus on message B and delete message A then the main composer will show up. This is due to the faulty EmojiPickerAction.isActiveReportAction check as it returns true for message A even though the emoji picker is no longer open.

What we had to do is to clear the active report action accordingly i.e. if we are deleting message A and it's emoji picker is not open then EmojiPickerAction.isActiveReportAction should return false.

Copy link
Contributor

Choose a reason for hiding this comment

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

This caused a bug where this condition returns false when composer is not focused but save button is pressed, since onFocus function is synchronous and takes some times to get focus. Thus causing edit input to be still open and main composer not to show: #32218

return;
}

// Show the main composer when the focused message is deleted from another client
// to prevent the main composer stays hidden until we swtich to another chat.
ComposerActions.setShouldShowComposeInput(true);
};
}, []);
}, [props.action.reportActionID]);

/**
* Save the draft of the comment. This debounced so that we're not ceaselessly saving your edit. Saving the draft
Expand Down Expand Up @@ -346,6 +348,7 @@ function ReportActionItemMessageEdit(props) {
onModalHide={() => InteractionManager.runAfterInteractions(() => textInputRef.current.focus())}
onEmojiSelected={addEmojiToTextBox}
nativeID={emojiButtonID}
reportAction={props.action}
/>
</View>

Expand Down
Loading