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

fix(scroll): acidental scroll to top on iOS devices #2695

Merged
merged 7 commits into from
Apr 27, 2024
Merged

Conversation

neSpecc
Copy link
Member

@neSpecc neSpecc commented Apr 27, 2024

Problem

When you type something on iOS, page will be scrolled to top.

IMG_5245.MP4

Fixes #2689
Possible fixes #2261

Cause

Due to lack of the "strict" mode in the tsconfig there was a missed problem in the "scroll-locker.ts" class.

The iOS-specific method unlockHard() (called from the Popover' hide() which is being called every keydown) checks for null, but this.scrollPosition was undefined initially

  /**
   * Unlocks hard scroll lock
   */
  private unlockHard(): void {
    document.body.classList.remove(ScrollLocker.CSS.scrollLockedHard);
    if (this.scrollPosition !== null) { 
      window.scrollTo(0, this.scrollPosition);
    }
    this.scrollPosition = null;
  }

Solution

  1. I initialized this.scrollPosition with null explicitly.
  2. Additionally, I updated the PopoverMobile's hide() method. Now it won't trigger any logic if popover is already hidden (to prevent possible layout caused by CSS classes adding)

@@ -64,6 +64,12 @@ export class PopoverMobile extends PopoverAbstract<PopoverMobileNodes> {
* Closes popover
*/
public hide(): void {
const isAlreadyHidden = this.nodes.overlay.classList.contains(css.overlayHidden);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe store isOpen flag in a variable? Not sure it's a good idea to check dom

Copy link
Member Author

Choose a reason for hiding this comment

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

why not? it does not lead layout

@neSpecc neSpecc merged commit 1028577 into next Apr 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants