Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kbecciv opened this issue Jul 8, 2022 · 56 comments
Closed
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kbecciv
Copy link

kbecciv commented Jul 8, 2022

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?

  • Web
  • iOS ✅
  • Android
  • Desktop App
  • Mobile Web

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

Bug5641034_mweb_0807.mp4
Bug5641034_mweb_freezes_0807.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Jul 8, 2022

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

@tylerkaraszewski
Copy link
Contributor

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

@tylerkaraszewski tylerkaraszewski removed their assignment Jul 8, 2022
@tylerkaraszewski tylerkaraszewski added the External Added to denote the issue can be worked on by a contributor label Jul 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 8, 2022

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

@jliexpensify jliexpensify changed the title mWeb - New Group - Page freezes when selecting 8 group members and unable to scroll the page mWeb - Page freezes when creating New Group and selecting 8 members Jul 11, 2022
@melvin-bot melvin-bot bot added the Overdue label Jul 11, 2022
@jliexpensify jliexpensify changed the title mWeb - Page freezes when creating New Group and selecting 8 members [iOS] - Page freezes when creating New Group and selecting 8 members Jul 11, 2022
@jliexpensify
Copy link
Contributor

I suspect this is an iOS issue:

  • Tested on Web, seems to be fine?

image

  • Tested on Android, works fine?

Both were 1.1.82.5

@melvin-bot melvin-bot bot removed the Overdue label Jul 11, 2022
@jliexpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 2022

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

@melvin-bot melvin-bot bot changed the title [iOS] - Page freezes when creating New Group and selecting 8 members [$250] [iOS] - Page freezes when creating New Group and selecting 8 members Jul 11, 2022
@rushatgabhane

This comment was marked as outdated.

@jliexpensify
Copy link
Contributor

@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
Copy link
Member

rushatgabhane commented Jul 11, 2022

@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.

Screen.Recording.2022-07-11.at.7.55.25.AM.mov

@jliexpensify
Copy link
Contributor

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

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

@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 11, 2022

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 😄

@kadiealexander kadiealexander changed the title [$1000] [iOS] - Page freezes when creating New Group and selecting 8 members [$2000] [iOS] - Page freezes when creating New Group and selecting 8 members Nov 10, 2022
@kadiealexander
Copy link
Contributor

Price doubled!

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@Julesssss
Copy link
Contributor

The latest update for this issue is still valid

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 14, 2022
@kevinksullivan
Copy link
Contributor

@kadiealexander looks like this could be doubled again?

@JmillsExpensify
Copy link

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

  • Decide whether any proposals currently meet our guidelines and can be approved as-is
  • For any that can't, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #bug-zero

Thanks everyone!

@melvin-bot
Copy link

melvin-bot bot commented Nov 18, 2022

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

@Julesssss
Copy link
Contributor

Hi @kadiealexander could you double the price again please

@melvin-bot melvin-bot bot removed the Overdue label Nov 18, 2022
@rushatgabhane
Copy link
Member

And.. give it a shoutout on slack please 🙏

@kadiealexander kadiealexander changed the title [$2000] [iOS] - Page freezes when creating New Group and selecting 8 members [$4000] [iOS] - Page freezes when creating New Group and selecting 8 members Nov 21, 2022
@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2022
@kadiealexander
Copy link
Contributor

kadiealexander commented Nov 21, 2022

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 21, 2022
@railway17
Copy link
Contributor

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
Copy link
Contributor

hellohublot commented Nov 21, 2022

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

_before_should_notice_the_blur.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

_after.mp4

// Main code

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

OR

// Full code diff
hellohublot@0377b83?diff=unified

Waiting for your suggestion

@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

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
Copy link
Contributor

Proposal Append For #9809 (comment)

This is another solution, the screenshot is same as the previous solution #9809 (comment), but this PR #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

export default function getCardStyles(isSmallScreenWidth, screenWidth) {
return {
position: 'fixed',
width: isSmallScreenWidth ? screenWidth : variables.sideBarWidth,
right: 0,
height: '100%',
};
}

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
Copy link
Member

reviewing today

@dylanexpensify
Copy link
Contributor

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

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 22, 2022

@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?

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 22, 2022

@hellohublot could you please explain why position absolute would fix the bug for safari?
Why isn't it required for other browsers?

Anyway, I don't think we'll move ahead with a solution that requires us to detect browsers

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 22, 2022

Hi @hellohublot 👋
Are you on expensify's opensource slack channel yet? If yes, what's your handle?

@marcaaron
Copy link
Contributor

I'm kinda lost on this issue. Is it only affecting Safari on older iOS versions? I was not able to repro this at all on iOS 15 version of Safari.

@hellohublot
Copy link
Contributor

hellohublot commented Nov 23, 2022

Q & A

Are you on expensify's opensource slack channel yet?

I have sent the application email to contributors@expensify.com, my username is hublot@aliyun.com

How to reproduce

You can slide the list of this page to the top, then continue to drag down, and then repeatedly drag up and down,
Or slide the list of this page to the bottom, continue to drag up, and then repeatedly drag up and down

Why it happened

SectionList.overflow.scroll does not support bounce, so this event cannot be accepted, and this event will be sent to Document, because Document always supports bounce

Can it be reproduced on iOS 15+

Document bounce has been changed to support pull-down refresh in this version, so this problem cannot be reproduced. I can’t borrow a mobile phone with iOS 15+ system. Maybe You can try to slide the list to the bottom and then drag it up

Can we intercept this event and hand it over to SectionList?

I have found many ways to solve this gesture penetration, but none of them work
Include the following.

  • touchmove.stopPropagation
  • document.body.position.fixed
  • overscroll-behavior: contain
  • webkit-overflow-scrolling: touch
  • replace SectionList to View

Finally, I guess if SectionList.overflow.scroll can support bounce, it must be inconsistent with w3c, because other web cores do not support bounce
So I ended up giving up on intercepting the event

My solution

Although we can't intercept the Document.scroll.bounce event, we can give the user some visual feedback instead of the page gesture being frozen

First solution addEventListener('scroll')

We can listen the offset change of Document.scroll.bounce, and then let the entire page of NewGroupChat scroll together

Second solution position: 'absolute'

This link can explain what is the difference between 'absolute' and fixed, https://www.w3.org/wiki/CSS_absolute_and_fixed_positioning?source=post_page
Simply put, absolute will not break away from the document flow, and the pop-up window will slide along with the document flow, but fixed cannot slide along with the document flow

So if we don't want the first solution, we can change NewGroupPage to absolute, it will automatically scroll along with the document flow, so we don't have to listen to offset

Can we cancel the position condition and set it all to absolute?

I don't think so, because only MobileSafari supports bounce, it is special so we also treat it specially,

If we really need to cancel the position condition, set them all to absolute.

The code I want to debug conflicts with this PR #12366. This PR caused some bugs in https://staging.new.expensify.com/, you can use MobileSafari to go reproduce those bugs, such as ProfileScreen and SearchScreen and more, so I want to wait until they are fixed, and I also hope they can continue to use cardStyle.position, then I can continue to debug and answer this question

Thanks !

@trjExpensify
Copy link
Contributor

I have sent the application email to contributors@expensify.com, my username is hublot@aliyun.com

Hey @hellohublot, that email hasn't been received on our side. Can you resend it and include a link to your Upwork profile please. After which, we can get you added to Slack. Thanks!

@trjExpensify
Copy link
Contributor

I'm kinda lost on this issue. Is it only affecting Safari on older iOS versions? I was not able to repro this at all on iOS 15 version of Safari.

I'm with @marcaaron here. I can't reproduce this on iOS 15 (staging v1.2.30-0) using a iPhone 13 Pro.

RPReplay_Final1669209350.MP4

Given that this issue has not been proven to be reproducible on iOS 15 or later, and it's isolated to mWeb Safari not impacting the material use of the app, I'm going to close this issue. Let's channel our efforts elsewhere! 🚀

If anyone disagrees, feel free to continue the discussion in this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests