-
Notifications
You must be signed in to change notification settings - Fork 29.7k
WidgetController.startGesture trackpad support #114312
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
Conversation
dnfield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great improvement, thanks! Some style/formatting nits, otherwise LGTM
2cb43c2 to
e701fcb
Compare
e701fcb to
3732770
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
dnfield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will look into google failures
|
Having trouble telling, but it looks like the internal roll pulled in unrelated tool changes and is failing on that. |
|
There's a real failure internally. The test is failing on these lines: final input = await tester.createGesture(kind: testEntry.key);
await input.moveTo(const Offset(50, 50));Asserts |
|
and final pointerDeviceToInputType = {
PointerDeviceKind.invertedStylus: InputType.touch,
PointerDeviceKind.mouse: InputType.keyboardAndMouse,
PointerDeviceKind.stylus: InputType.touch,
PointerDeviceKind.touch: InputType.touch,
PointerDeviceKind.trackpad: InputType.keyboardAndMouse,
PointerDeviceKind.unknown: InputType.touch,
}; |
|
Ahh I see, this test just needs to be updated to skip trackPad now. |
|
I couldn't implement moveTo since a gesture only makes sense with relative movements, not absolute ones. Strange it's only failing on that now, it should have been hitting the later assertion anyways. |
|
I've updated the internal test and AFAICT this will now pass. |
|
@dnfield What do you recommend to do now about flutter-gold ? |
|
auto label is removed for flutter/flutter, pr: 114312, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Flutter gold seems to be in an error state fo rthis PR and I'm not sure why. @Piinks? |
|
I had the same issue in #97928 but that one was 6 months+ old so seemed reasonable. Maybe I should recreate this one as well? |
|
Filed for follow up: https://bugs.chromium.org/p/skia/issues/detail?id=13882 https://flutter-gold.skia.org/json/v1/changelist_summary/github/114312 shows that there are three image changes, but the dashboard does not show anything. If you run the tests locally are there image failures? Local execution will output the affected images. Also relevant: https://bugs.chromium.org/p/skia/issues/detail?id=13847 it was de-duped with another issue I filed for it. We've been seeing this error pop up more frequently, but functionality has been unaffected - but that may have changed.. I will follow up with the Gold team tomorrow |
If it's not a problem, would you mind just closing this PR and opening a new one? Sorry about this. |
|
Recreated at #114631 |
WidgetController.startGesture starts with a down event. It's updated here to start with a panZoomStart event if kind is set to
PointerDeviceKind.trackpad. ThemoveByandupmethod have also been updated so that theTestGesturecan be used in the same way for trackpad as other device kinds, but sending appropriate gesture events for a trackpad. Added some more descriptive assertions when trying to send inappropriate events on a trackpad pointer.Fixes #113773
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on [Discord].