Switching between chats using LHN causes chats to get stuck offscreen

arielgreen commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding!

Expected Result:

When switching between chats in mobile web for E.Cash, I expect the chat to load and display correctly

Actual Result:

When switching between chats in mobile web for E.cash in Chrome on iOS, the second chat you switch to is not displayed correctly, and is off-screen

Action Performed:

Sign in to Expensify.Cash on Chrome or Safari on iOS
Tap on the chat switcher, and switch to a chat with a user (or use the search function)
Tap back to the chat list
Tap on the chat switcher again, and switch to a chat with a different user
The chat should render mostly off-screen, with most of the chat content not visible (if it doesn't, switch back to the other chat - it is inconsistent!)

Workaround:

You are forced to reload the page.

Platform:

Web: No iOS: No Android: No Desktop App: No Mobile Web: iOS - Yes | Android - No

Version Number: v1.0.2-73 Notes/Photos/Videos: Any additional supporting documentation

Here's a short recording of this happening:

ScreenFlow - 02 Here's how the chat renders:

https://www.upwork.com/jobs/~01a09bd14ffed37ba9 https://github.com/Expensify/Expensify/issues/157743

attaradev commented 3 years ago

Hello @tylerkaraszewski, I would love to look into this. Working on a proposal.

kaushiktd commented 3 years ago

Hi,

This is Kaushik here. I would like to put a technical explanation for this,

It may be an animation interpolate issue, first we will deploy to our local system and check with a real device with debugging.

kidroca commented 3 years ago

There's a good chance this might be solved by the loading transition proposed here: #2154, I can take a look, when I start working, and report back

marcaaron commented 3 years ago

I'm not sure how to solve this yet but, it seems like it would be related to the DrawerNavigator implementation in react-navigation

kidroca commented 3 years ago

I've found another way to recreate it:

From Home
Tap the FAB button
select New Chat

https://user-images.githubusercontent.com/12156624/113295396-d3f0f880-9300-11eb-8aaa-d5c258d1d67a.mp4

I think I found what's causing it:

https://github.com/Expensify/Expensify.cash/blob/68866790ce210cfb78c41e84cb079b6b192fa5b0/src/components/OptionsSelector.js#L89-L91

Removing this fixes the issue when you tap the search icon or when you open a new chat. I don't know why (yet 😀) but focusing causes navigation transitions to stop

In summary the issue seems related to focusing, focusing quickly on a field during animation causes this, but it's perhaps something that should be addressed in React Navigation

Using the InteractionManager like this does not help:

        InteractionManager.runAfterInteractions(() => {
            this.textInput.focus();
        });

...and (am I getting annoying with this 😄) removing input autofocus on mobile (mWeb as well, aka small screens) would "fix" this

You can use desktop safari for more debugging options

tylerkaraszewski commented 3 years ago

@marcaaron - what do you think about this analysis?

marcaaron commented 3 years ago

I'm not able to reproduce this. But if disabling the "auto focus" behavior will fix it then we are discussing this possibility here and if we do decide to remove the auto focus for inputs on mobile then this should be fixed by default. cc @trjExpensify since this is another potential issue related to auto focus.

parasharrajat commented 3 years ago

I tested it. Autofocus has nothing to do with it. It still exists. Easier reproduction for this is to try to change the resolution in a responsive mode in the browser.

https://user-images.githubusercontent.com/24370807/113441370-c83e2880-940b-11eb-944e-584bda264f3f.mp4

kidroca commented 3 years ago

Autofocus has nothing to do with it

That's a bit harsh ☹️, In my attempts removing the focus in OptionsSelector.js fixed the issue when I open a New Chat and when I open Search. It did not fix the issue for switching between chats as the focusing handled by a different component there

Changing the size dynamically might be another way to cause this problem. Some bugs can stem from a combination of factors

https://user-images.githubusercontent.com/12156624/113465298-9fce2280-943b-11eb-84d5-b1d1e0e3cd3f.mp4

kidroca commented 3 years ago

I've researched the handling of input fields in mobile Safari further.

When an input is focused mobile Safari would try to position it nicely in the middle for the user In our case this happens while the drawer is still moving

Here is a good explanation: https://gist.github.com/kiding/72721a0553fa93198ae2bb6eefaa3299

When an input element gets focused, iOS Safari tries to put it in the center by scrolling (and zooming.) Zooming can be easily disabled using a meta tag, but the scrolling hasn't been quite easy.

And here's the same problem reported on stackoverflow: https://stackoverflow.com/a/6918582/4834103

I've had this problem before where a fixed size webapp (built for iPad) gets partially scrolled out of view by the browser because it tries to center the text-field in the view.

Why does calling focus() break my CSS transition?

Changing the viewport dimensions might be causing the same issue, but I think it's a lot less likely to happen IRL

kidroca commented 3 years ago

There's a ticket related to viewport size change on react navigation: https://github.com/react-navigation/react-navigation/issues/8551

Thought specifically for the autofocus cases:

an action is selected
content with a field is mounted outside the viewport (abs position?)
the field is focused during the transition and the transition stops

I've also recreated this on mobile Chrome

marcaaron commented 3 years ago

It does seem like perhaps the fix would be in react-navigation based on that issue. Curious what some debugging might reveal.

Not sure if react-navigation is using a CSS transition in this case, I believe it's using react-native-reanimated for the drawer and setting the translateX property. Not sure if it helps, but it doesn't seem to happen consistently which makes me wonder if some slow running JS is causing the translateX value to be incorrect.

marcaaron commented 3 years ago

I am somewhat convinced that slow running JS is the culprit here and wonder if just delaying the render of the ReportScreen until after the drawer transition completes can help prevent this. Not sure the drawer has a transitionEnd event we could listen for, but that might be something to look into.

kidroca commented 3 years ago

Well it runs really well and smooth on a real device. On my video you can see that the drawer is not involved - I use the FAB to start a new chat - which transitions to a modal navigator. Same thing with Search

I can't recreate the issue when switching chats But I'm recreating it 100% from the FAB or pressing on Search

kidroca commented 3 years ago

I've played with the <Placeholder /> idea while the drawer is in the way. Don't render the actual content under it. But that brings some other problems - you can swipe and peek what's behind the drawer - it would be a blank screen if nothing is rendered. When you pick a chat it starts rendering instantly (well not yet 😄) and the drawer would actually help cover this. If you wait for the transition to end you will see a slide/fade transition then a white screen then chat items will start appear as they are rendered

Regarding slow js, I found a thing but I don't know where to post it:

withWindowDimensions is perhaps not implemented in the best way

25 unique components use it
one of them is PopoverWithMeasuredContent (indirectly through Modal) - this popover is rendered for each chat message
one of them is ReportActionContextMenu (indirectly through Tooltip) - this is rendered a couple of times for each chat message, thought they are covered by isVisible mount check, one used for measureContent is not covered
there are others - in the Keyboard Spacer, the modals inside the ReportActionCompose etc...

The point is there would be a lot of withWindowDimensions rendered, they probably don't fire more than once or twice, but all of them would assign listeners and use lifecycle methods which makes an impact on mobile especially during startup when they allocate their resources

You can still have a withWindowDimensions HOC, working the way it works, but it should just be a context consumer and provide data from context to the wrapped component.

A single element wraps the main layout - the way the SafeAreaProvider does Only that element subscribes to Dimensions changes and renders a DimensionsProvider with the values withWindowDimensions renders a DimensionsConsumer and is updated whenever an event from Dimensions happens Only one listener is used and only one component with lifecycle

Some numbers

on a chat with 3 messages I have 35 withWindowDimensions mounted
on a chat with 20 messages I have 101 withWindowDimensions mounted
adding an attachment would raise the count by 4-5
adding a plain text would raise the count by 2

I can open a separate ticket about this if you'd like

marcaaron commented 3 years ago

When you pick a chat it starts rendering instantly (well not yet 😄) and the drawer would actually help cover this. If you wait for the transition to end you will see a slide/fade transition then a white screen then chat items will start appear as they are rendered

This behavior maybe seems OK for web/mWeb where there is no swipe gesture? And the problem doesn't exist on native so maybe this could help.

all of them would assign listeners and use lifecycle methods which makes an impact on mobile especially during startup when they allocate their resources

Ah yes, I've thought about this before, but have not investigated the impacts (some benchmarks would be helpful if you decide to create an issue). I did look into whether there is some event delegation to handle this on the react-native-web side. It doesn't seem so different from the context solution, but maybe it would be better?

kidroca commented 3 years ago

The optimization won't reduce these counts, but will remove additional instantiation of classes, allocating memory, and running their lifecycle methods

E.g. Instead of calling 100 times componentDidMount it will be just 1 Instead of creating 100 listener funcs for onDimensionChange.bind it will be one etc...

marcaaron commented 3 years ago

Sounds nice! Maybe try to test and benchmark so we have some idea of how much it can help? In any case, I would love to see an issue for it 👍

kidroca commented 3 years ago

This behavior maybe seems OK for web/mWeb where there is no swipe gesture? And the problem doesn't exist on native so maybe this could help.

Could be, but would add yet another Platform specifying handling

Removing focus on the other hand... 😄

https://user-images.githubusercontent.com/12156624/114098651-54e45d00-98ca-11eb-9134-f17f49536bd1.mov

Still behaves smooth (this is DEV ⬆️) I'm firmly convinced focus is at the core of the issue. Even when the screen is not yanked to the side, the input would sometime remain covered under the keyboard instead of adjusting above it

parasharrajat commented 3 years ago

@kidroca with respect to the above, If you could put a check in the Event listeners so they fire on each render. It would save a lot of computations. And yes, Putting focus immediately cause the animations to stop for Search, new Chat, and new Group page. They should be handled via InteractionManager.

kidroca commented 3 years ago

If you could put a check in the Event listeners so they fire on each render. It would save a lot of computations.

Sorry I didn't get that. The problem is not that onDimensionChange would fire often, it actually would not It definitely won't be triggered for each render

parasharrajat commented 3 years ago

Yeah, Correct. I think putting it in context would stop its triggering(Currently it fires a lot).

kidroca commented 3 years ago

Create a ticket about withWindowDimensions: #2326

kidroca commented 3 years ago

Proposal

It seems you want this fixed, but dropping auto focus on mobile is not a suitable solution for you You want this bug fixed and still retain the ability to auto focus even if you decide against auto focus at some point down the road, correct?

Here's a proposal in that direction: Apply the autofocus after the nav/modal transition ends so that focusing does not interfere with the transition

A potential fix might be to encapsulate logic inside the TextInputFocusable that would react to navigation state like this

// from: import { useFocusEffect } from '@react-navigation/native';
useFocusEffect(() => {
  // This code would run when a screen containing THIS TextInputFocusable is focused
  if (props.autoFocus) {
    textInput.focus();

    // the uglier version (in case the above doesn't work) is to focus after a brief  `setTimeout`
    //  (hook logic should be added to clear pending timers)
  }
})

The code above would run for a TextInputFocusable that is a part of a screen which becomes focused

Actually disregard the setTimeout note here's what I found out: Delaying effect until transition finishes

useFocusEffect(
  React.useCallback(() => {
    const task = InteractionManager.runAfterInteractions(() => {
      textInput.focus();
    });

    return () => task.cancel();
  }, [])
);

I'm using the hook syntax for simplicity this can be recreated with a HOC, though it was agreed that in situations like this one it might be for the best to just use hooks

TextInputFocusable is a class component so the hook code cannot go directly there, but you can easily wrap the TextInput used as an end result in a functional component wrapping

props.autoFocus Can serve to override this behavior if needed.

there is more than one input field on the screen, we want to make sure the focus is on the first field
you decide to drop autofocus on mobile

The ticket mentioned above #2326 could also be considered as when the keyboard appears all the dimension listeners from withWindowDimensions HOC are triggered all at once.

marcaaron commented 3 years ago

It seems you want this fixed, but dropping auto focus on mobile is not a suitable solution for you You want this bug fixed and still retain the ability to auto focus even if you decide against auto focus at some point down the road, correct?

I'm not sure that's correct. Pretty sure we have unanimously come to the decision that we should stop auto focusing this input when we open up to a new chat (on mobile and mobile web at least).

I'm curious though, after the ReportView changes is this issue still reproducible?

kidroca commented 3 years ago

@marcaaron

I'm not sure that's correct. Pretty sure we have unanimously come to the decision that we should stop auto focusing this input when we open up to a new chat (on mobile and mobile web at least).

Then it would be even easier. If the behavior become platform specific. you could already use the existing autoFocus prop to disable this behavior on small screens. There are some tricks to detect whether the current device is touch only (software keyboard) or it has a dedicated keyboard but things become a bit hacky and need better research on the topic.

I'm curious though, after the ReportView changes is this issue still reproducible?

Yes, it doesn't happen 100% of the time, but it would still happen (mobile web Safari). I'm using webpack dev server, though, for a prod build it might happen less often

kidroca commented 3 years ago

Since autofocus still happens on mobile web this issue still occurs:

Autofocus still happens when switching chats due to: https://github.com/Expensify/Expensify.cash/blob/6db2ed04dcfd04aba495371dda93983f7691a058/src/components/TextInputFocusable/index.js#L94-L96

Autofocus still happens on FAB -> search, or FAB -> new chat because: https://github.com/Expensify/Expensify.cash/blob/431b60d09f4e41c7532707454625dbe2c3c22c21/src/components/OptionsSelector.js#L89-L91

Earlier I've provided info on how autofocus is causing this issue. Since it is decided that on mobile and mWeb fields should not be focused automatically addressing the remaining input focus issues would fix this bug as well.

Proposal

Refactor TextInputFocusable so that the default value for autoFocus prop is based on this logic here: https://github.com/Expensify/Expensify.cash/blob/6db2ed04dcfd04aba495371dda93983f7691a058/src/pages/home/report/ReportActionCompose.js#L99

Defaulting field autoFocus to true for desktop/web and false for mobile and mWeb

Fix the on mount code of TextInputFocusable to respect the autoFocus

https://github.com/Expensify/Expensify.cash/blob/6db2ed04dcfd04aba495371dda93983f7691a058/src/components/TextInputFocusable/index.js#L94-L96

Simply removing the call to focus is enough as the autoFocus prop is applied as attribute on the DOM node and the browser will handle this

Since we modify the default props autoFocus can be overriden if necessary by the parent component using TextInputFocusable

Update the code in the OptionsSelector to use the TextInputFocusable component instead of TextInputWithFocusStyles

https://github.com/Expensify/Expensify.cash/blob/431b60d09f4e41c7532707454625dbe2c3c22c21/src/components/OptionsSelector.js#L170-L175

Or Update the TextInputWithFocusStyles to use a TextInputFocusable internally instead of TextInput: https://github.com/Expensify/Expensify.cash/blob/431b60d09f4e41c7532707454625dbe2c3c22c21/src/components/TextInputWithFocusStyles.js#L64-L67

Whichever is easier and makes more sense

This would allow reuse and cut down repetitive code

~Remove the canFocusInputOnScreenFocus from ReportActionCompose since the behavior will be applied by the TextInputFocusable~

Edit: the logic actually would have to stay because of the modal handling:

https://github.com/Expensify/Expensify.cash/blob/8c80984da71fc8512658818f4e13f9df76ad2601/src/pages/home/report/ReportActionCompose.js#L123-L127

I need to think this a bit more, but there might be a way to address this more elegantly, or not addressed at all for the moment

marcaaron commented 3 years ago

@kidroca It does seem to improve things quite a bit and also that behavior to focus on mount in TextInputFocusable is 100% wrong given the recent changes.

Anyways, I think this is a good improvement and just removing the this.focusInput() alone seems to do wonders for un-sticking the drawer. It is confusing to have TextInputWithFocusStyles and TextInputFocusable so I am in favor of refactoring that stuff. Maybe we can split some of the proposed changes into multiple PRs.

One thing I've also noticed is that when a new report is loaded this can also cause re-renders in the SidebarLinks (which are pretty expensive to re-render) so preventing re-render when the drawer is closed might also get us some minor improvements. The next version of react-navigation provides a useDrawerProgress hook that could be used to delay expensive operations, but we can probably worry about that another time.

parasharrajat commented 3 years ago

I think this is a good improvement and just removing the this.focusInput()

@marcaaron It's worth mentioning that it has been removed already. https://github.com/Expensify/Expensify.cash/pull/2528

kidroca commented 3 years ago

It is confusing to have TextInputWithFocusStyles and TextInputFocusable so I am in favor of refactoring that stuff. Maybe we can split some of the proposed changes into multiple PRs.

Yes, I that was my initial thought but seeing that TextInputFocusable does not deal with styles - just the behavior. It would be easier to put it inside TextInputWithFocusStyles to verify the bug is fixed. And then we can arrange something better. Because TextInputFocusable does not deal with styles itself it might be necessary to be wrapped by a "styling" component like TextInputWithFocusStyles otherwise the styles would go up to OptionsSelector ...

kidroca commented 3 years ago

The next version of react-navigation provides a useDrawerProgress hook that could be used to delay expensive operations, but we can probably worry about that another time.

Couldn't the InteractionManager.runAfterInteractions be used as it's intended exactly for this. Or it doesn't work for some reason?

kidroca commented 3 years ago

One thing I've also noticed is that when a new report is loaded this can also cause re-renders in the SidebarLinks (which are pretty expensive to re-render)

I'll take a look too

marcaaron commented 3 years ago

Sorry @kidroca let's actually hold since maybe there is not much more that needs to be done if this has already been fixed. InteractionManager.runAfterInteractions() doesn't seem as relevant to issues affecting web.

kidroca commented 3 years ago

@marcaaron Only the code in TextInputFocusable was updated - The issue would be fixed when switching chats, but not for "Search", "New Chat", "New Group Chat", other options with an input field

The autofocus inside the OptionSelector is still a fact: https://github.com/Expensify/Expensify.cash/blob/431b60d09f4e41c7532707454625dbe2c3c22c21/src/components/OptionsSelector.js#L89-L91

This needs to be updated so that autofocus happens only on desktop/web.

It would be better to have this behavior encapsulated in TextInputFocusable and reused for all the places (and future usages) that need auto focus on desktop/web but not on mobile

parasharrajat commented 3 years ago

@kidroca This is has been taken care of by https://github.com/Expensify/Expensify.cash/pull/2505. But moreover, I think autofocus is one factor and there are also other involved factors that are causing this issue. If you try to resize the browser window you can again reproduce this issue.

Other than this I noticed is that we should debounce the dimension changes which is really causing a lot of calculations

Also only emit the changes when they actually change by checking the state changes here https://github.com/Expensify/Expensify.cash/blob/07370d220e9ab02edeb79087381cd13bff5a24ea/src/components/withWindowDimensions.js#L56-L60

by putting an if the check for previous and new values.

Sorry for the tone. Just wanted to reduce the duplicated efforts/ remove conflicts I didn't mean to.

kidroca commented 3 years ago

Ok so "no autofocus" on mobile is not 100% true

This will still auto focus on all platforms

       this.interactionHandle = InteractionManager.runAfterInteractions(() => {
            this.textInput.focus();
        });

As Marc pointed out

InteractionManager.runAfterInteractions() doesn't seem as relevant to issues affecting web.

The issue would probably still occur on mobile safari

kidroca commented 3 years ago

Other than this I noticed is that we should debounce the dimension changes which is really causing a lot of calculations

I've added measurements on the effect of dimension changes in the ticket here: #2326 There are no dimension changes happening when the bug occurs "naturally" - the users aren't changing window size or anything they just switch screens and this does not trigger dimension changes

marcaaron commented 3 years ago

This needs to be updated so that autofocus happens only on desktop/web.

To clarify, I'm not sure this has actually been decided. The case that we are discussing here is switching between chats.

kidroca commented 3 years ago

Ok so this ticket is only about switching chats and the "Search", "New Chat", "New Group Chat" will be handled separately, or you mean focus handling for them is not decided yet?

Just want to point out that all the "discoveries" here are related to "Search", "New Chat", "New Group Chat" and the issue is the same - a transition that involves immediate focus causes the "stuck offscreen" bug

marcaaron commented 3 years ago

this ticket is only about switching chats and the "Search", "New Chat", "New Group Chat" will be handled separately, or you mean focus handling for them is not decided yet?

Both of those statements are correct. This issue is related to switching chats (seems to be fixed now). As for disabling input focus on other screens I think probably it would be best to get some other opinions.

kidroca commented 3 years ago

Sorry, I'm not talking about that autofocus would necessarily need to be removed on those screens, but the same "stuck offscreen" bug would remain for them

The same issue occurs when you use the FAB, but I don't think there's a separate ticket about it, I thought this ticket would address it as well. I've brought that up a couple of times: https://github.com/Expensify/Expensify.cash/issues/2051#issuecomment-811881242

https://user-images.githubusercontent.com/12156624/113295396-d3f0f880-9300-11eb-8aaa-d5c258d1d67a.mp4

Also if autofocus should stay for the "Search", "New Chat", "New Group Chat" ... I've made another proposal that can keep autofocus and still fix the bug: https://github.com/Expensify/Expensify.cash/issues/2051#issuecomment-818045780

Anyway I'll wait for things to get cleared out

marcaaron commented 3 years ago

Ok sorry for all the back and forth here, but I think what we should do at this point is close this issue and create a new one so we can:

Verify the new issue you've discovered and the severity
See what can be done about the input focusing there or if it should be disabled

I thought this ticket would address it as well. I've brought that up a couple of times

Apologies for the confusion here. I should have encouraged us to create a new issue for that earlier, but assumed they would have the same root cause. Given that the main issue here is fixed, but the other is not we should create a new one.

mallenexpensify commented 3 years ago

@kidroca do you want to create the new issue, propose a solution, then fix it? If so, you'll be eligible for the $150 bonus

kidroca commented 3 years ago

Yeah, I'll open a new ticket next week.

We'd have to discuss this there before a proposal can be made:

See what can be done about the input focusing there or if it should be disabled

Expensify / App