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

Temporarily hold timeline if mouse moved recently (fixes #8630) #9200

Merged
merged 1 commit into from Nov 5, 2018

Conversation

Projects
None yet
2 participants
@lmorchard
Copy link
Contributor

lmorchard commented Nov 4, 2018

  • On recent mouse movement, hold timeline position so statuses remain in
    place for interactions in progress.

  • If the timeline had been scrolled to the top before mouse movement,
    restore scroll on mouse idle.

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 4, 2018

You should know I have implemented and then reverted a similar patch. The mouse moves all the time. Having the timeline constantly stuck in the past is very frustrating. Ref: #4859, #4865 (looks like you already know about that as you commented in that one)

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 4, 2018

I can see one difference is that the old patch was based on the hover state, while yours is based on move timing. It could be better but I don't know by how much.

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Nov 4, 2018

You should know I have implemented and then reverted a similar patch.

Yeah, I figured I'd give it one more shot, because mis-clicking on the wrong status really bothers me. If this attempt gets rejected, I'll understand. 😅

For what it's worth, I used a timer because the _recentlyMoved logic wouldn't be checked until the next mouse move or other column state update (e.g. new statuses).

I think a timer makes this more responsive - because one of the things I've got it doing is scrolling back to the top of the column on mouse idle, so that it doesn't actually stay stuck in the past.

Temporarily hold timeline if mouse moved recently (fixes #8630)
- On recent mouse movement, hold timeline position so statuses remain in
  place for interactions in progress.

- If the timeline had been scrolled to the top before mouse movement,
  restore scroll on mouse idle.

@lmorchard lmorchard force-pushed the lmorchard:8630-timeline-pause branch from cd0db8c to aaa03ac Nov 4, 2018

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Nov 4, 2018

Pushed a small tweak to better clear the timer, since I realized the property never returned to null after a clearTimeout

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 4, 2018

I think a timer makes this more responsive - because one of the things I've got it doing is scrolling back to the top of the column on mouse idle, so that it doesn't actually stay stuck in the past.

I have to ask - what if I intentionally scroll down to visually mark my position on something. This would pull me back to top?

Consider an alternative to this is disabling updates on mouseDown, since the click event is fired on mouseUp, the most annoying things are when you click on something and then it moves from under your mouse when you release, right?

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Nov 4, 2018

I have to ask - what if I intentionally scroll down to visually mark my position on something. This would pull me back to top?

No, if you're not at scrollTop === 0 when a mouse move starts, scroll to top on idle doesn't happen. So, if you're scrolled down at the start of a mouse move, this code does basically nothing.

Consider an alternative to this is disabling updates on mouseDown, since the click event is fired on mouseUp, the most annoying things are when you click on something and then it moves from under your mouse when you release, right?

I don't think pause-on-mouseDown would help. For me, the issue feels most often like the timeline has moved in the span of approaching, landing on, and then clicking on a UI element. The mouseDown / mouseUp span is the shortest part of all that.

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 5, 2018

Okay, so there's kind of only one way to find out. I will merge this, it will get some use from people running master, and then we'll know if it's a good change or if the behaviour will be disturbing. Fingers crossed but if it'll turn out worse I'll revert it, just a heads up 🙏

@Gargron

Gargron approved these changes Nov 5, 2018

@Gargron Gargron merged commit 6a1216d into tootsuite:master Nov 5, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Nov 5, 2018

Fingers crossed but if it'll turn out worse I'll revert it, just a heads up 🙏

Yay, thanks for the consideration! No worries from me if it gets reverted. Figured I'd give it one more try

yi0713 added a commit to yi0713/mastodon that referenced this pull request Nov 7, 2018

Gargron added a commit that referenced this pull request Nov 7, 2018

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 7, 2018

@lmorchard Could you please investigate #9217 as I am now getting complaints from people on servers running master. It seems that this patch interferes with manual scrolling. If it's the result of a minor bug, it would be great to find it, otherwise I am preparing to revert this.

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

lmorchard commented Nov 7, 2018

Sure, I will take a look in a few hours. I think I see what's going on.

Edit: I see there's already a PR to revert - should I investigate? Or are you already on path to revert?

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 7, 2018

As I said I am prepared to revert, but I can wait until you investigate.

lmorchard added a commit to lmorchard/mastodon that referenced this pull request Nov 8, 2018

Cancel list scroll reset after mouse move on wheel scroll
Also fixes an error introduced in tootsuite#9200 caused by attempting to use an
undefined prop.

Fixes tootsuite#9217

lmorchard added a commit to lmorchard/mastodon that referenced this pull request Nov 8, 2018

Cancel list scroll reset after mouse move on wheel scroll
Also fixes an error introduced in tootsuite#9200 caused by attempting to use an
undefined prop.

Fixes tootsuite#9217

lmorchard added a commit to lmorchard/mastodon that referenced this pull request Nov 8, 2018

Cancel list scroll reset after mouse move on wheel scroll
- Use object properties rather than component state for
  mouseMovedRecently and scrollToTopOnMouseIdle flags

- Fix an error introduced in tootsuite#9200 caused by attempting to use an
  undefined prop.

Fixes tootsuite#9217

yi0713 added a commit to yi0713/mastodon that referenced this pull request Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment