Closed lanitochka17 closed 5 months ago
Job added to Upwork: https://www.upwork.com/jobs/~0156cc7a92d86433bd
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External
)
Triggered auto assignment to @zanyrenney (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
We think that this bug might be related to #vip-vsp CC @quinthar
mWeb - Device back button with emoji picker opened leads to LHN instead of closing picker
We are call Navigation.goBack
when back button is pressed.
Before navigating check if the emoji picker is visible or not, if visible close it and do not navigate. We can get emojiPicker status from EmojiPickerAction
and call hideEmojiPicker
.
mWeb - Device back button with emoji picker opened leads to LHN instead of closing picker
When emoji modal open we don't update window history state after that user clicks on "The browser back button" or "Device Back Button" it will navigate back to the previous page (base on browser history)
We will handle update the history state for Modal Web and to make sure this change only affects some the modals we can add new props shouldHandleNavigationBack
for Modal
```diff
+function Modal({fullscreen = true, onModalHide = () => {}, type, onModalShow = () => {}, children, shouldHandleNavigationBack, ...rest}: BaseModalProps) {
const theme = useTheme();
const StyleUtils = useStyleUtils();
const [previousStatusBarColor, setPreviousStatusBarColor] = useState<string>();
const setStatusBarColor = (color = theme.appBG) => {
if (!fullscreen) {
return;
}
StatusBar.setBackgroundColor(color);
};
const hideModal = () => {
setStatusBarColor(previousStatusBarColor);
onModalHide();
+ if (window.history.state.shouldGoBack) {
+ window.history.back();
+ }
};
+ const handlePopStateRef = useRef(() => {
+ rest.onClose();
+ });
const showModal = () => {
const statusBarColor = StatusBar.getBackgroundColor() ?? theme.appBG;
....
+ if (shouldHandleNavigationBack) {
+ window.history.pushState({shouldGoBack: true}, '', null);
+ window.addEventListener('popstate', handlePopStateRef.current);
+ }
onModalShow?.();
};
+ useEffect(
+ () => () => {
+ window.removeEventListener('popstate', handlePopStateRef.current);
+ },
+ [],
+ );
return (
<BaseModal
...
At EmojiPicker pass props shouldHandleNavigationBack
to Popover with condition is Browser.isMobile()
https://github.com/Expensify/App/assets/11959869/c9be0775-13cf-4074-a384-14a25608d678
Well opening up an emoji picker is not a navigation action.
Naturally clicking the back button would navigate back to the previous screen.
This is working as intended.
The only thing I see is that the background has a disabled state. We could improve the user experience if desired to disable all clicks and only close the emoji picker when clicking outside of it. But this is not what the Issue is asking for.
This issue should be handled in this issue
Thank you everyone for your proposals, but we're closing this issue 🙇
Hey @hayata-suenaga I agree with @jeremy-croff. It's not related to the goBack function and opening the emoji picker is not a navigation action.
If you want to implement this anyway, then this ticket should be separated form the goBack issue.
Not sure how the presented proposal will work on native platforms and how it interferes with web history though.
Personally I like the improvement mentioned by @jeremy-croff
The only thing I see is that the background has a disabled state. We could improve the user experience if desired to disable all clicks and only close the emoji picker when clicking outside of it. But this is not what the Issue is asking for.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
re-posted about this issue in the #expensify-open-source channel
@sobitneupane please review the proposals when you have time 🙇
My proposal can work the same native behavior when pressing the back button on Android device (the bottom sheet will be closed)
@sobitneupane, @zanyrenney Huh... This is 4 days overdue. Who can take care of this?
hey @suneox do you have a draft branch? I would like to test a few things
hey @suneox do you have a draft branch? I would like to test a few things
@sobitneupane @adamgrzybowski You can try on this branch
@sobitneupane it seems like Adam has reviewed the proposals above. Can you formally respond as to who we should be selecting here? If you do not have capacity as C+, please let me know and I will reassign the issue.
@adamgrzybowski is OOO and I will try to help.
@suneox I have quickly checked it, and for now, I have found one bigger issue in this PR:
I'm leaving also some notes:
addEventListener
should probably be in the useEffect
as well.Are you also 100% sure, it won't break the @react-navigation
history as those changes are not synced with this library?
@suneox I have quickly checked it, and for now, I have found one bigger issue in this PR:
Go to the Profile (must have avatar uploaded).
Press the Edit button on the Avatar.
Press "View photo"
It doesn't work.
This issue is not related to "view photo" it isn't using Popover, and the tested branch cover for Popover to handle this issue
I'm leaving also some notes:
- What would happen in nested modals? I think it might break the back button behavior.
The test branch has a prop shouldHandleNavigationBack
on Modal, and the expectation for this issue is happen on Popover so I have updated only the fix for Popover and we don't have nested Popover
- addEventListener should probably be in the useEffect as well.
- We shouldn't add event listeners on non mobile devices.
It can be resolved in the PR
Are you also 100% sure, it won't break the
@react-navigation
history as those changes are not synced with this library?
We update the history state of window so it not affect to react state of @react-navigation
library
This issue is not related to "view photo" it isn't using Popover, and the tested branch cover for Popover to handle this issue
I'm not sure I understand.
The Popover is using Modal (which modifies the browser history), and this branch affects it. The problem described above doesn't occur on the main/staging.
The Popover is using Modal (which modifies the browser history), and this branch affects it. The problem described above doesn't occur on the main/staging.
@kosmydel Thank you for your testing, I have seen a problem due to we always window.addEventListener('popstate', handlePopStateRef.current);
when showModal
so we will move to condition check shouldHandleNavigationBack
if (shouldHandleNavigationBack) {
window.history.pushState({isModal: true}, '', null);
+ window.addEventListener('popstate', handlePopStateRef.current);
}
- window.addEventListener('popstate', handlePopStateRef.current);
to avoid regression, we will set shouldHandleNavigationBack
to PopoverWithMeasuredContent on EmojiPicker
Could you please double check on the latest branch?
what do you think @sobitneupane ?
@sobitneupane @zanyrenney this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@suneox Personally, I don't like this new change. I think we should find a way, to solve this problem application-wide (to support nested popovers).
For instance, your initial solution worked also in this case (mWeb Android):
After your change, we are only covering one case from many. We should find a general solution.
Overall, the high-level concept makes sense to me. But we should wait for @WojtekBoman (coming back on Wednesday) or @adamgrzybowski to get another confirmation if it doesn't break the navigation.
thanks so much for the detailed review @kosmydel
Agree with your landing on waiting for now and getting an additional check from @WojtekBoman and @adamgrzybowski
we're waiting on ^^
After your change, we are only covering one case from many. We should find a general solution.
First, I think we should fix by the scope of this ticket. A generic solution I also implemented at Modal but the shouldHandleNavigationBack
default is false
due to Modal using a lot of places and we don't have a context to make assumption.
@sobitneupane @zanyrenney this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@WojtekBoman and @adamgrzybowski can you take a look a the comment above and changing the scope and let us know what you think please?
In my opinion, if we want to quickly fix the error, we can change the scope of this issue. However, it's definitely a good idea to create a follow-up to fix the error globally in the app. From our perspective, the fix seems ok, but I think that Adam's opinion is relevant in this case.
thanks so much @WojtekBoman @adamgrzybowski curious for your thoughts too please!
@adamgrzybowski what do you think? what are our next steps here?
@adamgrzybowski is OOO. He should be back next week.
@sobitneupane @zanyrenney this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks!
Current assignee @sobitneupane is eligible for the Internal assigner, not assigning anyone new.
Thanks for the update @kosmydel
@adamgrzybowski when you're back from OOO, please let us know your thoughts here.
I have clean-up my proposal matching with a testing branch.
@adamgrzybowski could you review the proposal or @kosmydel ?
bump @adamgrzybowski @kosmydel
@suneox Should this also cover the desktop browser with narrow layout? I am testings this branch https://github.com/suneox/ExpensifyApp/tree/35756-close-emoji-when-navigation-back and can't make it work.
But maybe I am doing something wrong.
@suneox Should this also cover the desktop browser with narrow layout? I am testings this branch https://github.com/suneox/ExpensifyApp/tree/35756-close-emoji-when-navigation-back and can't make it work.
But maybe I am doing something wrong.
@adamgrzybowski currently a condition only applies for mobile browser shouldHandleNavigationBack={Browser.isMobile()}
if we want to support narrow layout we can update condition but we should consider due to this behavior on desktop browser will raise confuse
@suneox Isn't this an inconsistency between platforms? For mobile web the back action will hide the emoji picker but for the narrow desktop web (which looks similar) it will go back to the previous screen.
I am not a UX expert but we may want to consider if we want to introduce such a difference between platforms. Maybe implementing it in both cases would be better. @zanyrenney might know who to ask about that.
going to tag the design team for another look at this one!
Triggered auto assignment to @shawnborton (Design
), see these Stack Overflow questions for more details.
Hey @shawnborton - I'm heeding the advice you mentioned in Slack. please could you take a look at this one as we aren't sure about the inconsistencies in the UX? Thank you!
At first glance, this does seem like a bug and it does seem like the proposal here would solve it.
cc @Julesssss for a second opinion since this is an Android related issue. The tldr; is that if you are viewing the chats list, then you open up a chat, and then open the emoji picker, the device back button doesn't close the emoji picker and send you back to the chat but rather it takes you back to the chats list.
cc @Expensify/design for vis too but I think they will likely agree with me :)
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.36-0 Reproducible in staging?: Y Reproducible in production?:Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4279504 Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:
Action Performed:
Expected Result:
Emoji picker should be closed, user should remain in chat history
Actual Result:
User is redirected back to LHN
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/31c26028-7122-47f2-abe4-66363db68835
View all open jobs on GitHub
Upwork Automation - Do Not Edit