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

refactor(datetime): use scroll listener to detect month changes #25586

Merged
merged 25 commits into from
Jul 15, 2022

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Jul 7, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue URL: resolves #24980, resolves #25257

The datetime component in v6 was originally built using IntersectionObserver to detect when the month was swiped. This was done as it was easy to configure and had minimal impact on performance. Unfortunately, the implementation of IntersectionObserver across browsers is inconsistent, buggy, and overall unreliable for our use case.

Related issues:

#25007
#24482
#24472
#24405
#23992
#23985
#25257
#24980

We have made several patches to avoid these issues. However, the team has decided to look into using a scroll listener instead to improve the reliability of the component.

What is the new behavior?

This PR makes the following changes:

  • Removes the IO on the calendar body and replaces it with a scroll event listener. This scroll listener sets a timer that fires when scrolling stops (~150ms after the last scroll event). The setTimeout callback then runs the same logic that the IO callback did to update the active month.
  • Removes the IO-specific hacks from the month change callback.
  • Updates the name of listener methods to remove any mention of intersection observer
  • Removes the overlayIsPresenting hack
  • Added a tech debt ticket for the onIonChange hacks for the picker wheels.

Note: The following hacks for Safari 14 are still relevant as the scroll event is also impacted:

  1. // Due to a Safari 14 issue we need to destroy
  2. https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime.scss#L120

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 6.1.14-dev.11657300607.18e0f2d1

Performance

The performance difference is pretty minimal. The following stats were taken from an iPhone 7 running iOS 15.4.1.

(I also tested iOS 13 + iOS 14 and saw no behavior quirks)

Overall the CPU usage remained about the same. The main branch does show a slightly higher average CPU usage, but the difference between the two averages is only a few percentage points.

The screenshot with the scroll listener does show significantly more timers which is to be expected. Additionally, there is more purple (JavaScript & Events). We are using setTimeout to detect the end of the scroll gesture and cancelling it every time a new scroll event is dispatched.

main branch

image

datetime-scroll-listener branch
image (1)

I also tested this on a Samsung Galaxy S3 (https://www.gsmarena.com/samsung_galaxy_s_iii_cdma-4799.php) running Android 4.4 with Chrome 81. The phone was too slow to take a timeline in Chrome dev tools, but the swiping experience between main and this branch was very similar.

@github-actions github-actions bot added the package: core @ionic/core package label Jul 7, 2022
@liamdebeasi
Copy link
Contributor Author

liamdebeasi commented Jul 8, 2022

Open question: Should we remove the tests that were added to account for IntersectionObserver-related quirks? For example, the sub-pixel-width and zoom tests

@liamdebeasi liamdebeasi marked this pull request as ready for review July 8, 2022 17:38
@liamdebeasi liamdebeasi requested a review from a team as a code owner July 8, 2022 17:38
@liamdebeasi liamdebeasi requested review from averyjohnston and sean-perkins and removed request for a team July 8, 2022 17:38
@sean-perkins
Copy link
Contributor

Yeah I think removing the sub-pixel and zoom tests makes sense now that we don't have a dependency on an intersection observer.

@averyjohnston
Copy link
Contributor

^ Agreed.

Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work on this, should save us a bunch of trouble 😁

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Death to the IO 💀

@zhaoxiongxing
Copy link

This problem is most serious in earlier versions of Android and Huawei Hongmeng system. Thank you for solving this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
4 participants