Skip to content

Conversation

@Usiel
Copy link
Contributor

@Usiel Usiel commented Aug 8, 2020

  • Do not abort nestedScroll
  • Enable swipe to refresh only when no vertical nestedOffset is indicated

Fixes #900

Steps to test this PR:
See #900 linked websites

While this fixes the issue it's not pretty unfortunately. I observed the following behavior with Chrome, but not quite sure how to implement it:

  1. Swipe refresh enabled on load
  2. User swipes down -> Swipe refresh disabled
  3. User swipes up and triggers the overscroll animation (swipe refresh still disabled)
    a) If user swipes up again while the overscroll animation is active -> Swipe refresh stays disabled
    or
    b) Overscroll animation finishes -> Swipe refresh enabled

- Do not abort nestedScroll

- Enable swipe to refresh only when no vertical nestedOffset is indicated

Fixes duckduckgo#900
@Usiel Usiel force-pushed the bug/usiel/fix_pull_to_refresh_triggered_early branch from 5cc9284 to 41a8e08 Compare August 8, 2020 07:35
@JeffAment
Copy link
Contributor

Thank you for fixing this 🙏 I see what you mean about the overscroll animation, but overall everything works much better with these changes.

I've added two other websites where this is easy to reproduce as an edit to my original post on #900; both of these sites also behave much better after these changes, and the issue only seems to happen when the overscroll animation is still active, though I do wonder if checking whether the address bar is fully docked via an offsetChangedListener etc. before enabling swipe to refresh would help this further, as the hidden address bar still seems to be the common denominator. Great work 👍

@cmonfortep cmonfortep self-assigned this Aug 10, 2020
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

LGTM, I agree there are still some user events that trigger the pull to refresh when it was not expected (hard to reproduce them) but, like @JeffAment already said, this is definitely an improvement compared to the previous experience, so really good job here for the quickest contribution.

@cmonfortep
Copy link
Contributor

@Usiel going to merge this PR as I don't want to delay the fix for our users.
The idea of @JeffAment about checking if the toolbar is fully shown seems interesting and maybe we want to explore it too. So if anyone has the time for that, feel free to go for it, I'm happy to review it or give support there.

@cmonfortep cmonfortep merged commit ea3eef2 into duckduckgo:develop Aug 10, 2020
@Usiel
Copy link
Contributor Author

Usiel commented Aug 13, 2020

@cmonfortep Sure makes sense. I'll see if I can tinker with Jeff's idea in the next few days and achieve a cleaner behavior hopefully (both code and UI).

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.

Swipe to refresh triggers when address bar is hidden

4 participants