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

Fixes #1488 by switching on WheelEvent.deltaMode #1489

Closed
wants to merge 3 commits into from
Closed

Conversation

pobocks
Copy link
Contributor

@pobocks pobocks commented Mar 20, 2019

Description

Takes into account WheelEvent.deltaMode, multiplying scrollAmount by wrapper height if it's page, or former default if it's line (finding actual line-height might be preferable, but this restores status quo and is much easier.

Related JIRA Ticket or GitHub Issue

#1488

Motivation and Context

Scrolling is broken for sure on Firefox and maybe elsewhere.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@lmcglohon lmcglohon self-requested a review April 4, 2019 12:59
@lmcglohon lmcglohon self-assigned this Apr 4, 2019
@lmcglohon
Copy link
Contributor

@pobocks I believe this will introduce the original issue that was causing the scrolling to go too fast in chrome and causing accessibility issues but it looks as if it fixes the scrolling issue in FireFox. We need a better solution to account for both.

@pobocks
Copy link
Contributor Author

pobocks commented Apr 4, 2019

@lmcglohon So, I can't speak to mobile - I need to test this - but it definitely shouldn't (and didn't when I checked) reintroduce problems on Chrome - Chrome defaults to/uses per-pixel scrolling, so this does the same thing as current code on Chrome (and any other per-line environment), while reverting to the heuristic "guess how much to scroll" on Firefox (and any other per-line env)

@pobocks
Copy link
Contributor Author

pobocks commented Apr 16, 2019

@lmcglohon just pinging this to make sure previous comment got seen.

@lmcglohon
Copy link
Contributor

@pobocks Saw your commit today. Should be able to review it early next week.

@pobocks
Copy link
Contributor Author

pobocks commented Apr 19, 2019

@lmcglohon excellent! Thanks! It's now getting the actual line-height of the wrapper, which should be more or less rigorously correct, though it feels maybe a touch slow.

Whomever tests it should take note, it needs to be tested with an actual mouse, FF uses per-pixel mode with trackpads and other continuous scrolling peripherals.

@andrew-morrison
Copy link
Contributor

We'd really like to fix the slow scrolling in the collection organization view in Firefox. So I have just tested this using two methods:

  • By switching my development system to the "issue_1488" branch, and running it without any plug-ins or config changes.
  • By replicating the relevant JavaScript and template changes in https://github.com/harvard-library/aspace-hvd-pui in our local plug-in and running that on a test instance of a production system on v2.5.1.

In both cases, it made scrolling with the mouse scroll wheel in Firefox 72 much, much too fast. The merest touch sends it to middle of a huge collection, then another skips straight to the end. But it has no effect in Chrome 79. Tested on Ubuntu and macOS (scrolling with gestures on a trackpad works fine in the latter.)

@andrew-morrison
Copy link
Contributor

Forgot to mention: the scroll on the Harvard "HOLLIS for Archival Discovery" web site seems to work fine when tested with the same web browsers on the same OSes, so perhaps there is something in your CSS which is crucial to make this fix work?

@pobocks
Copy link
Contributor Author

pobocks commented Feb 10, 2020

Huh. I'll try to replicate and poke at this - the fix should be identical to the aspace-hvd-pui code, but I'll look at the CSS, and verify that I didn't update one place and not the other.

@lmcglohon lmcglohon removed their request for review February 14, 2020 14:19
@lmcglohon lmcglohon removed their assignment Feb 14, 2020
@cdibella
Copy link
Contributor

Given the age of this pull request and since the code in it does not resolve the problem without creating a different problem we're going to close it. @pobocks or @andrew-morrison (or anyone else reading this) we definitely encourage a new pull request if there's code that fixes this issue for all (or most) scenarios.

@cdibella cdibella closed this Jul 24, 2020
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.

None yet

5 participants