[HOLD #11768] [$8000] mWeb - Swipe to go back on safari mobile app flickers - reported by @adeel0202

mvtglobally commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Navigate to the app
Go to any chat, open settings or any other app screens
Swipe back

Expected Result:

Swipe back should not flicker or display any other pages in between

Actual Result:

Swipe to go back on safari mobile app flickers, screens displayed inconsistently

Workaround:

unknown

Platform:

Where is this issue occurring?

Mobile Web

Version Number: 1.1.38-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/153819765-f9d0e90d-75a4-40bd-9b04-1382296814f5.MP4

Expensify/Expensify Issue URL: Issue reported by: @adeel0202 Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643823162505289

View all open jobs on GitHub

MelvinBot commented 2 years ago

Triggered auto assignment to @lschurr (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

MelvinBot commented 2 years ago

Triggered auto assignment to @timszot (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

MelvinBot commented 2 years ago

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

MitchExpensify commented 2 years ago

Upwork Job

MelvinBot commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

MelvinBot commented 2 years ago

Triggered auto assignment to @Julesssss (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Julesssss commented 2 years ago

Awaiting proposals

Julesssss commented 2 years ago

Still awaiting proposals, should we double the bountly?

Julesssss commented 2 years ago

Doubled bounty. Still awaiting proposals

adeel0202 commented 2 years ago

@Julesssss, I think the price is still $250 on Upwork 👀

Julesssss commented 2 years ago

@MitchExpensify would you mind updating the job price? Ta

MitchExpensify commented 2 years ago

Done! Thanks @Julesssss

MitchExpensify commented 2 years ago

Doubled the price in Upwork and in title, and posted in #expensify-open-source

Julesssss commented 2 years ago

Still awaiting proposals. We should probably double the price again soon.

MitchExpensify commented 2 years ago

Will double tomorrow given that'll be a week with no proposals

MitchExpensify commented 2 years ago

Doubled

Julesssss commented 2 years ago

Not overdue. Though we should probably double again @MitchExpensify

MitchExpensify commented 2 years ago

Not overdue

MitchExpensify commented 2 years ago

Doubled! https://expensify.slack.com/archives/C01GTK53T8Q/p1651106508246309

Julesssss commented 2 years ago

Not overdue, we're awaiting proposals still

MitchExpensify commented 2 years ago

Asked about doubling here https://expensify.slack.com/archives/C02NK2DQWUX/p1652285549493959

marcaaron commented 2 years ago

Seems related to react-navigation potentially. Almost like there are two sets of screens getting pushed and dismissed somehow...

Julesssss commented 2 years ago

Not overdue

marcaaron commented 2 years ago

looked into this for a second and here's what I have as a theory but would need to look into it more...

When we swipe to go back Safari will show us a preview of the previous page. This works for static sites but not so much with single page apps. It is also just triggering a simple 'popstate' event.
The popstate event should update the state of the navigation (there are some known issues with this IIRC).
The old state is compared to the new state
Probably in this case, the state comparison tells us that we need to close the drawer.
That actually happens fine - but what we are seeing is the weird "previous view from memory" getting reshown on the swipe.

So yeah, I'm not really sure what the fix would be here. popstate event is working correctly (can test by using the back button instead of swipe) - it's just that swiping back in this way looks bad because we are:

Showing a cached view of the previous route (internal Safari feature) on swipe
When the popstate event is triggered the drawer closes and runs an animation for that (which looks fine if you use the back button to navigate)
It looks jank because we quickly go from LHN to Chat and animate back to LHN.

The best solution I can think of is to disable the drawer animation when we are handling a 'popstate' event (which maybe can be done with drawerStatusBarAnimation: 'none'). But this seems a bit hacky since we'll need to listen for the 'popstate' event in the drawer (also we don't really need to do this when the back button is pressed - also no clear way to determine the difference between a swipe or back button press).

parasharrajat commented 2 years ago

May be Drawer is exposing the hammer gesture, otherwise, If this globally available on window then we can listen for swipe and act. Need to check this.

marcaaron commented 2 years ago

(which maybe can be done with drawerStatusBarAnimation: 'none')

Actually, this seems wrong the "status bar" is not what we want to not animate. If we want to disable the drawer animation I think at a minimum we would need a custom drawer animation since there's no default way to disable this.

May be Drawer is exposing the hammer gesture, otherwise, If this globally available on window then we can listen for swipe and act.

Pretty sure it is Safari. We could try to track touch events to see if the 'popstate' event was a swipe (maybe) but seems like a hack. Hopefully there is some way to see. If so, seems reasonable to propose a fix in react-navigation.

parasharrajat commented 2 years ago

I can try to create a Reproduction to see if this is really happening on react-navigation. Then it would be good to fix there.

marcaaron commented 2 years ago

Probably just take this example.

then make it a slide type and see what happens when we swipe back

marcaaron commented 2 years ago

https://snack.expo.dev/c9jpEcNnH

marcaaron commented 2 years ago

Might need to deploy it via netlify or something. The snack doesn't have back button controls.

marcaaron commented 2 years ago

Hey everyone, I'm going to put a HOLD on this issue as I'm not sure what we even want the expected behavior to be here and need to review our wish-list of navigation features in general before accepting any more proposals.

MitchExpensify commented 2 years ago

On hold, not overdue

Julesssss commented 2 years ago

On hold, not overdue

No change here.

MitchExpensify commented 2 years ago

@marcaaron how's the hold on this one.. holding up?

Does this still hold true?

Hey everyone, I'm going to put a HOLD on this issue as I'm not sure what we even want the expected behavior to be here and need to review our wish-list of navigation features in general before accepting any more proposals.

melvin-bot[bot] commented 2 years ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

marcaaron commented 2 years ago

Yes!

MitchExpensify commented 1 year ago

On hold still