Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.85k forks source link

[$4000] [iOS] - Page freezes when creating New Group and selecting 8 members #9809

Closed kbecciv closed 1 year ago

kbecciv 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:

  1. Go to URL https://staging.new.expensify.com/
  2. Click on the green + button
  3. Click on Create Group
  4. Start the group creation flow and add 8 participants.
  5. Scroll the page

Expected Result:

The page should move and scroll when adding 8 members

Actual Result:

Page freezes when selecting 8 group members and unable to scroll the page

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.82.5

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/178056852-7b0e4aee-ee0a-4fdd-a3d1-e1b23c813198.mp4

https://user-images.githubusercontent.com/93399543/178056853-ffb30259-015a-45cf-8661-186c0b52a806.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

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

tylerkaraszewski commented 2 years ago

Reproduced on iOS on my iPhone 13, does not scroll correctly.

melvin-bot[bot] commented 2 years ago

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

jliexpensify commented 2 years ago

I suspect this is an iOS issue:

image

Both were 1.1.82.5

jliexpensify commented 2 years ago

Posted:

Internal - https://www.upwork.com/ab/applicants/1546309325160779776/job-details External - https://www.upwork.com/jobs/~0166ca314670548e41

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

rushatgabhane commented 2 years ago

main works fine on iOS and mWeb Safari.

v1.1.79-18, iPhone 13

https://user-images.githubusercontent.com/29673073/178189987-51cb8c46-01f7-4434-ae07-a08f020c42d6.mov

jliexpensify commented 2 years ago

@rushatgabhane it looks like we all tested on 1.1.82.5 - can you try with that version? I believe that's newer right?

rushatgabhane commented 2 years ago

@jliexpensify yeah my bad, here's v 1.1.82-5

The main branch works well on safari mWeb, and iOS. So I'm guessing this issue will fix itself in the next release cycle.

https://user-images.githubusercontent.com/29673073/178191668-a3e0235a-34eb-44c7-bb9c-b89f42034095.mov

jliexpensify commented 2 years ago

@rushatgabhane - awesome, thanks for testing! Might just be general lagginess happening then?

Can we close this issue, since it'll be fixed soon?

rushatgabhane commented 2 years ago

Can we close this issue

@jliexpensify not just yet, we should close when QA can't repro this bug during the weekly tests.

Because I could be wrong 😄

jliexpensify commented 2 years ago

Not overdue, waiting on weekly tests!

rushatgabhane commented 2 years ago

@kbecciv is this issue still reproducible on staging?

jliexpensify commented 2 years ago

I've had to take some unexpected time off, so assigning to Kadie (my leave buddy) to manage!

jliexpensify commented 2 years ago

TL;DR - waiting for @kbecciv to confirm if this is an issue on staging.

If not, we can close this GH!

kbecciv commented 2 years ago

@jliexpensify Sure, will update you shortly.

kbecciv commented 2 years ago

@jliexpensify Tester was able to reproduce it with latest build 1.1.87.8

https://user-images.githubusercontent.com/93399543/182613452-ebbbc3e0-8b87-4690-8429-8f1990849246.mp4

rushatgabhane commented 2 years ago

Thanks so much for testing it @kbecciv I can't repro this on staging and production.

Maybe this is a physical device only thing. @Julesssss if you have an iPhone, can you please verify?

v1.1.87-8

https://user-images.githubusercontent.com/29673073/182625884-06f937db-51c5-4f82-a2a1-3cc907ae6b68.mov

v1.1.86-5

https://user-images.githubusercontent.com/29673073/182625124-7451b4c3-72cb-4ee3-9414-388616524d61.mov

Julesssss commented 2 years ago

Maybe this is a physical device only thing. @Julesssss if you have an iPhone, can you please verify?

I tested on v1.187-9 and I'm also unable to reproduce. Physical iPhone 7 running 13.3

@kbecciv could you share the OS version and device that your team can reproduce it on? Thanks.

kbecciv commented 2 years ago

Checking with team, will update you shortly

kbecciv commented 2 years ago

@Julesssss Please attached the version and model of the device. iPhone xs Max/ 14.8.1

Image from iOS (43)

Julesssss commented 2 years ago

Sharing internally to see if anyone can reproduce with a similar device

kadiealexander commented 2 years ago

@Julesssss did you have any luck getting someone to repro this?

Julesssss commented 2 years ago

No, and I'm not sure we can do anything further until we have specific steps to narrow down the device 😕 I wonder if it has something to do with device language, as I can't think what else differs between the test devices

kadiealexander commented 2 years ago

@tylerkaraszewski I saw you were able to reproduce this in the past on an iPhone 13. Could you please let us know the OS version, and whether it's still reproducible?

Julesssss commented 2 years ago

Page freezes when selecting 8 group members and unable to scroll the page

@trjExpensify am I crazy, or wasn't the chat supposed to be limited to 8 participants TOTAL?

I'm able to create a chat with 9 participants (myself included), and the UI does sort of lock in place. I wonder if that's what the initial issue was trying to describe, instead of the app actually locking up?

rushatgabhane commented 2 years ago

am I crazy, or wasn't the chat supposed to be limited to 8 participants TOTAL

holy shit, yeah?? Btw, the backend allows a group of 9 to be created.

and the UI does sort of lock in place

yes but it's not a freeze 🥶 (i can still scroll, and unselect participants)

Julesssss commented 2 years ago

Okay, so this is not a bug on most devices IMO. With 8 participants selected you view and remove all of them. Perhaps this is an issue on smaller devices though, but this needs to be confirmed.

Does anyone have a small device that restricts participant deselection?

kadiealexander commented 2 years ago

Same experience as Rajat for me on the ol' Asus phone. It removes the list of people you can select, but doesn't freeze - you can still deselect and take actions.

Julesssss commented 2 years ago

Same experience as Rajat for me on the ol' Asus phone. It removes the list of people you can select, but doesn't freeze - you can still deselect and take actions.

Cool, so that sounds okay.

Looking at the original reports I do think we should try to allow scroll when the participants are locked. But this is a pretty minor bug.

kadiealexander commented 2 years ago

Just adding a note that I'm OOO until September 21st. As this is on hold I won't reassign it, but if you need a contributor manager to do anything before that point please reapply the external label.

kadiealexander commented 2 years ago

Job doubled! New Upwork post:

https://www.upwork.com/ab/applicants/1584714876102766592/job-details https://www.upwork.com/jobs/~015735edd1fac507c5

puneetlath commented 1 year ago

@kadiealexander I added the help wanted label since I believe we're looking for external proposals, yeah? Also, it looks like it might be time to double again.

kadiealexander commented 1 year ago

Sounds good! It's been a hot minute, so here's the new Upworks post doubled to $1000:

https://www.upwork.com/ab/applicants/1587903133329772544/job-details https://www.upwork.com/jobs/~019d964b285383ca3e

kadiealexander commented 1 year ago

Price doubled!

Julesssss commented 1 year ago

The latest update for this issue is still valid

kevinksullivan commented 1 year ago

@kadiealexander looks like this could be doubled again?

JmillsExpensify commented 1 year ago

@Julesssss To help clear out the extensive backlog of /App bugs, we're putting the spotlight on all bugs older than 4 weeks old. To help unblock the roadmap and get our bug pipeline back in equilibrium, can you:

Thanks everyone!

melvin-bot[bot] commented 1 year ago

@Julesssss, @rushatgabhane, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

Julesssss commented 1 year ago

Hi @kadiealexander could you double the price again please

rushatgabhane commented 1 year ago

And.. give it a shoutout on slack please 🙏

kadiealexander commented 1 year ago

Sorry, was out sick at the end of last week. Job doubled!

railway17 commented 1 year ago

Is this issue still reproducible? I couldn't try to test it on old versions of iOS but looks like not reproducible in newer versions.

hellohublot commented 1 year ago

Proposal

Reproduce

Reproducible don't need adding 8 users, pull down when SectionList is at the top, then pull up repeatedly, or pull up when SectionList is at the bottom, then pull down repeatedly. You can notice the blur changes of the safari navigationBar and tabBar in the follow video

https://user-images.githubusercontent.com/20135674/203045431-26630035-6a75-446c-afd6-351ebbdccbbe.mp4

This is because of the conflict between document.scroll and SectionList.overflow.scroll, SectionList.overflow.scroll can't bounce, but document.scroll always supports bounce, so these bounce scroll gestures will be sent to document.scroll, I have tried ['touchmove.stopPropagation', 'document.body.position.fixed', 'overscroll-behavior: contain', '-webkit-overflow-scrolling: touch', replace SectionList to View], none of these can solve this scroll penetration, but we can give the user some visual feedback instead of gesture freezing

https://user-images.githubusercontent.com/20135674/203047155-ee5c017e-cbd5-44e1-8924-44d1b19b7bd5.mp4

// Main code

window.addEventListener('scroll', () => {
    this.followWindowScrollContainer.current.style.top = `${-document.documentElement.scrollTop}pt`
})

OR

// Full code diff https://github.com/hellohublot/App/commit/0377b8355cdf8954e7fbd2b7e403bbc1e55bd370?diff=unified

Waiting for your suggestion

melvin-bot[bot] commented 1 year 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.

hellohublot commented 1 year ago

Proposal Append For https://github.com/Expensify/App/issues/9809#issuecomment-1321958007

This is another solution, the screenshot is same as the previous solution https://github.com/Expensify/App/issues/9809#issuecomment-1321958007, but this PR https://github.com/Expensify/App/pull/12366 removed the cardStyle position today, I also found some problems on mobile_safari in that PR, such as the head of SearchScreen cannot be fixed at the top when scroll, such as the first time you enter the ProfileScreen page to edit the LastName, the page will move to the left, maybe should wait before he/she solve that

if we still need cardStyle position in the future, you can take a look at this solution, Thanks

https://github.com/Expensify/App/blob/6faef3fe06a35839f13e74734acdce111b13c948/src/styles/cardStyles/index.js#L9-L16

export default function getCardStyles(isSmallScreenWidth, screenWidth) {
    return {
        position: Browser.getBrowser() == CONST.BROWSER.SAFARI && Browser.isMobile() ? 'absolute' : 'fixed',
        ...
    };
}
<ScrollDocument>
    <html>
        <body>
              <ReportScreen />
              <NewGroupScreen style={{ position:  isMobileSafari ? 'absolute' : 'fixed' }}>
                    <SectionList style={{ overflow: 'scroll' }} />
              </NewGroupScreen>
        <body>
    <html>
</ScrollDocument>
rushatgabhane commented 1 year ago

reviewing today

dylanexpensify commented 1 year ago

Amazing, just as a friendly reminder, we expect all reviews to be completed and done within 48 hours!

rushatgabhane commented 1 year ago

@hellohublot thank you for your detailed proposal!

This is because of the conflict between document.scroll and SectionList.overflow.scroll, SectionList.overflow.scroll can't bounce, but document.scroll always supports bounce, so these bounce scroll gestures will be sent to document.scroll

i understand

this.followWindowScrollContainer.current.style.top = ${-document.documentElement.scrollTop}pt

I feel like in the first proposal we're trying to replicate the bounce behavior using a hack. You've found the root cause (conflict between sectionList and document scroll). Can we fix that in some way?