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

[ALL-1182] PanModal 내 tableView를 스크롤 중일 때 tableView 업데이트 시 간헐적으로 발생하는 크래시 수정 #1

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

justin-handhug
Copy link

@justin-handhug justin-handhug commented Oct 17, 2023

개요

  • 상품 선택화면에서 간헐적으로 발생하는 크래시를 수정합니다.

재현 방법

  • 재고 정보가 캐시되지 않은 상품의 상품 선택화면 진입 후 재고 정보가 조회되는 동안 옵션 테이블뷰를 최하단으로 마구 스크롤합니다.
  • 재고 정보 조회가 완료되어 테이블 뷰가 업데이트 될 때 옵션 테이블뷰를 스크롤 중이면 간헐적으로 크래시가 발생합니다.

원인

  • panModal은 panModal과 scrollView간의 부드러운 트랜지션을 위해서 KVO를 이용해서 scrollView의 contentOffset의 변경을 감지하고 didPanOnScrollView 메소드로 scrollViewYOffset을 추적하여 값을 저장합니다.

  • 스크롤 중 테이블 뷰의 스크롤 뷰가 업데이트 되면 (특히 n개 남았어요 UI가 노출되면서 presentedView의 높이가 증가하는 경우) presentedView frame의 minY의 값이 줄어듭니다.

  • 줄어든 minY값은 didPanOnScrollView 메소드가 scrollViewYOffset의 추적을 멈추고 scrollViewYOffset 위치로 스크롤하도록 합니다.(이때 의도와 다르게 동작하게 되는 것 같음 그렇게 생각하는 이유는 정상 작동 시 추적을 멈추는 경우는 스크롤뷰를 아래로 내려서 panModal을 dismiss하려고 할 때 혹은 최하단 스크롤 후 화면에서 손을 떼서 바운스로인해 제자리로 찾아갈 때여서)

  • scrollViewYOffset 위치로 스크롤하도록 할 때 scrollViewYOffset이 실제 offset인 scrollView.contentOffset.y보다 값이 큰 상태입니다. 위 programatic한 스크롤은 스크롤 변경을 감지하는 KVO를 재귀적으로 호출하게 되면서 크래시가 발생합니다.

  • 이를 방지하기 위해 scrollViewYOffset는 실제 offset보다 작거나 같아야 합니다.

작업 내용

  • scrollViewYOffset 추적을 멈추고 scrollViewYOffset 위치로 스크롤하도록 할 때 scrollViewYOffset과 실제 offset 중 작은 값을 사용하도록 수정하였습니다.

기타

  • 간헐적으로 발생하는 이슈여서 완전한 검증이 어렵습니다. 반영 후 크래시 리포트를 확인하겠습니다.
  • panModal을 사용하는 쿠폰리스트에서 작동 확인

참조

@kanz-kang kanz-kang self-requested a review October 18, 2023 01:11
@kanz-kang kanz-kang merged commit f8dfc42 into jellycrew:master Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants