Fix emoji picker hide itself when there is an archived chat in the navigation stack #24479
Conversation
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@aimane-chnaif If you experience an issue where the task is reopened by itself after canceling it, try to do it offline. This issue is reported here |
|
@aimane-chnaif after looking at the proposal from @hoangzinh here, I think it made much more sense to hide the emoji picker when the main composer unmount. Pros
We don't need to care whatever the condition is added when we hide the emoji picker when the composer hides. Currently, we only hide the picker when I tested both #21376 and our PR and it works fine. Let me know if we should proceed with this decision. |
|
Please check this bug report: And also, while showing emoji picker in edit composer in public room, admin updates write permission to admins only, then what happens? Does new solution fix all these concerns? |
Can you confirm that whenever switch chat (i.e. from browser back button), main composer is always unmounted? |
|
Checking.. |
|
I may have found another edge case: Let's say user opens report A on mobile. And then open report B on top of A. |
|
This is basically the same issue as our issue, but with a different way to reproduce it. So, both current and new solutions will fix it.
The emoji picker stays open, both current and new solution Screen.Recording.2023-08-15.at.14.12.07.mov
Web/mWeb: yes, both back and forward
No, we use id to identify which emoji picker is active. When we open the picker in B, the id is the report id of B. So, when A composer becomes hidden, it won't hide the emoji picker in B because the ID is different. |
|
ok so there's no issue which current solution fixes but new solution doesn't fix, right? |
correct. So, the current solution is to only hide the emoji picker when the main composer should hide and if the active emoji picker instance is initiated from the main composer. -if (ReportUtils.shouldHideComposer(this.props.report)) {
+if (ReportUtils.shouldHideComposer(this.props.report) && EmojiPickerAction.isActive(this.props.report.reportID)) {
EmojiPickerAction.hideEmojiPicker(true);
}This works fine for now. However, we are thinking to improve this by hiding the emoji picker when the main composer hides (unmount) instead. componentWillUnmount() {
ReportActionComposeFocusManager.clear();
KeyDownListener.removeKeyDownPressListner(this.focusComposerOnKeyPress);
this.unsubscribeNavigationBlur();
this.unsubscribeNavigationFocus();
+ if (EmojiPickerAction.isActive(this.props.report.reportID)) {
+ EmojiPickerAction.hideEmojiPicker(true);
+ }
}The benefit of this new solution is explained here #24479 (comment) The new solution will also fix #24559 (btw, do we need to compensate @hoangzinh too?). @AndrewGable Let me know your thought about the new solution |
|
@bernhardoj please fix conflict |
|
Conflicts solved. @AndrewGable gentle bump on above |
|
Fixed another conflict. I think we should proceed with the new solution and @AndrewGable will review it? @aimane-chnaif |
|
New solution sounds good 👍 |
|
@aimane-chnaif updated with the new solution |
|
looks like something wrong with the ci |
Please pull main again. Just heard this announcement:
|
|
Merged with main |
| @@ -42,9 +42,9 @@ const EmojiPicker = forwardRef((props, ref) => { | |||
| * @param {Element} emojiPopoverAnchorValue - Element to which Popover is anchored | |||
| * @param {Object} [anchorOrigin=DEFAULT_ANCHOR_ORIGIN] - Anchor origin for Popover | |||
| * @param {Function} [onWillShow=() => {}] - Run a callback when Popover will show | |||
| * @param {Object} reportActionValue - ReportAction for EmojiPicker | |||
| * @param {Object} id - Unique id for EmojiPicker | |||
There was a problem hiding this comment.
| * @param {Object} id - Unique id for EmojiPicker | |
| * @param {String} id - Unique id for EmojiPicker |
| reportActionID: PropTypes.string, | ||
| }), | ||
| /** Unique id for emoji picker */ | ||
| emojiPickerID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), |
There was a problem hiding this comment.
Let's consolidate these into string. No reason for 2 inconsistent types
| * | ||
| * @param {Function} [onModalHide=() => {}] - Run a callback when Modal hides. | ||
| * @param {Function} [onEmojiSelected=() => {}] - Run a callback when Emoji selected. | ||
| * @param {Element} emojiPopoverAnchor - Element on which EmojiPicker is anchored | ||
| * @param {Object} [anchorOrigin] - Anchor origin for Popover | ||
| * @param {Function} [onWillShow=() => {}] - Run a callback when Popover will show | ||
| * @param {Object} reportAction - ReportAction for EmojiPicker | ||
| * @param {Object} id - Unique id for EmojiPicker |
There was a problem hiding this comment.
| * @param {Object} id - Unique id for EmojiPicker | |
| * @param {String} id - Unique id for EmojiPicker |
| * | ||
| * @param {Number|String} actionID | ||
| * @param {Number|String} id |
There was a problem hiding this comment.
| * @param {Number|String} id | |
| * @param {String} id |
| * @return {Boolean} | ||
| */ | ||
| const isActiveReportAction = (actionID) => Boolean(actionID) && reportAction.reportActionID === actionID; | ||
| const isActive = (id) => Boolean(id) && id === activeID; |
There was a problem hiding this comment.
If type of id and activeID is different (one is number, another is string), === will not work.
Let's make them consistent as string type
| * | ||
| * @param {Number|String} actionID | ||
| * @param {Number|String} id |
|
Ah, I thought report action id is a number. Updated! |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.58-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.58-5 🚀
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
Details
We currently have a code to hide the emoji picker if the main composer becomes hidden. We do this on every component update. However, we didn't think of a case where the previous screen in the stack is still receiving component updates. I
Fixed Issues
$ #23757
PROPOSAL: #23757 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-08-12.at.12.12.48.mov
Mobile Web - Chrome
Chrome.mp4
Mobile Web - Safari
Screen.Recording.2023-08-12.at.12.39.07.mov
Desktop
Screen.Recording.2023-08-12.at.12.31.56.mov
iOS
Screen.Recording.2023-08-12.at.13.00.15.mov
Android
New.Expensify.Dev.mp4