Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Convert Safari and Firefox wheel delta to physical pixels. #35428

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

knopp
Copy link
Member

@knopp knopp commented Aug 16, 2022

Chrome on macOS seems to be using physical pixels when reporting scroll delta, while Safari and Firefox are reporting logical pixels. This makes scrolling on Safari and Firefox feel much slower even if there isn't any jank.

You can verify this by trying to scroll on non-hdpi screen where all browsers scroll with consistent speed.

This makes PR makes scrolling consistent with Chrome and partially fixes flutter/flutter#75850

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

This makes it consistent with Chrome and partially fixes flutter/flutter#75850
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Aug 16, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@knopp
Copy link
Member Author

knopp commented Aug 16, 2022

/cc @yjbanov

@knopp knopp requested a review from yjbanov August 22, 2022 10:42
@mdebbar mdebbar self-requested a review August 22, 2022 18:46
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Could you please add a test for your change so we can move the PR forward? This would be the right file to look at: https://github.com/flutter/engine/blob/main/lib/web_ui/test/engine/pointer_binding_test.dart

@knopp
Copy link
Member Author

knopp commented Sep 29, 2022

Thanks! Apologies, this completely slipped my mind.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 29, 2022
@mdebbar
Copy link
Contributor

mdebbar commented Sep 29, 2022

@knopp no problem! We look forward to more of your contributions 🙂

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 29, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 29, 2022

auto label is removed for flutter/engine, pr: 35428, due to - The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/35428

@knopp
Copy link
Member Author

knopp commented Dec 12, 2023

@mdebbar, seems like this now also needs to be done on Chromium?

knopp added a commit that referenced this pull request Dec 18, 2023
This has been previously done for Safari in Firefox in
#35428. I'm not entirely sure
since which release, but for past couple of months Chromium seems to be
reporting logical pixels during scroll events as well, which now makes
scrolling on Chrome / macOs 2x slower.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web: scrolling behaviour on Mac does not match platform
3 participants