Interactive viewer doesn't appear to respect the trackpadScrollCausesScale parameter #127114
Conversation
flutter-dashboard
bot
added
the
framework
|
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. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
JsGjKJzi
force-pushed
the
interactive_respect_trackpadScrollCausesScale
branch
from
69899b8 to
a3a3d38
Compare
|
Fix looks correct, but will need a test in Probably just a few lines, adding a use of |
|
Ok I haven't dug into testing yet so it'll take me a moment to learn the ropes. I also need to think about the possible cases. Maybe it's simple, but I just want to think it through carefully. Hope to have a test added on Monday. |
|
Actually, it looks like a test case already exists. At first glance it seems to cover this scenario. I'll play with it and try to understand why the test case doesn't seem to jive with actual behavior. Test Code testWidgets('trackpadScrollCausesScale', (WidgetTester tester) async {
final TransformationController transformationController = TransformationController();
const double boundaryMargin = 50.0;
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Center(
child: InteractiveViewer(
boundaryMargin: const EdgeInsets.all(boundaryMargin),
transformationController: transformationController,
trackpadScrollCausesScale: true,
child: const SizedBox(width: 200.0, height: 200.0),
),
),
),
),
);
expect(transformationController.value.getMaxScaleOnAxis(), 1.0);
// Send a vertical scroll.
final TestPointer pointer = TestPointer(1, PointerDeviceKind.trackpad);
final Offset center = tester.getCenter(find.byType(SizedBox));
await tester.sendEventToBinding(pointer.panZoomStart(center));
await tester.pump();
expect(transformationController.value.getMaxScaleOnAxis(), 1.0);
await tester.sendEventToBinding(pointer.panZoomUpdate(center, pan: const Offset(0, -81)));
await tester.pump();
expect(transformationController.value.getMaxScaleOnAxis(), moreOrLessEquals(1.499302500056767));
// Send a horizontal scroll (should have no effect).
await tester.sendEventToBinding(pointer.panZoomUpdate(center, pan: const Offset(81, -81)));
await tester.pump();
expect(transformationController.value.getMaxScaleOnAxis(), moreOrLessEquals(1.499302500056767));
}); |
|
The current test works for "proper trackpad events" which use the full PointerPanZoom sequence. But on web, we don't have those events (not enough info provided by web APIs). Instead we have PointerScrollEvents with |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
+1 to what @moffatman said. Otherwise this looks good, thanks for catching this! I'm happy to rereview when you have a test.
|
Ok, I've added a test case to cover this event specifically. I verified locally that the test does not pass without the patch, and does pass with the patch, and the outcome of other test does not change. I'm not sure if there's a way to do that here to verify that the test case. @justinmc Could you please review as appropriate when you have a moment? Thanks in advance. @moffatman Thank you for pointing me in the right direction on the test! |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM, thanks for checking that this test fails on master.
github-actions
bot
removed
the
framework
|
trying to link issue... fixes #127111 |
|
@moffatman Could you give a secondary review? |
|
Thanks! |
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
flutter/flutter@ee162e4...3db9504 2023-05-28 [email protected] Roll Flutter Engine from 340936f7c03c to a4d4ad9dae1a (1 revision) (flutter/flutter#127772) 2023-05-28 [email protected] Roll Flutter Engine from 57e491a46e05 to 340936f7c03c (1 revision) (flutter/flutter#127767) 2023-05-28 [email protected] Roll Flutter Engine from a37b1a38d97a to 57e491a46e05 (1 revision) (flutter/flutter#127765) 2023-05-28 [email protected] Roll Flutter Engine from b1698020bd57 to a37b1a38d97a (1 revision) (flutter/flutter#127761) 2023-05-27 [email protected] Roll Flutter Engine from 9e97c7379c7f to b1698020bd57 (1 revision) (flutter/flutter#127758) 2023-05-27 [email protected] Roll Flutter Engine from 0f38a0f0a18a to 9e97c7379c7f (1 revision) (flutter/flutter#127755) 2023-05-27 [email protected] Roll Flutter Engine from 1187168472a7 to 0f38a0f0a18a (1 revision) (flutter/flutter#127752) 2023-05-27 [email protected] Roll Flutter Engine from 3426cb827efb to 1187168472a7 (1 revision) (flutter/flutter#127745) 2023-05-27 [email protected] Roll Flutter Engine from 795db50e7453 to 3426cb827efb (9 revisions) (flutter/flutter#127738) 2023-05-27 [email protected] Updated the `ToggleButtons` API doc to link to `SegmentedButton` (flutter/flutter#127021) 2023-05-26 [email protected] Remove more `test_api/src` imports (flutter/flutter#127716) 2023-05-26 [email protected] Move shared inline widget logic to `RenderInlineWidgetContainerDefaults` (flutter/flutter#127308) 2023-05-26 [email protected] Roll Flutter Engine from 3b5b5fc96b8f to 795db50e7453 (1 revision) (flutter/flutter#127720) 2023-05-26 [email protected] Update collection-fors to prefer final (as per updated `prefer_final_in_for_each`) (flutter/flutter#127511) 2023-05-26 [email protected] Roll Flutter Engine from eed12f36f595 to 3b5b5fc96b8f (1 revision) (flutter/flutter#127713) 2023-05-26 [email protected] Remove references to deprecated `ThemeData.primaryColorBrightness` (flutter/flutter#127238) 2023-05-26 [email protected] TwoDimensional scrolling foundation (flutter/flutter#125437) 2023-05-26 [email protected] Roll Flutter Engine from 858d9753453c to eed12f36f595 (2 revisions) (flutter/flutter#127702) 2023-05-26 [email protected] Remove button announcement for `MenuItemButton` and `SubmenuButton` (flutter/flutter#127620) 2023-05-26 [email protected] Reland "Update labeler action wildcards #127524" (flutter/flutter#127690) 2023-05-26 [email protected] Revert "Log all lines from ios-deploy (#127502)" (flutter/flutter#127684) 2023-05-26 [email protected] Roll Flutter Engine from ec7b0ae3599f to 858d9753453c (2 revisions) (flutter/flutter#127700) 2023-05-26 [email protected] Add M3 date range picker tests and fix header background theme color (flutter/flutter#127662) 2023-05-26 [email protected] Support minifcation for apps that depend on AGP 8 and integration_test (flutter/flutter#127628) 2023-05-26 [email protected] Bump Snippet version (flutter/flutter#127688) 2023-05-26 [email protected] [Linux] Use up_client_get_devices2 when possible (flutter/flutter#127699) 2023-05-26 [email protected] Roll Flutter Engine from 84f2fc16e55d to ec7b0ae3599f (3 revisions) (flutter/flutter#127698) 2023-05-26 [email protected] Fix `TextField` error in production environment because it wrongly uses ancestor render boxes (flutter/flutter#126324) 2023-05-26 [email protected] Interactive viewer doesn't appear to respect the trackpadScrollCausesScale parameter (flutter/flutter#127114) 2023-05-26 [email protected] Remove release timeline (flutter/flutter#127685) 2023-05-26 [email protected] Manual roll Flutter Engine from 8573f3b63a1f to 84f2fc16e55d (7 revisions) (flutter/flutter#127676) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
…llCausesScale parameter (flutter/flutter#127114)
3 participants
PR #114280 does not actually restore the functionality to pre #114280 behavior, where a two-finger scroll up/down on a trackpad results in a scale (zoom) even when the trackpadScrollCausesScale parameter is set to
true.This PR causes InteractiveViewer to behave as expected and fixes issue #127111.
Pre-launch Checklist
///).